[Asterisk-code-review] chan sip: improved ip:port finding of peers for non-UDP tran... (asterisk[13])
Jenkins2
asteriskteam at digium.com
Mon Aug 27 07:43:04 CDT 2018
Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/9859 )
Change subject: chan_sip: improved ip:port finding of peers for non-UDP transports.
......................................................................
chan_sip: improved ip:port finding of peers for non-UDP transports.
Also remove function peer_ipcmp_cb since it's not used (according to
rmudgett).
Prior to b2c4e8660a9c89d07041271371151779b7ec75f6 (ASTERISK_27457)
insecure=port was the defacto standard. That commit also prevented
insecure=port from being applied for sip/tcp or sip/tls.
Into consideration there are three sets of behaviour:
1. "previous" - before the above commit.
2. "current" - post above commit, pre this one.
3. "new" - post this commit.
The problem that the above commit tried to address was guests over TCP.
It succeeded in doing that but broke transport!=udp with host!=dynamic.
This commit attempts to restore sane behaviour with respect to
transport!=udp for host!=dynamic whilst still retaining the guest users
over tcp.
It should be noted that when looking for a peer, two passes are made, the
first pass doesn't have SIP_INSECURE_PORT set for the searched-for peer,
thus looking for full matches (IP + Port), the second pass sets
SIP_INSECURE_PORT, thus expecting matches on IP only where the matched
peer allows for that (in the author's opinion: UDP with insecure=port,
or any TCP based, non-dynamic host).
In previous behaviour there was special handling for transport=tcp|tls
whereby a peer would match during the first pass if the utilized
transport was TCP|TLS (and the peer allowed that specific transport).
This behaviour was wrong, or dubious at best. Consider two dynamic tcp
peers, both registering from the same IP (NAT), in this case either peer
could match for connections from an IP. It's also this behaviour that
prevented SIP guests over tcp.
The above referenced commit removed this behaviour, but kept applying
the SIP_INSECURE_PORT only to WS|WSS|UDP. Since WS and WSS is also TCP
based, the logic here should fall into the TCP category.
This patch updates things such that the previously non-explicit (TCP
behaviour) transport test gets performed explicitly (ie, matched peer
must allow for the used transport), as well as the indeterministic
source-port nature of the TCP protocol is taken into account. The new
match algorithm now looks like:
1. As per previous behaviour, IP address is matched first.
2. Explicit filter with respect to transport protocol, previous
behaviour was semi-implied in the test for TCP pure IP match - this now
made explicit.
3. During first pass (without SIP_INSECURE_PORT), always match on port.
4. If doing UDP, match if matched against peer also has
SIP_INSECURE_PORT, else don't match.
5. Match if not a dynamic host (for non-UDP protocols)
6. Don't match if this is WS|WSS, or we can't trust the Contact address
(presumably due to NAT)
7. Match (we have a valid Contact thus if the IP matches we have no
choice, this will likely only apply to non-NAT).
To logic-test this we need a few different scenarios. Towards this end,
I work with a set number of peers defined in sip.conf:
[peer1]
host=1.1.1.1
transport=tcp
[peer2]
host=1.1.1.1
transport=udp
[peer3]
host=1.1.1.1
port=5061
insecure=port
transport=udp
[peer4]
host=1.1.1.2
transport=udp,tcp
[peer5]
host=dynamic
transport=udp,tcp
Test cases for UDP:
1 - incoming UDP request from 1.1.1.1:
- previous:
- pass 1:
* peer1 or peer2 if from port 5060 (indeterminate, depends on peer
ordering)
* peer3 if from port 5061
* peer5 if registered from 1.1.1.1 and source port matches
- pass 2:
* peer3
- current: as per previous.
- new:
- pass 1:
* peer2 if from port 5060
* peer3 if from port 5061
* peer5 if registered from 1.1.1.1 and source port matches
- pass 2:
* peer3
2 - incoming UDP request from 1.1.1.2:
- previous:
- pass 1:
* peer5 if registered from 1.1.1.2 and port matches
* peer4 if source port is 5060
- pass 2:
* no match (guest)
- current: as previous.
- new as previous (with the variation that if peer5 didn't have udp as
allowed transport it would not match peer5 whereas previous
and current code could).
3 - incoming UDP request from anywhere else:
- previous:
- pass 1:
* peer5 if registered from that address and source port matches.
- pass 2:
* peer5 if insecure=port is additionally set.
* no match (guest)
- current - as per previous
- new - as per previous
Test cases for TCP based transports:
4 - incoming TCP request from 1.1.1.1
- previous:
- pass 1 (indeterministic, depends on ordering of peers in memory):
* peer1; or
* peer5 if peer5 registered from 1.1.1.1 (irrespective of source port); or
* peer2 if the source port happens to be 5060; or
* peer3 if the source port happens to be 5061.
- pass 2: cannot happen since pass 1 will always find a peer.
- current:
- pass 1:
* peer1 or peer2 if from source port 5060
* peer3 if from source port 5060
* peer5 if registered as 1.1.1.1 and source port matches
- pass 2:
* no match (guest)
- new:
- pass 1:
* peer 1 if from port 5060
* peer 5 if registered and source port matches
- pass 2:
* peer 1
5 - incoming TCP request from 1.1.1.2
- previous (indeterminate, depends on ordering):
- pass 1:
* peer4; or
* peer5 if peer5 registered from 1.1.1.2
- pass 2: cannot happen since pass 1 will always find a peer.
- current:
- pass 1:
* peer4 if source port is 5060
* peer5 if peer5 registered as 1.1.1.2 and source port matches
- pass 2:
* no match (guest).
- new:
- pass 1:
* peer4 if source port is 5060
* peer5 if peer5 registered as 1.1.1.2 and source port matches
- pass 2:
* peer4
6 - incoming TCP request from anywhere else:
- previous:
- pass 1:
* peer5 if registered from that address
- pass 2: cannot happen since pass 1 will always find a peer.
- current:
- pass 1:
* peer5 if registered from that address and port matches.
- pass 2:
* no match (guest)
- new: as per current.
It should be noted the test cases don't make explicit mention of TLS, WS
or WSS. WS and WSS previously followed UDP semantics, they will now
enforce source port matching. TLS follow TCP semantics.
The previous commit specifically tried to address test-case 6, but broke
test-cases 4 and 5 in the process.
ASTERISK-27881 #close
Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2
---
M channels/chan_sip.c
1 file changed, 45 insertions(+), 22 deletions(-)
Approvals:
Richard Mudgett: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 47fd40c..af4f06d 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -34157,17 +34157,14 @@
* This function has two modes.
* - If the peer arg does not have INSECURE_PORT set, then we will only return
* a match for a peer that matches both the IP and port.
- * - If the peer arg does have the INSECURE_PORT flag set, then we will only
- * return a match for a peer that matches the IP and has insecure=port
- * in its configuration.
+ * - If the peer arg does have the INSECURE_PORT flag set, then we will return
+ * a match for UDP peers with insecure=port set, or a peer that does NOT have
+ * host=dynamic for other protocols (or have a valid Contact: header in REGISTER).
+ * This callback will be used twice when doing peer matching, as per the two modes
+ * described above.
*
- * This callback will be used twice when doing peer matching. There is a first
- * pass for full IP+port matching, and a second pass in case there is a match
- * that meets the insecure=port criteria.
- *
- * \note Connections coming in over TCP or TLS should never be matched by port.
- *
- * \note the peer's addr struct provides to fields combined to make a key: the sin_addr.s_addr and sin_port fields.
+ * \note the peer's addr struct provides to fields combined to make a key: the
+ * sin_addr.s_addr and sin_port fields (transport is compared separately).
*/
static int peer_ipcmp_cb_full(void *obj, void *arg, void *data, int flags)
{
@@ -34186,24 +34183,50 @@
return 0;
}
- /* We matched the IP, check to see if we need to match by port as well. */
- if (((peer->transports & peer2->transports) &
- (AST_TRANSPORT_UDP | AST_TRANSPORT_WS | AST_TRANSPORT_WSS)) &&
- ast_test_flag(&peer2->flags[0], SIP_INSECURE_PORT)) {
+ if ((peer->transports & peer2->transports) == 0) {
+ /* transport setting doesn't match */
+ return 0;
+ }
+
+ if (!ast_test_flag(&peer2->flags[0], SIP_INSECURE_PORT)) {
+ /* On the first pass only match if ports match. */
+ return ast_sockaddr_port(&peer->addr) == ast_sockaddr_port(&peer2->addr) ?
+ (CMP_MATCH | CMP_STOP) : 0;
+ }
+
+ /* We can reach here only if peer2 is for SIP_INSECURE_PORT, in
+ * other words, the second pass where we only try to match against IP.
+ *
+ * Some special handling for UDP vs non-UDP (TCP, TLS, WS and WSS), since
+ * for non-UDP the source port won't typically be controlled, we only want
+ * to check the source IP, but only if the host isn't dynamic. This isn't
+ * done in the first pass so that if a peer registers from the same IP as
+ * a static IP peer that registration (port match) will take prescedence).
+ */
+ if (peer2->transports == AST_TRANSPORT_UDP) {
/* We are allowing match without port for peers configured that
* way in this pass through the peers. */
return ast_test_flag(&peer->flags[0], SIP_INSECURE_PORT) ?
(CMP_MATCH | CMP_STOP) : 0;
}
- /* Now only return a match if the port matches, as well. */
- return ast_sockaddr_port(&peer->addr) == ast_sockaddr_port(&peer2->addr) ?
- (CMP_MATCH | CMP_STOP) : 0;
-}
+ if (!peer->host_dynamic) {
+ return CMP_MATCH | CMP_STOP;
+ }
-static int peer_ipcmp_cb(void *obj, void *arg, int flags)
-{
- return peer_ipcmp_cb_full(obj, arg, NULL, flags);
+ /* Conditions taken from parse_register_contact() */
+ if (peer2->transports & (AST_TRANSPORT_WS | AST_TRANSPORT_WSS)) {
+ /* The contact address of websockets is always the transport source address and port */
+ return 0;
+ }
+
+ if (ast_test_flag(&peer->flags[0], SIP_NAT_FORCE_RPORT)) {
+ /* The contact address of NATed peers is always the transport source address and port */
+ return 0;
+ }
+
+ /* Have to assume that we used the registered contact header (non-NAT) */
+ return CMP_MATCH | CMP_STOP;
}
static int threadt_hash_cb(const void *obj, const int flags)
@@ -35325,7 +35348,7 @@
/* the fact that ao2_containers can't resize automatically is a major worry! */
/* if the number of objects gets above MAX_XXX_BUCKETS, things will slow down */
peers = ao2_t_container_alloc(HASH_PEER_SIZE, peer_hash_cb, peer_cmp_cb, "allocate peers");
- peers_by_ip = ao2_t_container_alloc(HASH_PEER_SIZE, peer_iphash_cb, peer_ipcmp_cb, "allocate peers_by_ip");
+ peers_by_ip = ao2_t_container_alloc(HASH_PEER_SIZE, peer_iphash_cb, NULL, "allocate peers_by_ip");
dialogs = ao2_t_container_alloc(HASH_DIALOG_SIZE, dialog_hash_cb, dialog_cmp_cb, "allocate dialogs");
dialogs_needdestroy = ao2_t_container_alloc(1, NULL, NULL, "allocate dialogs_needdestroy");
dialogs_rtpcheck = ao2_t_container_alloc(HASH_DIALOG_SIZE, dialog_hash_cb, dialog_cmp_cb, "allocate dialogs for rtpchecks");
--
To view, visit https://gerrit.asterisk.org/9859
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2
Gerrit-Change-Number: 9859
Gerrit-PatchSet: 4
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180827/c6e63a61/attachment-0001.html>
More information about the asterisk-code-review
mailing list