CallerID: Fix parsing of malformed callerid

This allows the callerid parsing function to handle malformed input
strings and strings containing escaped and unescaped double quotes.
This also adds a unittest to cover many of the cases where the parsing
algorithm previously failed.

Review: https://reviewboard.asterisk.org/r/3923/
Review: https://reviewboard.asterisk.org/r/3933/
........

Merged revisions 422112 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 422113 from http://svn.asterisk.org/svn/asterisk/branches/11
........

Merged revisions 422114 from http://svn.asterisk.org/svn/asterisk/branches/12


git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/13@422154 65c4cc65-6c06-0410-ace0-fbb531ad65f3

channels/chan_sip.c

@@ -12538,6 +12538,7 @@ static int add_rpid(struct sip_request *req, struct sip_pvt *p)
 {
 	struct ast_str *tmp = ast_str_alloca(256);
 	char tmp2[256];
+	char lid_name_buf[128];
 	char *lid_num;
 	char *lid_name;
 	int lid_pres;
@@ -12563,6 +12564,7 @@ static int add_rpid(struct sip_request *req, struct sip_pvt *p)
 	if (!lid_name) {
 		lid_name = lid_num;
 	}
+	ast_escape_quoted(lid_name, lid_name_buf, sizeof(lid_name_buf));
 	lid_pres = ast_party_id_presentation(&connected_id);
 
 	if (((lid_pres & AST_PRES_RESTRICTION) != AST_PRES_ALLOWED) &&
@@ -12586,7 +12588,7 @@ static int add_rpid(struct sip_request *req, struct sip_pvt *p)
 		if (ast_test_flag(&p->flags[1], SIP_PAGE2_TRUST_ID_OUTBOUND) != SIP_PAGE2_TRUST_ID_OUTBOUND_LEGACY) {
 			/* trust_id_outbound = yes - Always give full information even if it's private, but append a privacy header
 			 * When private data is included */
-			ast_str_set(&tmp, -1, "\"%s\" <sip:%s@%s>", lid_name, lid_num, fromdomain);
+			ast_str_set(&tmp, -1, "\"%s\" <sip:%s@%s>", lid_name_buf, lid_num, fromdomain);
 			if ((lid_pres & AST_PRES_RESTRICTION) != AST_PRES_ALLOWED) {
 				add_header(req, "Privacy", "id");
 			}
@@ -12594,14 +12596,14 @@ static int add_rpid(struct sip_request *req, struct sip_pvt *p)
 			/* trust_id_outbound = legacy - behave in a non RFC-3325 compliant manner and send anonymized data when
 			 * when handling private data. */
 			if ((lid_pres & AST_PRES_RESTRICTION) == AST_PRES_ALLOWED) {
-				ast_str_set(&tmp, -1, "\"%s\" <sip:%s@%s>", lid_name, lid_num, fromdomain);
+				ast_str_set(&tmp, -1, "\"%s\" <sip:%s@%s>", lid_name_buf, lid_num, fromdomain);
 			} else {
 				ast_str_set(&tmp, -1, "%s", anonymous_string);
 			}
 		}
 		add_header(req, "P-Asserted-Identity", ast_str_buffer(tmp));
 	} else {
-		ast_str_set(&tmp, -1, "\"%s\" <sip:%s@%s>;party=%s", lid_name, lid_num, fromdomain, p->outgoing_call ? "calling" : "called");
+		ast_str_set(&tmp, -1, "\"%s\" <sip:%s@%s>;party=%s", lid_name_buf, lid_num, fromdomain, p->outgoing_call ? "calling" : "called");
 
 		switch (lid_pres) {
 		case AST_PRES_ALLOWED_USER_NUMBER_NOT_SCREENED:
@@ -18857,7 +18859,10 @@ static void receive_message(struct sip_pvt *p, struct sip_request *req, struct a
 	from = (char *) get_calleridname(from, from_name, sizeof(from_name));
 	from = get_in_brackets(from);
 	if (from_name[0]) {
-		res |= ast_msg_set_from(msg, "\"%s\" <%s>", from_name, from);
+		char from_buf[128];
+
+		ast_escape_quoted(from_name, from_buf, sizeof(from_buf));
+		res |= ast_msg_set_from(msg, "\"%s\" <%s>", from_buf, from);
 	} else {
 		res |= ast_msg_set_from(msg, "<%s>", from);
 	}

include/asterisk/utils.h

@@ -318,6 +318,15 @@ int ast_xml_escape(const char *string, char *outbuf, size_t buflen);
  */
 char *ast_escape_quoted(const char *string, char *outbuf, int buflen);
 
+/*!
+ * \brief Unescape quotes in a string
+ *
+ * \param quote_str The string with quotes to be unescaped
+ *
+ * \note This function mutates the passed-in string.
+ */
+void ast_unescape_quoted(char *quote_str);
+
 static force_inline void ast_slinear_saturated_add(short *input, short *value)
 {
 	int res;

main/callerid.c

@@ -1009,50 +1009,39 @@ int ast_is_shrinkable_phonenumber(const char *exten)
 
 int ast_callerid_parse(char *instr, char **name, char **location)
 {
-	char *ns, *ne, *ls, *le;
+	char *ls, *le, *name_start;
 
-	/* Try "name" <location> format or name <location> format */
-	if ((ls = strrchr(instr, '<')) && (le = strrchr(ls, '>'))) {
-		*ls = *le = '\0';	/* location found, trim off the brackets */
+	/* Handle surrounding quotes */
+	instr = ast_strip_quoted(instr, "\"", "\"");
+
+	/* Try "name" <location> format or name <location> format or with a missing > */
+	if ((ls = strrchr(instr, '<'))) {
+		if ((le = strrchr(ls, '>'))) {
+			*le = '\0';	/* location found, trim off the brackets */
+		}
+		*ls = '\0';
 		*location = ls + 1;	/* and this is the result */
-		if ((ns = strchr(instr, '"')) && (ne = strchr(ns + 1, '"'))) {
-			*ns = *ne = '\0';	/* trim off the quotes */
-			*name = ns + 1;		/* and this is the name */
-		} else if (ns) {
-			/* An opening quote was found but no closing quote was. The closing
-			 * quote may actually be after the end of the bracketed number
-			 */
-			if (strchr(le + 1, '\"')) {
-				*ns = '\0';
-				*name = ns + 1;
-				ast_trim_blanks(*name);
-			} else {
-				*name = NULL;
-			}
-		} else { /* no quotes, trim off leading and trailing spaces */
-			*name = ast_skip_blanks(instr);
-			ast_trim_blanks(*name);
-		}
+
+		name_start = ast_strip_quoted(instr, "\"", "\"");
 	} else {	/* no valid brackets */
 		char tmp[256];
 
 		ast_copy_string(tmp, instr, sizeof(tmp));
 		ast_shrink_phone_number(tmp);
 		if (ast_isphonenumber(tmp)) {	/* Assume it's just a location */
-			*name = NULL;
+			name_start = NULL;
 			strcpy(instr, tmp); /* safe, because tmp will always be the same size or smaller than instr */
 			*location = instr;
 		} else { /* Assume it's just a name. */
 			*location = NULL;
-			if ((ns = strchr(instr, '"')) && (ne = strchr(ns + 1, '"'))) {
-				*ns = *ne = '\0';	/* trim off the quotes */
-				*name = ns + 1;		/* and this is the name */
-			} else { /* no quotes, trim off leading and trailing spaces */
-				*name = ast_skip_blanks(instr);
-				ast_trim_blanks(*name);
+			name_start = ast_strip_quoted(instr, "\"", "\"");
 		}
 	}
+
+	if (name_start) {
+		ast_unescape_quoted(name_start);
 	}
+	*name = name_start;
 	return 0;
 }
 
@@ -1079,14 +1068,18 @@ char *ast_callerid_merge(char *buf, int bufsiz, const char *name, const char *nu
 {
 	if (!unknown)
 		unknown = "<unknown>";
-	if (name && num)
-		snprintf(buf, bufsiz, "\"%s\" <%s>", name, num);
-	else if (name)
+	if (name && num) {
+		char name_buf[128];
+
+		ast_escape_quoted(name, name_buf, sizeof(name_buf));
+		snprintf(buf, bufsiz, "\"%s\" <%s>", name_buf, num);
+	} else if (name) {
 		ast_copy_string(buf, name, bufsiz);
-	else if (num)
+	} else if (num) {
 		ast_copy_string(buf, num, bufsiz);
-	else
+	} else {
 		ast_copy_string(buf, unknown, bufsiz);
+	}
 	return buf;
 }
 

main/utils.c

@@ -483,6 +483,29 @@ char *ast_escape_quoted(const char *string, char *outbuf, int buflen)
 
 	return outbuf;
 }
+
+void ast_unescape_quoted(char *quote_str)
+{
+	int esc_pos;
+	int unesc_pos;
+	int quote_str_len = strlen(quote_str);
+
+	for (esc_pos = 0, unesc_pos = 0;
+		esc_pos < quote_str_len;
+		esc_pos++, unesc_pos++) {
+		if (quote_str[esc_pos] == '\\') {
+			/* at least one more char and current is \\ */
+			esc_pos++;
+			if (esc_pos >= quote_str_len) {
+				break;
+			}
+		}
+
+		quote_str[unesc_pos] = quote_str[esc_pos];
+	}
+	quote_str[unesc_pos] = '\0';
+}
+
 int ast_xml_escape(const char *string, char * const outbuf, const size_t buflen)
 {
 	char *dst = outbuf;

res/res_pjsip_caller_id.c

@@ -404,7 +404,11 @@ static void modify_id_header(pj_pool_t *pool, pjsip_fromto_hdr *id_hdr, const st
 	id_uri = pjsip_uri_get_uri(id_name_addr->uri);
 
 	if (id->name.valid) {
-		pj_strdup2(pool, &id_name_addr->display, id->name.str);
+		int name_buf_len = strlen(id->name.str) * 2 + 1;
+		char *name_buf = ast_alloca(name_buf_len);
+
+		ast_escape_quoted(id->name.str, name_buf, name_buf_len);
+		pj_strdup2(pool, &id_name_addr->display, name_buf);
 	}
 
 	if (id->number.valid) {
@@ -438,7 +442,11 @@ static pjsip_fromto_hdr *create_new_id_hdr(const pj_str_t *hdr_name, pjsip_tx_da
 	id_uri = pjsip_uri_get_uri(id_name_addr->uri);
 
 	if (id->name.valid) {
-		pj_strdup2(tdata->pool, &id_name_addr->display, id->name.str);
+		int name_buf_len = strlen(id->name.str) * 2 + 1;
+		char *name_buf = ast_alloca(name_buf_len);
+
+		ast_escape_quoted(id->name.str, name_buf, name_buf_len);
+		pj_strdup2(tdata->pool, &id_name_addr->display, name_buf);
 	}
 
 	pj_strdup2(tdata->pool, &id_uri->user, id->number.str);

tests/test_callerid.c

@@ -0,0 +1,165 @@
+/*
+ * Asterisk -- An open source telephony toolkit.
+ *
+ * Copyright (C) 2014, Kinsey Moore
+ *
+ * Kinsey Moore <kmoore@digium.com>
+ *
+ * See http://www.asterisk.org for more information about
+ * the Asterisk project. Please do not directly contact
+ * any of the maintainers of this project for assistance;
+ * the project provides a web site, mailing lists and IRC
+ * channels for your use.
+ *
+ * This program is free software, distributed under the terms of
+ * the GNU General Public License Version 2. See the LICENSE file
+ * at the top of the source tree.
+ */
+
+/*!
+ * \file
+ * \brief Callerid Tests
+ *
+ * \author\verbatim Kinsey Moore <kmoore@digium.com> \endverbatim
+ *
+ * This is an Asterisk test module for callerid functionality
+ * \ingroup tests
+ */
+
+/*** MODULEINFO
+	<depend>TEST_FRAMEWORK</depend>
+	<support_level>core</support_level>
+ ***/
+
+#include "asterisk.h"
+#include "asterisk/callerid.h"
+
+ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
+
+#include "asterisk/utils.h"
+#include "asterisk/module.h"
+#include "asterisk/test.h"
+
+struct cid_set {
+	char *cid;
+	char *name;
+	char *number;
+};
+
+AST_TEST_DEFINE(parse_nominal)
+{
+	static const struct cid_set cid_sets[] = {
+		{"\"name\" <number>", "name", "number"},
+		{"\"   name  \" <number>", "   name  ", "number"},
+		{"name <number>", "name", "number"},
+		{"         name     <number>", "name", "number"},
+		{"\"\" <number>", NULL, "number"},
+		{"<number>", NULL, "number"},
+		{"name", "name", NULL},
+		{"\"name\"", "name", NULL},
+		{"\"name\" <>", "name", NULL},
+		{"name <>", "name", NULL},
+		{"1234", NULL, "1234"},
+		{"\"na\\\"me\" <number>", "na\"me", "number"},
+	};
+	char *name;
+	char *number;
+	int i;
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "parse_nominal";
+		info->category = "/main/callerid/";
+		info->summary = "Callerid nominal parse unit test";
+		info->description =
+			"This tests parsing of nominal callerid strings.";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	for (i = 0; i < ARRAY_LEN(cid_sets); i++) {
+		RAII_VAR(char *, callerid, ast_strdup(cid_sets[i].cid), ast_free);
+
+		ast_callerid_parse(callerid, &name, &number);
+		if (!cid_sets[i].name == !ast_strlen_zero(name) || (cid_sets[i].name && strcmp(name, cid_sets[i].name))) {
+			ast_test_status_update(test,
+				"Expected callerid name '%s' instead of '%s'\n",
+				cid_sets[i].name, name);
+			return AST_TEST_FAIL;
+		}
+
+		if (!cid_sets[i].number == !ast_strlen_zero(number) || (cid_sets[i].number && strcmp(number, cid_sets[i].number))) {
+			ast_test_status_update(test,
+				"Expected callerid number '%s' instead of '%s'\n",
+				cid_sets[i].number, number);
+			return AST_TEST_FAIL;
+		}
+	}
+
+	return AST_TEST_PASS;
+}
+
+AST_TEST_DEFINE(parse_off_nominal)
+{
+	static const struct cid_set cid_sets[] = {
+		{"na\\\"me <number>", "na\"me", "number"},
+		{"\"na\"me\" <number>", "na\"me", "number"},
+		{"na\"me <number>", "na\"me", "number"},
+		{"\"name <number>", "\"name", "number"},
+		{"name <number", "name", "number"},
+		{"\"name <number>\"", "name", "number"},
+	};
+	char *name;
+	char *number;
+	int i;
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "parse_off_nominal";
+		info->category = "/main/callerid/";
+		info->summary = "Callerid off-nominal parse unit test";
+		info->description =
+			"This tests parsing of off-nominal callerid strings.";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	for (i = 0; i < ARRAY_LEN(cid_sets); i++) {
+		RAII_VAR(char *, callerid, ast_strdup(cid_sets[i].cid), ast_free);
+
+		ast_callerid_parse(callerid, &name, &number);
+		if (!cid_sets[i].name == !ast_strlen_zero(name) || (cid_sets[i].name && strcmp(name, cid_sets[i].name))) {
+			ast_test_status_update(test,
+				"Expected callerid name '%s' instead of '%s'\n",
+				cid_sets[i].name, name);
+			return AST_TEST_FAIL;
+		}
+
+		if (!cid_sets[i].number == !ast_strlen_zero(number) || (cid_sets[i].number && strcmp(number, cid_sets[i].number))) {
+			ast_test_status_update(test,
+				"Expected callerid number '%s' instead of '%s'\n",
+				cid_sets[i].number, number);
+			return AST_TEST_FAIL;
+		}
+	}
+
+	return AST_TEST_PASS;
+}
+
+static int unload_module(void)
+{
+	AST_TEST_UNREGISTER(parse_nominal);
+	AST_TEST_UNREGISTER(parse_off_nominal);
+	return 0;
+}
+
+static int load_module(void)
+{
+	AST_TEST_REGISTER(parse_nominal);
+	AST_TEST_REGISTER(parse_off_nominal);
+	return AST_MODULE_LOAD_SUCCESS;
+}
+
+AST_MODULE_INFO_STANDARD(ASTERISK_GPL_KEY, "Callerid Parse Tests");

tests/test_utils.c

@@ -546,6 +546,100 @@ AST_TEST_DEFINE(crypt_test)
 }
 
 
+struct quote_set {
+	char *input;
+	char *output;
+};
+
+AST_TEST_DEFINE(quote_mutation)
+{
+	char escaped[64];
+	static const struct quote_set escape_sets[] = {
+		{"\"string\"", "\\\"string\\\""},
+		{"\"string", "\\\"string"},
+		{"string\"", "string\\\""},
+		{"string", "string"},
+		{"str\"ing", "str\\\"ing"},
+		{"\"", "\\\""},
+		{"\\\"", "\\\\\\\""},
+	};
+	int i;
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "quote_mutation";
+		info->category = "/main/utils/";
+		info->summary = "Test mutation of quotes in strings";
+		info->description =
+			"This tests escaping and unescaping of quotes in strings to "
+			"verify that the original string is recovered.";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	for (i = 0; i < ARRAY_LEN(escape_sets); i++) {
+		ast_escape_quoted(escape_sets[i].input, escaped, sizeof(escaped));
+
+		if (strcmp(escaped, escape_sets[i].output)) {
+			ast_test_status_update(test,
+				"Expected escaped string '%s' instead of '%s'\n",
+				escape_sets[i].output, escaped);
+			return AST_TEST_FAIL;
+		}
+
+		ast_unescape_quoted(escaped);
+		if (strcmp(escaped, escape_sets[i].input)) {
+			ast_test_status_update(test,
+				"Expected unescaped string '%s' instead of '%s'\n",
+				escape_sets[i].input, escaped);
+			return AST_TEST_FAIL;
+		}
+	}
+
+	return AST_TEST_PASS;
+}
+
+AST_TEST_DEFINE(quote_unescaping)
+{
+	static const struct quote_set escape_sets[] = {
+		{"\"string\"", "\"string\""},
+		{"\\\"string\"", "\"string\""},
+		{"\"string\\\"", "\"string\""},
+		{"str\\ing", "string"},
+		{"string\\", "string"},
+		{"\\string", "string"},
+	};
+	int i;
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "quote_unescaping";
+		info->category = "/main/utils/";
+		info->summary = "Test unescaping of off-nominal strings";
+		info->description =
+			"This tests unescaping of strings which contain a mix of "
+			"escaped and unescaped sequences.";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	for (i = 0; i < ARRAY_LEN(escape_sets); i++) {
+		RAII_VAR(char *, escaped, ast_strdup(escape_sets[i].input), ast_free);
+
+		ast_unescape_quoted(escaped);
+		if (strcmp(escaped, escape_sets[i].output)) {
+			ast_test_status_update(test,
+				"Expected unescaped string '%s' instead of '%s'\n",
+				escape_sets[i].output, escaped);
+			return AST_TEST_FAIL;
+		}
+	}
+
+	return AST_TEST_PASS;
+}
+
 static int unload_module(void)
 {
 	AST_TEST_UNREGISTER(uri_encode_decode_test);
@@ -558,6 +652,8 @@ static int unload_module(void)
 	AST_TEST_UNREGISTER(agi_loaded_test);
 	AST_TEST_UNREGISTER(safe_mkdir_test);
 	AST_TEST_UNREGISTER(crypt_test);
+	AST_TEST_UNREGISTER(quote_mutation);
+	AST_TEST_UNREGISTER(quote_unescaping);
 	return 0;
 }
 
@@ -573,6 +669,8 @@ static int load_module(void)
 	AST_TEST_REGISTER(agi_loaded_test);
 	AST_TEST_REGISTER(safe_mkdir_test);
 	AST_TEST_REGISTER(crypt_test);
+	AST_TEST_REGISTER(quote_mutation);
+	AST_TEST_REGISTER(quote_unescaping);
 	return AST_MODULE_LOAD_SUCCESS;
 }