[Asterisk-code-review] chan_sip.c: Prevent address change on unauthenticated SIP request. (asterisk[17])

[Friendly Automation]

Hello Benjamin Keith Ford,

I'd like you to do a code review. Please visit

    https://gerrit.asterisk.org/c/asterisk/+/13238

to review the following change.


Change subject: chan_sip.c: Prevent address change on unauthenticated SIP request.
......................................................................

chan_sip.c: Prevent address change on unauthenticated SIP request.

If the name of a peer is known and a SIP request is sent using that
peer's name, the address of the peer will change even if the request
fails the authentication challenge. This means that an endpoint can
be altered and even rendered unusuable, even if it was in a working
state previously. This can only occur when the nat option is set to the
default, or auto_force_rport.

This change checks the result of authentication first to ensure it is
successful before setting the address and the nat option.

ASTERISK-28589 #close

Change-Id: I581c5ed1da60ca89f590bd70872de2b660de02df
---
M channels/chan_sip.c
1 file changed, 16 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/38/13238/1

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 80040ae..72b2851 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -19245,18 +19245,6 @@
 		bogus_peer = NULL;
 	}
 
-	/*  build_peer, called through sip_find_peer, is not able to check the
-	 *  sip_pvt->natdetected flag in order to determine if the peer is behind
-	 *  NAT or not when SIP_PAGE3_NAT_AUTO_RPORT or SIP_PAGE3_NAT_AUTO_COMEDIA
-	 *  are set on the peer.  So we check for that here and set the peer's
-	 *  address accordingly.
-	 */
-	set_peer_nat(p, peer);
-
-	if (p->natdetected && ast_test_flag(&peer->flags[2], SIP_PAGE3_NAT_AUTO_RPORT)) {
-		ast_sockaddr_copy(&peer->addr, &p->recv);
-	}
-
 	if (!ast_apply_acl(peer->acl, addr, "SIP Peer ACL: ")) {
 		ast_debug(2, "Found peer '%s' for '%s', but fails host access\n", peer->name, of);
 		sip_unref_peer(peer, "sip_unref_peer: check_peer_ok: from sip_find_peer call, early return of AUTH_ACL_FAILED");
@@ -19325,6 +19313,21 @@
 		ast_string_field_set(p, peermd5secret, NULL);
 	}
 	if (!(res = check_auth(p, req, peer->name, p->peersecret, p->peermd5secret, sipmethod, uri2, reliable))) {
+
+		/* build_peer, called through sip_find_peer, is not able to check the
+		 * sip_pvt->natdetected flag in order to determine if the peer is behind
+		 * NAT or not when SIP_PAGE3_NAT_AUTO_RPORT or SIP_PAGE3_NAT_AUTO_COMEDIA
+		 * are set on the peer. So we check for that here and set the peer's
+		 * address accordingly. The address should ONLY be set once we are sure
+		 * authentication was a success. If, for example, an INVITE was sent that
+		 * matched the peer name but failed the authentication check, the address
+		 * would be updated, which is bad.
+		 */
+		set_peer_nat(p, peer);
+		if (p->natdetected && ast_test_flag(&peer->flags[2], SIP_PAGE3_NAT_AUTO_RPORT)) {
+			ast_sockaddr_copy(&peer->addr, &p->recv);
+		}
+
 		/* If we have a call limit, set flag */
 		if (peer->call_limit)
 			ast_set_flag(&p->flags[0], SIP_CALL_LIMIT);
@@ -19424,6 +19427,7 @@
 		}
 	}
 	sip_unref_peer(peer, "check_peer_ok: sip_unref_peer: tossing temp ptr to peer from sip_find_peer");
+
 	return res;
 }
 

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13238
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: I581c5ed1da60ca89f590bd70872de2b660de02df
Gerrit-Change-Number: 13238
Gerrit-PatchSet: 1
Gerrit-Owner: Friendly Automation
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191121/abb42b0b/attachment.html>