From 54bf60c1186d9e3b92504167067cbcffd90fb842 Mon Sep 17 00:00:00 2001 From: Kinsey Moore Date: Wed, 27 Aug 2014 14:25:34 +0000 Subject: [PATCH] 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/ git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@422112 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- channels/chan_sip.c | 13 ++- include/asterisk/utils.h | 9 +++ main/callerid.c | 63 +++++++-------- main/utils.c | 22 ++++++ tests/test_callerid.c | 165 +++++++++++++++++++++++++++++++++++++++ tests/test_utils.c | 98 +++++++++++++++++++++++ 6 files changed, 331 insertions(+), 39 deletions(-) create mode 100644 tests/test_callerid.c diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 933e33a218..a8e7eb277b 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -11251,6 +11251,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; @@ -11278,6 +11279,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(&p->owner->connected.id); if (((lid_pres & AST_PRES_RESTRICTION) != AST_PRES_ALLOWED) && @@ -11301,7 +11303,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\" ", lid_name, lid_num, fromdomain); + ast_str_set(&tmp, -1, "\"%s\" ", lid_name_buf, lid_num, fromdomain); if ((lid_pres & AST_PRES_RESTRICTION) != AST_PRES_ALLOWED) { add_header(req, "Privacy", "id"); } @@ -11309,14 +11311,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\" ", lid_name, lid_num, fromdomain); + ast_str_set(&tmp, -1, "\"%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\" ;party=%s", lid_name, lid_num, fromdomain, p->outgoing_call ? "calling" : "called"); + ast_str_set(&tmp, -1, "\"%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: @@ -12550,8 +12552,11 @@ static void add_diversion_header(struct sip_request *req, struct sip_pvt *pvt) snprintf(header_text, sizeof(header_text), ";reason=%s", diverting_number, ast_sockaddr_stringify_host_remote(&pvt->ourip), reason); } else { + char diverting_name_buf[128]; + + ast_escape_quoted(diverting_name, diverting_name_buf, sizeof(diverting_name_buf)); snprintf(header_text, sizeof(header_text), "\"%s\" ;reason=%s", - diverting_name, diverting_number, + diverting_name_buf, diverting_number, ast_sockaddr_stringify_host_remote(&pvt->ourip), reason); } diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h index a6e3a2a7ee..14ec6d7003 100644 --- a/include/asterisk/utils.h +++ b/include/asterisk/utils.h @@ -300,6 +300,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; diff --git a/main/callerid.c b/main/callerid.c index cb6e588573..3b19932736 100644 --- a/main/callerid.c +++ b/main/callerid.c @@ -1008,50 +1008,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" format or name format */ - if ((ls = strrchr(instr, '<')) && (le = strrchr(ls, '>'))) { - *ls = *le = '\0'; /* location found, trim off the brackets */ - *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); + /* Handle surrounding quotes */ + instr = ast_strip_quoted(instr, "\"", "\""); + + /* Try "name" format or name 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 */ + + 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; } @@ -1078,14 +1067,18 @@ char *ast_callerid_merge(char *buf, int bufsiz, const char *name, const char *nu { if (!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; } diff --git a/main/utils.c b/main/utils.c index 1bb6e76a46..78c28a6c02 100644 --- a/main/utils.c +++ b/main/utils.c @@ -458,6 +458,28 @@ 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'; +} + /*! \brief ast_uri_decode: Decode SIP URI, URN, URL (overwrite the string) */ void ast_uri_decode(char *s) { diff --git a/tests/test_callerid.c b/tests/test_callerid.c new file mode 100644 index 0000000000..43b760d898 --- /dev/null +++ b/tests/test_callerid.c @@ -0,0 +1,165 @@ +/* + * Asterisk -- An open source telephony toolkit. + * + * Copyright (C) 2014, Kinsey Moore + * + * Kinsey Moore + * + * 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 \endverbatim + * + * This is an Asterisk test module for callerid functionality + * \ingroup tests + */ + +/*** MODULEINFO + TEST_FRAMEWORK + core + ***/ + +#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\" ", "name", "number"}, + {"\" name \" ", " name ", "number"}, + {"name ", "name", "number"}, + {" name ", "name", "number"}, + {"\"\" ", NULL, "number"}, + {"", NULL, "number"}, + {"name", "name", NULL}, + {"\"name\"", "name", NULL}, + {"\"name\" <>", "name", NULL}, + {"name <>", "name", NULL}, + {"1234", NULL, "1234"}, + {"\"na\\\"me\" ", "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 ", "na\"me", "number"}, + {"\"na\"me\" ", "na\"me", "number"}, + {"na\"me ", "na\"me", "number"}, + {"\"name ", "\"name", "number"}, + {"name \"", "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"); diff --git a/tests/test_utils.c b/tests/test_utils.c index 5d03a480be..c12002de59 100644 --- a/tests/test_utils.c +++ b/tests/test_utils.c @@ -394,6 +394,100 @@ AST_TEST_DEFINE(agi_loaded_test) return res; } +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); @@ -404,6 +498,8 @@ static int unload_module(void) AST_TEST_UNREGISTER(crypto_loaded_test); AST_TEST_UNREGISTER(adsi_loaded_test); AST_TEST_UNREGISTER(agi_loaded_test); + AST_TEST_UNREGISTER(quote_mutation); + AST_TEST_UNREGISTER(quote_unescaping); return 0; } @@ -417,6 +513,8 @@ static int load_module(void) AST_TEST_REGISTER(crypto_loaded_test); AST_TEST_REGISTER(adsi_loaded_test); AST_TEST_REGISTER(agi_loaded_test); + AST_TEST_REGISTER(quote_mutation); + AST_TEST_REGISTER(quote_unescaping); return AST_MODULE_LOAD_SUCCESS; } -- 2.47.2