Merge "res_pjsip: Ensure sanitized XML is NULL terminated." into 13

3 files changed, 80 insertions, 12 deletions

diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 43e3da6de..405ac6838 100644
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -40,6 +40,8 @@
 #include "asterisk/file.h"
 #include "asterisk/cli.h"
 #include "asterisk/res_pjsip_cli.h"
+#include "asterisk/test.h"
+#include "asterisk/res_pjsip_presence_xml.h"
 
 /*** MODULEINFO
 	<depend>pjproject</depend>
@@ -3701,6 +3703,57 @@ static void remove_request_headers(pjsip_endpoint *endpt)
 	}
 }
 
+AST_TEST_DEFINE(xml_sanitization_end_null)
+{
+	char sanitized[8];
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "xml_sanitization_end_null";
+		info->category = "/res/res_pjsip/";
+		info->summary = "Ensure XML sanitization works as expected with a long string";
+		info->description = "This test sanitizes a string which exceeds the output\n"
+			"buffer size. Once done the string is confirmed to be NULL terminated.";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	ast_sip_sanitize_xml("aaaaaaaaaaaa", sanitized, sizeof(sanitized));
+	if (sanitized[7] != '\0') {
+		ast_test_status_update(test, "Sanitized XML string is not null-terminated when it should be\n");
+		return AST_TEST_FAIL;
+	}
+
+	return AST_TEST_PASS;
+}
+
+AST_TEST_DEFINE(xml_sanitization_exceeds_buffer)
+{
+	char sanitized[8];
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "xml_sanitization_exceeds_buffer";
+		info->category = "/res/res_pjsip/";
+		info->summary = "Ensure XML sanitization does not exceed buffer when output won't fit";
+		info->description = "This test sanitizes a string which before sanitization would\n"
+			"fit within the output buffer. After sanitization, however, the string would\n"
+			"exceed the buffer. Once done the string is confirmed to be NULL terminated.";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	ast_sip_sanitize_xml("<><><>&", sanitized, sizeof(sanitized));
+	if (sanitized[7] != '\0') {
+		ast_test_status_update(test, "Sanitized XML string is not null-terminated when it should be\n");
+		return AST_TEST_FAIL;
+	}
+
+	return AST_TEST_PASS;
+}
+
 /*!
  * \internal
  * \brief Reload configuration within a PJSIP thread
@@ -3870,6 +3923,9 @@ static int load_module(void)
 	ast_res_pjsip_init_options_handling(0);
 	ast_cli_register_multiple(cli_commands, ARRAY_LEN(cli_commands));
 
+	AST_TEST_REGISTER(xml_sanitization_end_null);
+	AST_TEST_REGISTER(xml_sanitization_exceeds_buffer);
+
 	return AST_MODULE_LOAD_SUCCESS;
 }
 
@@ -3912,6 +3968,9 @@ static int unload_pjsip(void *data)
 
 static int unload_module(void)
 {
+	AST_TEST_UNREGISTER(xml_sanitization_end_null);
+	AST_TEST_UNREGISTER(xml_sanitization_exceeds_buffer);
+
 	/* The thread this is called from cannot call PJSIP/PJLIB functions,
 	 * so we have to push the work to the threadpool to handle
 	 */
diff --git a/res/res_pjsip/presence_xml.c b/res/res_pjsip/presence_xml.c
index 12bfa078c..b98ea0237 100644
--- a/res/res_pjsip/presence_xml.c
+++ b/res/res_pjsip/presence_xml.c
@@ -30,45 +30,54 @@ void ast_sip_sanitize_xml(const char *input, char *output, size_t len)
 {
 	char *copy = ast_strdupa(input);
 	char *break_point;
+	size_t remaining = len - 1;
 
 	output[0] = '\0';
 
-	while ((break_point = strpbrk(copy, "<>\"&'\n\r"))) {
+	while ((break_point = strpbrk(copy, "<>\"&'\n\r")) && remaining) {
 		char to_escape = *break_point;
 
 		*break_point = '\0';
-		strncat(output, copy, len);
+		strncat(output, copy, remaining);
+
+		/* The strncat function will write remaining+1 if the string length is
+		 * equal to or greater than the size provided to it. We take this into
+		 * account by subtracting 1, which ensures that the NULL byte is written
+		 * inside of the provided buffer.
+		 */
+		remaining = len - strlen(output) - 1;
 
 		switch (to_escape) {
 		case '<':
-			strncat(output, "&lt;", len);
+			strncat(output, "&lt;", remaining);
 			break;
 		case '>':
-			strncat(output, "&gt;", len);
+			strncat(output, "&gt;", remaining);
 			break;
 		case '"':
-			strncat(output, "&quot;", len);
+			strncat(output, "&quot;", remaining);
 			break;
 		case '&':
-			strncat(output, "&amp;", len);
+			strncat(output, "&amp;", remaining);
 			break;
 		case '\'':
-			strncat(output, "&apos;", len);
+			strncat(output, "&apos;", remaining);
 			break;
 		case '\r':
-			strncat(output, "&#13;", len);
+			strncat(output, "&#13;", remaining);
 			break;
 		case '\n':
-			strncat(output, "&#10;", len);
+			strncat(output, "&#10;", remaining);
 			break;
 		};
 
 		copy = break_point + 1;
+		remaining = len - strlen(output) - 1;
 	}
 
 	/* Be sure to copy everything after the final bracket */
-	if (*copy) {
-		strncat(output, copy, len);
+	if (*copy && remaining) {
+		strncat(output, copy, remaining);
 	}
 }
 
diff --git a/res/res_pjsip_pidf_digium_body_supplement.c b/res/res_pjsip_pidf_digium_body_supplement.c
index 86e673afa..4b1a78181 100644
--- a/res/res_pjsip_pidf_digium_body_supplement.c
+++ b/res/res_pjsip_pidf_digium_body_supplement.c
@@ -40,7 +40,7 @@ static int pidf_supplement_body(void *body, void *data)
 {
 	struct ast_sip_exten_state_data *state_data = data;
 	pj_xml_node *node;
-	char sanitized[256];
+	char sanitized[1024];
 
 	if (ast_strlen_zero(state_data->user_agent) ||
 	    !strstr(state_data->user_agent, "digium")) {