[Asterisk-code-review] pbx.c: Fix crash from malformed exten pattern. (asterisk[13])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5206 )

Change subject: pbx.c: Fix crash from malformed exten pattern.
......................................................................


pbx.c: Fix crash from malformed exten pattern.

Forgetting to indicate an exten is a pattern can cause a crash if the
"pattern" has a character set range.  e.g., "9999[3-5]" The crash is due
to a buffer overwrite because the '-' exten eye-candy wasn't removed as
expected and overran the allocated space.

The buffer overwrite is fixed two ways in this patch.

1) Fix ext_strncpy() to distinguish between pattern and non-pattern
extens.  Now '-' characters are removed when they are eye-candy and not
when they are part of a pattern character set.  Since the function is
private to pbx.c, the return value now returns the number of bytes written
to the destination buffer instead of the strlen() of the final buffer so
the callers that care don't need to add one.

2) Fix callers to ext_strncpy() to supply the correct available buffer
size of the destination buffer.

ASTERISK-26668

Change-Id: I555d97411140e47e0522684062d174fbe32aa84a
---
M main/pbx.c
1 file changed, 37 insertions(+), 18 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, approved
  George Joseph: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified



diff --git a/main/pbx.c b/main/pbx.c
index d648084..df99940 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -616,7 +616,7 @@
 static struct ast_context *find_context_locked(const char *context);
 static struct ast_context *find_context(const char *context);
 static void get_device_state_causing_channels(struct ao2_container *c);
-static int ext_strncpy(char *dst, const char *src, int len, int nofluff);
+static unsigned int ext_strncpy(char *dst, const char *src, size_t dst_size, int nofluff);
 
 /*!
  * \internal
@@ -6907,32 +6907,51 @@
 	return res;
 }
 
-/*! \brief copy a string skipping whitespace and dashes */
-static int ext_strncpy(char *dst, const char *src, int len, int nofluff)
+/*!
+ * \internal
+ * \brief Copy a string skipping whitespace and optionally dashes.
+ *
+ * \param dst Destination buffer to copy src string.
+ * \param src Null terminated string to copy.
+ * \param dst_size Number of bytes in the dst buffer.
+ * \param nofluf Nonzero if '-' chars are not copied.
+ *
+ * \return Number of bytes written to dst including null terminator.
+ */
+static unsigned int ext_strncpy(char *dst, const char *src, size_t dst_size, int nofluff)
 {
-	int count = 0;
-	int insquares = 0;
+	unsigned int count;
+	unsigned int insquares;
+	unsigned int is_pattern;
 
-	while (*src && (count < len - 1)) {
+	if (!dst_size--) {
+		/* There really is no dst buffer */
+		return 0;
+	}
+
+	count = 0;
+	insquares = 0;
+	is_pattern = *src == '_';
+	while (*src && count < dst_size) {
 		if (*src == '[') {
-			insquares = 1;
+			if (is_pattern) {
+				insquares = 1;
+			}
 		} else if (*src == ']') {
 			insquares = 0;
 		} else if (*src == ' ' && !insquares) {
-			src++;
+			++src;
 			continue;
 		} else if (*src == '-' && !insquares && nofluff) {
-			src++;
+			++src;
 			continue;
 		}
-		*dst = *src;
-		dst++;
-		src++;
-		count++;
+		*dst++ = *src++;
+		++count;
 	}
 	*dst = '\0';
 
-	return count;
+	return count + 1;
 }
 
 /*!
@@ -7246,10 +7265,10 @@
 		p += strlen(label) + 1;
 	}
 	tmp->name = p;
-	p += ext_strncpy(p, extension, strlen(extension) + 1, 0) + 1;
+	p += ext_strncpy(p, extension, strlen(extension) + 1, 0);
 	if (exten_fluff) {
 		tmp->exten = p;
-		p += ext_strncpy(p, extension, strlen(extension) + 1, 1) + 1;
+		p += ext_strncpy(p, extension, strlen(extension) + 1 - exten_fluff, 1);
 	} else {
 		/* no fluff, we don't need a copy. */
 		tmp->exten = tmp->name;
@@ -7259,10 +7278,10 @@
 
 	/* Blank callerid and NULL callerid are two SEPARATE things.  Do NOT confuse the two!!! */
 	if (callerid) {
-		p += ext_strncpy(p, callerid, strlen(callerid) + 1, 0) + 1;
+		p += ext_strncpy(p, callerid, strlen(callerid) + 1, 0);
 		if (callerid_fluff) {
 			tmp->cidmatch = p;
-			p += ext_strncpy(p, callerid, strlen(callerid) + 1, 1) + 1;
+			p += ext_strncpy(p, callerid, strlen(callerid) + 1 - callerid_fluff, 1);
 		}
 		tmp->matchcid = AST_EXT_MATCHCID_ON;
 	} else {

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I555d97411140e47e0522684062d174fbe32aa84a
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>