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

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review. ( https://gerrit.asterisk.org/5207 )

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(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/07/5207/1

diff --git a/main/pbx.c b/main/pbx.c
index 24a4a7f..bf66578 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -656,7 +656,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
@@ -6965,32 +6965,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;
 }
 
 /*!
@@ -7304,10 +7323,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;
@@ -7317,10 +7336,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/5207
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I555d97411140e47e0522684062d174fbe32aa84a
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>