[Asterisk-code-review] func strings: HASHKEY - negative array index can cause corru... (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/10666 )

Change subject: func_strings: HASHKEY - negative array index can cause corruption
......................................................................

func_strings: HASHKEY - negative array index can cause corruption

This patch makes it so only matching non-empty key names, and keys created by
the HASH function are eligible for inclusion in the comma separated string. It
also fixes a bug where it was possible to write to a negative index if the
result buffer was empty.

ASTERISK-28159
patches:
  ASTERISK-28159.diff submitted by Michael Walton (license 6502)

Change-Id: I6e57fe7307dfd856271753aed5ba64c59b511487
---
M funcs/func_strings.c
1 file changed, 41 insertions(+), 13 deletions(-)

Approvals:
  Corey Farrell: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Approved for Submit



diff --git a/funcs/func_strings.c b/funcs/func_strings.c
index cb25385..7afc40e 100644
--- a/funcs/func_strings.c
+++ b/funcs/func_strings.c
@@ -1089,10 +1089,33 @@
 	return 0;
 }
 
+static const char *get_key(const struct ast_str *prefix, const struct ast_var_t *var)
+{
+	const char *prefix_name = ast_str_buffer(prefix);
+	const char *var_name = ast_var_name(var);
+	int prefix_len;
+	int var_len;
+
+	if (ast_strlen_zero(var_name)) {
+		return NULL;
+	}
+
+	prefix_len = ast_str_strlen(prefix);
+	var_len = strlen(var_name);
+
+	/*
+	 * Make sure we only match on non-empty, hash function created keys. If valid
+	 * then return a pointer to the variable that's just after the prefix.
+	 */
+	return var_len > (prefix_len + 1) && var_name[var_len - 1] == '~' &&
+		strncmp(prefix_name, var_name, prefix_len) == 0 ? var_name + prefix_len : NULL;
+}
+
 static int hashkeys_read(struct ast_channel *chan, const char *cmd, char *data, char *buf, size_t len)
 {
 	struct ast_var_t *newvar;
 	struct ast_str *prefix = ast_str_alloca(80);
+	size_t buf_len;
 
 	if (!chan) {
 		ast_log(LOG_WARNING, "No channel was provided to %s function.\n", cmd);
@@ -1103,15 +1126,19 @@
 	memset(buf, 0, len);
 
 	AST_LIST_TRAVERSE(ast_channel_varshead(chan), newvar, entries) {
-		if (strncmp(ast_str_buffer(prefix), ast_var_name(newvar), ast_str_strlen(prefix)) == 0) {
-			/* Copy everything after the prefix */
-			strncat(buf, ast_var_name(newvar) + ast_str_strlen(prefix), len - strlen(buf) - 1);
-			/* Trim the trailing ~ */
+		const char *key = get_key(prefix, newvar);
+
+		if (key) {
+			strncat(buf, key, len - strlen(buf) - 1);
+			/* Replace the trailing ~ */
 			buf[strlen(buf) - 1] = ',';
 		}
 	}
 	/* Trim the trailing comma */
-	buf[strlen(buf) - 1] = '\0';
+	buf_len = strlen(buf);
+	if (buf_len) {
+		buf[buf_len - 1] = '\0';
+	}
 	return 0;
 }
 
@@ -1119,7 +1146,6 @@
 {
 	struct ast_var_t *newvar;
 	struct ast_str *prefix = ast_str_alloca(80);
-	char *tmp;
 
 	if (!chan) {
 		ast_log(LOG_WARNING, "No channel was provided to %s function.\n", cmd);
@@ -1129,17 +1155,19 @@
 	ast_str_set(&prefix, -1, HASH_PREFIX, data);
 
 	AST_LIST_TRAVERSE(ast_channel_varshead(chan), newvar, entries) {
-		if (strncmp(ast_str_buffer(prefix), ast_var_name(newvar), ast_str_strlen(prefix)) == 0) {
-			/* Copy everything after the prefix */
-			ast_str_append(buf, len, "%s", ast_var_name(newvar) + ast_str_strlen(prefix));
-			/* Trim the trailing ~ */
+		const char *key = get_key(prefix, newvar);
+
+		if (key) {
+			char *tmp;
+
+			ast_str_append(buf, len, "%s", key);
+			/* Replace the trailing ~ */
 			tmp = ast_str_buffer(*buf);
 			tmp[ast_str_strlen(*buf) - 1] = ',';
 		}
 	}
-	/* Trim the trailing comma */
-	tmp = ast_str_buffer(*buf);
-	tmp[ast_str_strlen(*buf) - 1] = '\0';
+
+	ast_str_truncate(*buf, -1);
 	return 0;
 }
 

-- 
To view, visit https://gerrit.asterisk.org/10666
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: I6e57fe7307dfd856271753aed5ba64c59b511487
Gerrit-Change-Number: 10666
Gerrit-PatchSet: 2
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.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/20181126/6b27ae94/attachment.html>