[Asterisk-code-review] res_pjsip_registrar.c: Prevent potential double free if AOR is not found (asterisk[17])

[Friendly Automation]

Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/13360 )

Change subject: res_pjsip_registrar.c: Prevent potential double free if AOR is not found
......................................................................

res_pjsip_registrar.c: Prevent potential double free if AOR is not found

The simple fix here is simply to NULL out username and password after we call
ast_free on them. Unfortunately, I noticed that we weren't checking for
allocation failures for username and password, and adding those checks made
things noisy and cumbersome.

So instead we partially rollback the recent LGTM patch, and move the alloca
calls into find_aor_name().

ASTERISK-28641 #close
Reported by: Ross Beer

Change-Id: Ic9d01624e717a020be0b0aee31f0814e7f1ffbe2
---
M res/res_pjsip_registrar.c
1 file changed, 22 insertions(+), 21 deletions(-)

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



diff --git a/res/res_pjsip_registrar.c b/res/res_pjsip_registrar.c
index 953be20..172ecc8 100644
--- a/res/res_pjsip_registrar.c
+++ b/res/res_pjsip_registrar.c
@@ -949,14 +949,21 @@
 	return 0;
 }
 
-static char *find_aor_name(const char *username, const char *domain, const char *aors)
+static char *find_aor_name(const pj_str_t *pj_username, const pj_str_t *pj_domain, const char *aors)
 {
 	char *configured_aors;
 	char *aors_buf;
 	char *aor_name;
 	char *id_domain;
+	char *username, *domain;
 	struct ast_sip_domain_alias *alias;
 
+	/* Turn these into C style strings for convenience */
+	username = ast_alloca(pj_strlen(pj_username) + 1);
+	ast_copy_pj_str(username, pj_username, pj_strlen(pj_username) + 1);
+	domain = ast_alloca(pj_strlen(pj_domain) + 1);
+	ast_copy_pj_str(domain, pj_domain, pj_strlen(pj_domain) + 1);
+
 	id_domain = ast_alloca(strlen(username) + strlen(domain) + 2);
 	sprintf(id_domain, "%s@%s", username, domain);
 
@@ -1006,11 +1013,10 @@
 {
 	struct ast_sip_aor *aor = NULL;
 	char *aor_name = NULL;
-	char *domain_name = NULL;
-	char *username = NULL;
 	int i;
 
 	for (i = 0; i < AST_VECTOR_SIZE(&endpoint->ident_method_order); ++i) {
+		pj_str_t username;
 		pjsip_sip_uri *uri;
 		pjsip_authorization_hdr *header = NULL;
 
@@ -1018,18 +1024,22 @@
 		case AST_SIP_ENDPOINT_IDENTIFY_BY_USERNAME:
 			uri = pjsip_uri_get_uri(rdata->msg_info.to->uri);
 
-			domain_name = ast_malloc(uri->host.slen + 1);
-			ast_copy_pj_str(domain_name, &uri->host, uri->host.slen + 1);
-			username = ast_malloc(uri->user.slen + 1);
-			ast_copy_pj_str(username, &uri->user, uri->user.slen + 1);
+			pj_strassign(&username, &uri->user);
 
 			/*
 			 * We may want to match without any user options getting
 			 * in the way.
+			 *
+			 * Logic adapted from AST_SIP_USER_OPTIONS_TRUNCATE_CHECK for pj_str_t.
 			 */
-			AST_SIP_USER_OPTIONS_TRUNCATE_CHECK(username);
+			if (ast_sip_get_ignore_uri_user_options()) {
+				pj_ssize_t semi = pj_strcspn2(&username, ";");
+				if (semi < pj_strlen(&username)) {
+					username.slen = semi;
+				}
+			}
 
-			aor_name = find_aor_name(username, domain_name, endpoint->aors);
+			aor_name = find_aor_name(&username, &uri->host, endpoint->aors);
 			if (aor_name) {
 				ast_debug(3, "Matched aor '%s' by To username\n", aor_name);
 			}
@@ -1038,12 +1048,8 @@
 			while ((header = pjsip_msg_find_hdr(rdata->msg_info.msg, PJSIP_H_AUTHORIZATION,
 				header ? header->next : NULL))) {
 				if (header && !pj_stricmp2(&header->scheme, "digest")) {
-					username = ast_malloc(header->credential.digest.username.slen + 1);
-					ast_copy_pj_str(username, &header->credential.digest.username, header->credential.digest.username.slen + 1);
-					domain_name = ast_malloc(header->credential.digest.realm.slen + 1);
-					ast_copy_pj_str(domain_name, &header->credential.digest.realm, header->credential.digest.realm.slen + 1);
-
-					aor_name = find_aor_name(username, domain_name, endpoint->aors);
+					aor_name = find_aor_name(&header->credential.digest.username,
+						&header->credential.digest.realm, endpoint->aors);
 					if (aor_name) {
 						ast_debug(3, "Matched aor '%s' by Authentication username\n", aor_name);
 						break;
@@ -1058,9 +1064,6 @@
 		if (aor_name) {
 			break;
 		}
-
-		ast_free(domain_name);
-		ast_free(username);
 	}
 
 	if (ast_strlen_zero(aor_name) || !(aor = ast_sip_location_retrieve_aor(aor_name))) {
@@ -1068,11 +1071,9 @@
 		pjsip_endpt_respond_stateless(ast_sip_get_pjsip_endpoint(), rdata, 404, NULL, NULL, NULL);
 		ast_sip_report_req_no_support(endpoint, rdata, "registrar_requested_aor_not_found");
 		ast_log(LOG_WARNING, "AOR '%s' not found for endpoint '%s'\n",
-			username ?: "", ast_sorcery_object_get_id(endpoint));
+			aor_name ?: "", ast_sorcery_object_get_id(endpoint));
 	}
 	ast_free(aor_name);
-	ast_free(domain_name);
-	ast_free(username);
 	return aor;
 }
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: Ic9d01624e717a020be0b0aee31f0814e7f1ffbe2
Gerrit-Change-Number: 13360
Gerrit-PatchSet: 3
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191209/35b04ac6/attachment-0001.html>