[Asterisk-code-review] AST-2018-010: Fix length of buffer needed for SRV and NAPTR ... (asterisk[master])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/10643 )

Change subject: AST-2018-010: Fix length of buffer needed for SRV and NAPTR results
......................................................................

AST-2018-010: Fix length of buffer needed for SRV and NAPTR results

When dn_expand was being called on SRV and NAPTR results, the
return value was being used to calculate the size of the buffer
needed to store the host names.  Since dn_expand returns the
length of the COMPRESSED name the buffer could be too short
to hold the EXPANDED name.  The expanded name is NULL terminated
so using strlen() is the correct way to determine the length
actually needed for the buffer.

ASTERISK-28127
Reported by: Jan Hoffmann

patches:
  patch.diff submitted by janhoffmann (license 6986)

Change-Id: I4d35d6c431c6c6836cb61d37b1378cc47f0b414d
---
M main/dns_naptr.c
M main/dns_srv.c
2 files changed, 20 insertions(+), 6 deletions(-)

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



diff --git a/main/dns_naptr.c b/main/dns_naptr.c
index 5490b55..4d67816 100644
--- a/main/dns_naptr.c
+++ b/main/dns_naptr.c
@@ -393,6 +393,7 @@
 	int replacement_size;
 	const char *end_of_record;
 	enum flags_result flags_res;
+	size_t naptr_len;
 
 	ptr = dns_find_record(data, size, query->result->answer, query->result->answer_size);
 	ast_assert(ptr != NULL);
@@ -435,7 +436,14 @@
 		return NULL;
 	}
 
-	replacement_size = dn_expand((unsigned char *)query->result->answer, (unsigned char *) end_of_record, (unsigned char *) ptr, replacement, sizeof(replacement) - 1);
+	/*
+	 * The return value from dn_expand represents the size of the replacement
+	 * in the buffer which MAY be compressed.  Since the expanded replacement
+	 * is NULL terminated, you can use strlen() to get the expanded size.
+	 */
+	replacement_size = dn_expand((unsigned char *)query->result->answer,
+		(unsigned char *) end_of_record, (unsigned char *) ptr,
+		replacement, sizeof(replacement) - 1);
 	if (replacement_size < 0) {
 		ast_log(LOG_ERROR, "Failed to expand domain name: %s\n", strerror(errno));
 		return NULL;
@@ -475,7 +483,9 @@
 		return NULL;
 	}
 
-	naptr = ast_calloc(1, sizeof(*naptr) + size + flags_size + 1 + services_size + 1 + regexp_size + 1 + replacement_size + 1);
+	naptr_len = sizeof(*naptr) + size + flags_size + 1 + services_size + 1
+		+ regexp_size + 1 + strlen(replacement) + 1;
+	naptr = ast_calloc(1, naptr_len);
 	if (!naptr) {
 		return NULL;
 	}
diff --git a/main/dns_srv.c b/main/dns_srv.c
index b562e32..e11c84e 100644
--- a/main/dns_srv.c
+++ b/main/dns_srv.c
@@ -73,7 +73,13 @@
 		return NULL;
 	}
 
-	host_size = dn_expand((unsigned char *)query->result->answer, (unsigned char *) end_of_record, (unsigned char *) ptr, host, sizeof(host) - 1);
+	/*
+	 * The return value from dn_expand represents the size of the replacement
+	 * in the buffer which MAY be compressed.  Since the expanded replacement
+	 * is NULL terminated, you can use strlen() to get the expanded size.
+	 */
+	host_size = dn_expand((unsigned char *)query->result->answer,
+		(unsigned char *) end_of_record, (unsigned char *) ptr, host, sizeof(host) - 1);
 	if (host_size < 0) {
 		ast_log(LOG_ERROR, "Failed to expand domain name: %s\n", strerror(errno));
 		return NULL;
@@ -83,7 +89,7 @@
 		return NULL;
 	}
 
-	srv = ast_calloc(1, sizeof(*srv) + size + host_size + 1);
+	srv = ast_calloc(1, sizeof(*srv) + size + strlen(host) + 1);
 	if (!srv) {
 		return NULL;
 	}
@@ -94,8 +100,6 @@
 
 	srv->host = srv->data + size;
 	strcpy((char *)srv->host, host); /* SAFE */
-	((char *)srv->host)[host_size] = '\0';
-
 	srv->generic.data_ptr = srv->data;
 
 	return (struct ast_dns_record *)srv;

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4d35d6c431c6c6836cb61d37b1378cc47f0b414d
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
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/20181114/f278ab1a/attachment-0001.html>