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