[asterisk-commits] chan pjsip: Fix PJSIP MEDIA OFFER dialplan function read. (asterisk[14])

[SVN commits to the Asterisk project]

Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5849 )

Change subject: chan_pjsip: Fix PJSIP_MEDIA_OFFER dialplan function read.
......................................................................

chan_pjsip: Fix PJSIP_MEDIA_OFFER dialplan function read.

The construction of the returned string assumed incorrectly that the
supplied buffer would always be initialized as an empty string.  If it is
not an empty string we could overrun the supplied buffer by the length of
the non-empty buffer string plus one.  It is also theoreticaly possible
for the supplied buffer to be overrun by a string terminator during a read
operation even if the supplied buffer is an empty string.

* Fix the assumption that the supplied buffer would already be an empty
string.  The buffer is not guaranteed to contain an empty string by all
possible callers.

* Fix string terminator buffer overrun potential.

Change-Id: If6a0806806527678c8554b1dcb34fd7808aa95c9
---
M channels/pjsip/dialplan_functions.c
1 file changed, 21 insertions(+), 14 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/channels/pjsip/dialplan_functions.c b/channels/pjsip/dialplan_functions.c
index 7d73830..01081e9 100644
--- a/channels/pjsip/dialplan_functions.c
+++ b/channels/pjsip/dialplan_functions.c
@@ -929,36 +929,40 @@
 static int media_offer_read_av(struct ast_sip_session *session, char *buf,
 			       size_t len, enum ast_media_type media_type)
 {
-	int i, size = 0;
+	int idx;
+	size_t accum = 0;
 
-	for (i = 0; i < ast_format_cap_count(session->req_caps); i++) {
-		struct ast_format *fmt = ast_format_cap_get_format(session->req_caps, i);
+	/* Note: buf is not terminated while the string is being built. */
+	for (idx = 0; idx < ast_format_cap_count(session->req_caps); ++idx) {
+		struct ast_format *fmt;
+		size_t size;
 
+		fmt = ast_format_cap_get_format(session->req_caps, idx);
 		if (ast_format_get_type(fmt) != media_type) {
 			ao2_ref(fmt, -1);
 			continue;
 		}
 
-		/* add one since we'll include a comma */
+		/* Add one for a comma or terminator */
 		size = strlen(ast_format_get_name(fmt)) + 1;
 		if (len < size) {
 			ao2_ref(fmt, -1);
 			break;
 		}
+
+		/* Append the format name */
+		strcpy(buf + accum, ast_format_get_name(fmt));/* Safe */
+		ao2_ref(fmt, -1);
+
+		accum += size;
 		len -= size;
 
-		/* no reason to use strncat here since we have already ensured buf has
-                   enough space, so strcat can be safely used */
-		strcat(buf, ast_format_get_name(fmt));
-		strcat(buf, ",");
-
-		ao2_ref(fmt, -1);
+		/* The last comma on the built string will be set to the terminator. */
+		buf[accum - 1] = ',';
 	}
 
-	if (size) {
-		/* remove the extra comma */
-		buf[strlen(buf) - 1] = '\0';
-	}
+	/* Remove the trailing comma or terminate an empty buffer. */
+	buf[accum ? accum - 1 : 0] = '\0';
 	return 0;
 }
 
@@ -998,6 +1002,9 @@
 		return media_offer_read_av(channel->session, buf, len, AST_MEDIA_TYPE_AUDIO);
 	} else if (!strcmp(data, "video")) {
 		return media_offer_read_av(channel->session, buf, len, AST_MEDIA_TYPE_VIDEO);
+	} else {
+		/* Ensure that the buffer is empty */
+		buf[0] = '\0';
 	}
 
 	return 0;

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

Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-MessageType: merged
Gerrit-Change-Id: If6a0806806527678c8554b1dcb34fd7808aa95c9
Gerrit-Change-Number: 5849
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-commits/attachments/20170616/cce1dff3/attachment.html>