res_pjsip: Ensure sanitized XML is NULL terminated.

The ast_sip_sanitize_xml function is used to sanitize
a string for placement into XML. This is done by examining
an input string and then appending values to an output
buffer. The function used by its implementation, strncat,
has specific behavior that was not taken into account.
If the size of the input string exceeded the available
output buffer size it was possible for the sanitization
function to write past the output buffer itself causing
a crash. The crash would either occur because it was
writing into memory it shouldn't be or because the resulting
string was not NULL terminated.

This change keeps count of how much remaining space is
available in the output buffer for text and only allows
strncat to use that amount.

Since this was exposed by the res_pjsip_pidf_digium_body_supplement
module attempting to send a large message the maximum allowed
message size has also been increased in it.

A unit test has also been added which confirms that the
ast_sip_sanitize_xml function is providing NULL terminated
output even when the input length exceeds the output
buffer size.

ASTERISK-25304 #close

Change-Id: I743dd9809d3e13d722df1b0509dfe34621398302

diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 0647e3884d77dcd1bfd3811147bf7d3832d5c49e..8621658812578b67f735f21e10961635e60cff18 100644 (file)
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -37,6 +37,9 @@
 #include "asterisk/sorcery.h"
 #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>
@@ -3278,6 +3281,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
@@ -3436,6 +3490,8 @@ static int load_module(void)
        ast_cli_register_multiple(cli_commands, ARRAY_LEN(cli_commands));
 
        ast_module_ref(ast_module_info->self);
+       AST_TEST_REGISTER(xml_sanitization_end_null);
+       AST_TEST_REGISTER(xml_sanitization_exceeds_buffer);
 
        return AST_MODULE_LOAD_SUCCESS;
 }
@@ -3456,6 +3512,8 @@ static int reload_module(void)
 
 static int unload_module(void)
 {
+       AST_TEST_UNREGISTER(xml_sanitization_end_null);
+       AST_TEST_UNREGISTER(xml_sanitization_exceeds_buffer);
        ast_cli_unregister_multiple(cli_commands, ARRAY_LEN(cli_commands));
        /* This will never get called as this module can't be unloaded */
        return 0;

diff --git a/res/res_pjsip/presence_xml.c b/res/res_pjsip/presence_xml.c
index 2fe6bdc00a6b9ee97d13ee020b61287d90da1090..267af547d6a1b6c26f3a114f0d4426ca9b4e98b5 100644 (file)
--- a/res/res_pjsip/presence_xml.c
+++ b/res/res_pjsip/presence_xml.c
@@ -40,45 +40,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 86e673afa1cd2e80db368413d2c6a060c1140016..4b1a781811f0f24d5e02fdca4bdfae27d1ef53f6 100644 (file)
--- 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")) {