[Asterisk-code-review] res/res pjsip: Standardize/fix localnet checks across pjsip. (asterisk[15])

Joshua Colp asteriskteam at digium.com
Wed Sep 6 10:13:29 CDT 2017 

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/6397 )

Change subject: res/res_pjsip: Standardize/fix localnet checks across pjsip.
......................................................................

res/res_pjsip: Standardize/fix localnet checks across pjsip.

In 2dee95cc (ASTERISK-27024) and 776ffd77 (ASTERISK-26879) there was
confusion about whether the transport_state->localnet ACL has ALLOW or
DENY semantics.

For the record: the localnet has DENY semantics, meaning that "not in
the list" means ALLOW, and the local nets are in the list.

Therefore, checks like this look wrong, but are right:

    /* See if where we are sending this request is local or not, and if
       not that we can get a Contact URI to modify */
    if (ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW) {
        ast_debug(5, "Request is being sent to local address, "
                     "skipping NAT manipulation\n");

(In the list == localnet == DENY == skip NAT manipulation.)

And conversely, other checks that looked right, were wrong.

This change adds two macro's to reduce the confusion and uses those
instead:

    ast_sip_transport_is_nonlocal(transport_state, addr)
    ast_sip_transport_is_local(transport_state, addr)

ASTERISK-27248 #close

Change-Id: Ie7767519eb5a822c4848e531a53c0fd054fae934
---
M include/asterisk/res_pjsip.h
M main/acl.c
M res/res_pjsip/config_transport.c
M res/res_pjsip_nat.c
M res/res_pjsip_sdp_rtp.c
M res/res_pjsip_session.c
M res/res_pjsip_t38.c
7 files changed, 19 insertions(+), 11 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve; Approved for Submit
  George Joseph: Looks good to me, approved



diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index e2c487a..ad88138 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -98,7 +98,10 @@
 	 */
 	pj_ssl_cipher ciphers[SIP_TLS_MAX_CIPHERS];
 	/*!
-	 * Optional local network information, used for NAT purposes
+	 * Optional local network information, used for NAT purposes.
+	 * "deny" (set) means that it's in the local network. Use the
+	 * ast_sip_transport_is_nonlocal and ast_sip_transport_is_local
+	 * macro's.
 	 * \since 13.8.0
 	 */
 	struct ast_ha *localnet;
@@ -124,6 +127,12 @@
 	struct ast_sockaddr external_media_address;
 };
 
+#define ast_sip_transport_is_nonlocal(transport_state, addr) \
+	(!transport_state->localnet || ast_apply_ha(transport_state->localnet, addr) == AST_SENSE_ALLOW)
+
+#define ast_sip_transport_is_local(transport_state, addr) \
+	(transport_state->localnet && ast_apply_ha(transport_state->localnet, addr) != AST_SENSE_ALLOW)
+
 /*
  * \brief Transport to bind to
  */
diff --git a/main/acl.c b/main/acl.c
index 3194567..6868ea1 100644
--- a/main/acl.c
+++ b/main/acl.c
@@ -737,8 +737,8 @@
 		char iabuf[INET_ADDRSTRLEN];
 		char iabuf2[INET_ADDRSTRLEN];
 		/* DEBUG */
-		ast_copy_string(iabuf, ast_inet_ntoa(sin->sin_addr), sizeof(iabuf));
-		ast_copy_string(iabuf2, ast_inet_ntoa(ha->netaddr), sizeof(iabuf2));
+		ast_copy_string(iabuf, ast_sockaddr_stringify(addr), sizeof(iabuf));
+		ast_copy_string(iabuf2, ast_sockaddr_stringify(&current_ha->addr), sizeof(iabuf2));
 		ast_debug(1, "##### Testing %s with %s\n", iabuf, iabuf2);
 #endif
 		if (ast_sockaddr_is_ipv4(&current_ha->addr)) {
diff --git a/res/res_pjsip/config_transport.c b/res/res_pjsip/config_transport.c
index 5f7eafa..0c804b8 100644
--- a/res/res_pjsip/config_transport.c
+++ b/res/res_pjsip/config_transport.c
@@ -1127,7 +1127,9 @@
 		return 0;
 	}
 
-	if (!(state->localnet = ast_append_ha("d", var->value, state->localnet, &error))) {
+	/* We use only the ast_apply_ha() which defaults to ALLOW
+	 * ("permit"), so we add DENY rules. */
+	if (!(state->localnet = ast_append_ha("deny", var->value, state->localnet, &error))) {
 		return -1;
 	}
 
diff --git a/res/res_pjsip_nat.c b/res/res_pjsip_nat.c
index 45b0d7c..e1d56e6 100644
--- a/res/res_pjsip_nat.c
+++ b/res/res_pjsip_nat.c
@@ -267,7 +267,7 @@
 		ast_sockaddr_set_port(&addr, tdata->tp_info.dst_port);
 
 		/* See if where we are sending this request is local or not, and if not that we can get a Contact URI to modify */
-		if (ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW) {
+		if (ast_sip_transport_is_local(transport_state, &addr)) {
 			ast_debug(5, "Request is being sent to local address, skipping NAT manipulation\n");
 			return PJ_SUCCESS;
 		}
diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c
index b082b1d..9110a1c 100644
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -1818,8 +1818,7 @@
 	ast_sockaddr_parse(&addr, host, PARSE_PORT_FORBID);
 
 	/* Is the address within the SDP inside the same network? */
-	if (transport_state->localnet
-		&& ast_apply_ha(transport_state->localnet, &addr) == AST_SENSE_ALLOW) {
+	if (ast_sip_transport_is_local(transport_state, &addr)) {
 		return;
 	}
 	ast_debug(5, "Setting media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index f6b3b93..545113f 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -3968,8 +3968,7 @@
 		ast_copy_pj_str(host, &sdp->conn->addr, sizeof(host));
 		ast_sockaddr_parse(&addr, host, PARSE_PORT_FORBID);
 
-		if (!transport_state->localnet
-			|| ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW) {
+		if (ast_sip_transport_is_nonlocal(transport_state, &addr)) {
 			ast_debug(5, "Setting external media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));
 			pj_strdup2(tdata->pool, &sdp->conn->addr, ast_sockaddr_stringify_host(&transport_state->external_media_address));
 		}
diff --git a/res/res_pjsip_t38.c b/res/res_pjsip_t38.c
index 27eff42..2e60493 100644
--- a/res/res_pjsip_t38.c
+++ b/res/res_pjsip_t38.c
@@ -963,8 +963,7 @@
 	ast_sockaddr_parse(&addr, host, PARSE_PORT_FORBID);
 
 	/* Is the address within the SDP inside the same network? */
-	if (transport_state->localnet
-		&& ast_apply_ha(transport_state->localnet, &addr) == AST_SENSE_ALLOW) {
+	if (ast_sip_transport_is_local(transport_state, &addr)) {
 		return;
 	}
 	ast_debug(5, "Setting media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));

-- 
To view, visit https://gerrit.asterisk.org/6397
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie7767519eb5a822c4848e531a53c0fd054fae934
Gerrit-Change-Number: 6397
Gerrit-PatchSet: 1
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170906/fe595830/attachment.html>

More information about the asterisk-code-review mailing list