XML: fix off-by-one in namespace prefix list parsing

njs_xml_parse_ns_list() allocated exactly size pointers, but the
terminating NULL was stored at the index equal to the number of parsed
tokens.  When the token count matched the current capacity, the
terminator was written one pointer past the allocation.

The growing loop is replaced with a single allocation holding the array
of pointers followed by the writable copy of the list, the number of
tokens is counted beforehand.

The list is split on any XML whitespace character, empty tokens are not
produced.

Also, fixed potential in place modification of QuickJS string value.

Reported by Vladimir Vulnerability Research Tech Lead @ Cyera, evilgensec

diff --git a/external/njs_xml_module.c b/external/njs_xml_module.c
index 4736c3ac1f696ccfda486447dc4fb106915e6517..f6687ad89ce86943020d5b1437065ff56b198317 100644 (file)
--- a/external/njs_xml_module.c
+++ b/external/njs_xml_module.c
@@ -1663,60 +1663,70 @@ njs_xml_c14n_visibility_cb(void *user_data, xmlNode *node, xmlNode *parent)
 }
 
 
+/* the C14N prefix list is a whitespace delimited list */
+#define njs_xml_ns_sep(c)                                                     \
+    ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r')
+
+
 static u_char **
 njs_xml_parse_ns_list(njs_vm_t *vm, njs_str_t *src)
 {
-    u_char    *p, **buf, **n, **out;
-    size_t  size, idx;
+    size_t  n;
+    u_char  *p, *end, *dst, **buf, **out;
+
+    /*
+     * The pointers and the writable copy of the list are allocated as a single
+     * block, the copy follows the NULL terminated array of pointers to it.
+     * The tokens are counted exactly the way the list is split below.
+     */
+
+    n = 0;
+    end = src->start + src->length;
 
-    out = NULL;
+    for (p = src->start; p < end && *p != '\0'; /* void */) {
+        if (njs_xml_ns_sep(*p)) {
+            p++;
+            continue;
+        }
+
+        n++;
+
+        while (p < end && *p != '\0' && !njs_xml_ns_sep(*p)) {
+            p++;
+        }
+    }
 
-    p =  njs_mp_alloc(njs_vm_memory_pool(vm), src->length + 1);
-    if (njs_slow_path(p == NULL)) {
+    if (njs_slow_path(n >= (SIZE_MAX - src->length - 1) / sizeof(u_char *))) {
         njs_vm_memory_error(vm);
         return NULL;
     }
 
-    memcpy(p, src->start, src->length);
-    p[src->length] = '\0';
-
-    size = 8;
-
-    buf = njs_mp_alloc(njs_vm_memory_pool(vm), size * sizeof(char *));
+    buf = njs_mp_alloc(njs_vm_memory_pool(vm),
+                       (n + 1) * sizeof(u_char *) + src->length + 1);
     if (njs_slow_path(buf == NULL)) {
         njs_vm_memory_error(vm);
         return NULL;
     }
 
-    out = buf;
+    dst = (u_char *) &buf[n + 1];
 
-    while (*p != '\0') {
-        idx = out - buf;
-
-        if (idx >= size) {
-            size *= 2;
-
-            n = njs_mp_alloc(njs_vm_memory_pool(vm), size * sizeof(char *));
-            if (njs_slow_path(buf == NULL)) {
-                njs_vm_memory_error(vm);
-                return NULL;
-            }
+    memcpy(dst, src->start, src->length);
+    dst[src->length] = '\0';
 
-            memcpy(n, buf, size * sizeof(char *) / 2);
-            buf = n;
+    out = buf;
+    p = dst;
 
-            out = &buf[idx];
+    while (*p != '\0') {
+        if (njs_xml_ns_sep(*p)) {
+            *p++ = '\0';
+            continue;
         }
 
         *out++ = p;
 
-        while (*p != ' ' && *p != '\0') {
+        while (*p != '\0' && !njs_xml_ns_sep(*p)) {
             p++;
         }
-
-        if (*p == ' ') {
-            *p++ = '\0';
-        }
     }
 
     *out = NULL;

diff --git a/external/qjs_xml_module.c b/external/qjs_xml_module.c
index ebb87657ce96183a1eb0d08296bd465d11a0630a..408b6b0475d7c29d200b2c3188d0d5b6542945b6 100644 (file)
--- a/external/qjs_xml_module.c
+++ b/external/qjs_xml_module.c
@@ -100,7 +100,7 @@ static int qjs_xml_attr_get_own_property_names(JSContext *cx,
     JSPropertyEnum **ptab, uint32_t *plen, JSValueConst obj);
 static void qjs_xml_attr_finalizer(JSRuntime *rt, JSValue val);
 
-static u_char **qjs_xml_parse_ns_list(JSContext *cx, u_char *src);
+static u_char **qjs_xml_parse_ns_list(JSContext *cx, njs_str_t *src);
 static int qjs_xml_c14n_visibility_cb(void *user_data, xmlNode *node,
     xmlNode *parent);
 static int qjs_xml_buf_write_cb(void *context, const char *buffer, int len);
@@ -245,11 +245,12 @@ qjs_xml_canonicalization(JSContext *cx, JSValueConst this_val, int argc,
     JSValueConst *argv, int magic)
 {
     int              comments;
-    u_char           **prefix_list, *pref;
+    u_char           **prefix_list;
     xmlDoc           *doc;
     xmlNode          *node;
     ssize_t          size;
     JSValue          excluding, prefixes, ret;
+    njs_str_t        pref;
     njs_chb_t        chain;
     qjs_xml_node_t   *nd;
     qjs_xml_nset_t   *nset, *children;
@@ -309,13 +310,15 @@ qjs_xml_canonicalization(JSContext *cx, JSValueConst this_val, int argc,
             goto fail;
         }
 
-        pref = (u_char *) JS_ToCString(cx, prefixes);
-        if (pref == NULL) {
-            JS_ThrowOutOfMemory(cx);
+        pref.start = (u_char *) JS_ToCStringLen(cx, &pref.length, prefixes);
+        if (pref.start == NULL) {
             goto fail;
         }
 
-        prefix_list = qjs_xml_parse_ns_list(cx, pref);
+        prefix_list = qjs_xml_parse_ns_list(cx, &pref);
+
+        JS_FreeCString(cx, (char *) pref.start);
+
         if (prefix_list == NULL) {
             goto fail;
         }
@@ -1720,47 +1723,68 @@ qjs_xml_attr_finalizer(JSRuntime *rt, JSValue val)
 }
 
 
+/* the C14N prefix list is a whitespace delimited list */
+#define qjs_xml_ns_sep(c)                                                     \
+    ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r')
+
+
 static u_char **
-qjs_xml_parse_ns_list(JSContext *cx, u_char *src)
+qjs_xml_parse_ns_list(JSContext *cx, njs_str_t *src)
 {
-    u_char    *p, **buf, **out;
-    size_t  size, idx;
+    size_t  n;
+    u_char  *p, *end, *dst, **buf, **out;
 
-    size = 8;
-    p = src;
+    /*
+     * The pointers and the writable copy of the list are allocated as a single
+     * block, the copy follows the NULL terminated array of pointers to it.
+     * The tokens are counted exactly the way the list is split below.
+     */
 
-    buf = js_mallocz(cx, size * sizeof(char *));
-    if (buf == NULL) {
+    n = 0;
+    end = src->start + src->length;
+
+    for (p = src->start; p < end && *p != '\0'; /* void */) {
+        if (qjs_xml_ns_sep(*p)) {
+            p++;
+            continue;
+        }
+
+        n++;
+
+        while (p < end && *p != '\0' && !qjs_xml_ns_sep(*p)) {
+            p++;
+        }
+    }
+
+    if (n >= (SIZE_MAX - src->length - 1) / sizeof(u_char *)) {
         JS_ThrowOutOfMemory(cx);
         return NULL;
     }
 
-    out = buf;
+    buf = js_malloc(cx, (n + 1) * sizeof(u_char *) + src->length + 1);
+    if (buf == NULL) {
+        return NULL;
+    }
 
-    while (*p != '\0') {
-        idx = out - buf;
+    dst = (u_char *) &buf[n + 1];
 
-        if (idx >= size) {
-            size *= 2;
+    memcpy(dst, src->start, src->length);
+    dst[src->length] = '\0';
 
-            buf = js_realloc(cx, buf, size * sizeof(char *));
-            if (buf == NULL) {
-                JS_ThrowOutOfMemory(cx);
-                return NULL;
-            }
+    out = buf;
+    p = dst;
 
-            out = &buf[idx];
+    while (*p != '\0') {
+        if (qjs_xml_ns_sep(*p)) {
+            *p++ = '\0';
+            continue;
         }
 
         *out++ = p;
 
-        while (*p != ' ' && *p != '\0') {
+        while (*p != '\0' && !qjs_xml_ns_sep(*p)) {
             p++;
         }
-
-        if (*p == ' ') {
-            *p++ = '\0';
-        }
     }
 
     *out = NULL;

diff --git a/test/xml/xml.t.mjs b/test/xml/xml.t.mjs
index 8590bddc97a43ec1a45cde4fdde48411860b3aeb..75ce9c4ccab35687dd0b5cb3febdf655b6c3215b 100644 (file)
--- a/test/xml/xml.t.mjs
+++ b/test/xml/xml.t.mjs
@@ -127,6 +127,34 @@ let c14n_tsuite = {
           expected: `<n1:elem1 xmlns:n1="http://b">foo</n1:elem1>` },
         { call: (doc) => xml.exclusiveC14n(doc.pdu.elem1, null, false, 'a b c d e f g h i j'),
           expected: `<n1:elem1 xmlns:n1="http://b">foo</n1:elem1>` },
+        { call: (doc) => xml.exclusiveC14n(doc.pdu.elem1, null, false,
+                                           'a '.repeat(7) + 'n0'),
+          expected: `<n1:elem1 xmlns:n0="http://a" xmlns:n1="http://b">foo</n1:elem1>` },
+        { call: (doc) => xml.exclusiveC14n(doc.pdu.elem1, null, false,
+                                           'a '.repeat(15) + 'n0'),
+          expected: `<n1:elem1 xmlns:n0="http://a" xmlns:n1="http://b">foo</n1:elem1>` },
+        { call: (doc) => xml.exclusiveC14n(doc.pdu.elem1, null, false,
+                                           'a '.repeat(127) + 'n0'),
+          expected: `<n1:elem1 xmlns:n0="http://a" xmlns:n1="http://b">foo</n1:elem1>` },
+        { call: (doc) => xml.exclusiveC14n(doc.pdu.elem1, null, false,
+                                           '  n0   a  '),
+          expected: `<n1:elem1 xmlns:n0="http://a" xmlns:n1="http://b">foo</n1:elem1>` },
+        { call: (doc) => xml.exclusiveC14n(doc.pdu.elem1, null, false,
+                                           'a\tn0\r\nb'),
+          expected: `<n1:elem1 xmlns:n0="http://a" xmlns:n1="http://b">foo</n1:elem1>` },
+        { doc: `<r xmlns="http://d" xmlns:p="http://p"><p:b>x</p:b></r>`,
+          call: (doc) => xml.exclusiveC14n(doc.r.b, null, false, '#default'),
+          expected: `<p:b xmlns="http://d" xmlns:p="http://p">x</p:b>` },
+        { doc: `<r xmlns="http://d" xmlns:p="http://p"><p:b>x</p:b></r>`,
+          call: (doc) => xml.exclusiveC14n(doc.r.b, null, false, ' a  b '),
+          expected: `<p:b xmlns:p="http://p">x</p:b>` },
+        { call: (doc) => {
+              let prefixes = 'n0 n1';
+              xml.exclusiveC14n(doc.pdu.elem1, null, false, prefixes);
+              return prefixes;
+          },
+          buffer: false,
+          expected: 'n0 n1' },
         { doc: `<note><to a="foo" b="bar">Tove</to><from>Jani</from></note>`,
           call: (doc) => xml.c14n(doc.note),
           expected: `<note><to a="foo" b="bar">Tove</to><from>Jani</from></note>` },