Querystring: fix decoding after invalid UTF-8

Previously, percent-decoding advanced past each encoded byte even when
the UTF-8 decoder left it unconsumed after reporting an invalid pending
sequence.  For example, "%D8%41%42" decoded to "\ufffdB" instead of
"\ufffdAB".  This affected querystring.parse() and unescape() in both
engines, and r.args in nginx.

Now njs_utf8_consume() reports whether the byte was consumed, so callers
can reread it before advancing the input, as the WHATWG UTF-8 decoder
requires.  Plus signs are also passed through the decoder as spaces,
so they flush invalid pending sequences without losing or reordering
the replacement character.

The issue was introduced in 50ea0dbc (0.4.3), copied into QuickJS
r.args in 201b1276 (0.8.6) and into its querystring module in f289dcb9
(0.8.10).

diff --git a/external/njs_query_string_module.c b/external/njs_query_string_module.c
index c5b4d58a92273dc0a0c30e556e2fd4fdfde64038..9426731744d8650042eedfc184bcf8cd592d1fcc 100644 (file)
--- a/external/njs_query_string_module.c
+++ b/external/njs_query_string_module.c
@@ -127,6 +127,7 @@ njs_query_string_decode(njs_vm_t *vm, njs_value_t *value, const u_char *start,
     u_char                *dst;
     uint32_t              cp;
     njs_int_t             ret;
+    njs_bool_t            consumed;
     njs_chb_t             chain;
     const u_char          *p, *end;
     njs_unicode_decode_t  ctx;
@@ -162,13 +163,18 @@ njs_query_string_decode(njs_vm_t *vm, njs_value_t *value, const u_char *start,
 
     while (p < end) {
         if (*p == '%' && end - p > 2 && hex[p[1]] >= 0 && hex[p[2]] >= 0) {
-            cp = njs_utf8_consume(&ctx, (hex[p[1]] << 4) | hex[p[2]]);
-            p += 3;
+            cp = njs_utf8_consume(&ctx, (hex[p[1]] << 4) | hex[p[2]],
+                                  &consumed);
+            if (consumed) {
+                p += 3;
+            }
 
         } else {
             if (*p == '+') {
-                cp = ' ';
-                p++;
+                cp = njs_utf8_consume(&ctx, ' ', &consumed);
+                if (consumed) {
+                    p++;
+                }
 
             } else {
                 cp = njs_utf8_decode(&ctx, &p, end);

diff --git a/external/qjs_query_string_module.c b/external/qjs_query_string_module.c
index 128a17cd547580f7831a9d2f484a65587da250e5..b3b429b20f44dc076f6db146c7aaef503206b9dc 100644 (file)
--- a/external/qjs_query_string_module.c
+++ b/external/qjs_query_string_module.c
@@ -172,6 +172,7 @@ qjs_query_string_decode(JSContext *cx, const u_char *start, size_t size)
 {
     u_char                *dst;
     uint32_t              cp;
+    njs_bool_t            consumed;
     njs_chb_t             chain;
     const u_char          *p, *end;
     njs_unicode_decode_t  ctx;
@@ -207,13 +208,18 @@ qjs_query_string_decode(JSContext *cx, const u_char *start, size_t size)
 
     while (p < end) {
         if (*p == '%' && end - p > 2 && hex[p[1]] >= 0 && hex[p[2]] >= 0) {
-            cp = njs_utf8_consume(&ctx, (hex[p[1]] << 4) | hex[p[2]]);
-            p += 3;
+            cp = njs_utf8_consume(&ctx, (hex[p[1]] << 4) | hex[p[2]],
+                                  &consumed);
+            if (consumed) {
+                p += 3;
+            }
 
         } else {
             if (*p == '+') {
-                cp = ' ';
-                p++;
+                cp = njs_utf8_consume(&ctx, ' ', &consumed);
+                if (consumed) {
+                    p++;
+                }
 
             } else {
                 cp = njs_utf8_decode(&ctx, &p, end);

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index 4f9ac9b2f33e3330a8dd31abe8c0a5945953eb8b..2b7109eddfa0de31219f43352ef9fd4789ba58c3 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -6120,6 +6120,7 @@ ngx_http_qjs_query_string_decode(njs_chb_t *chain, const u_char *start,
 {
     u_char                *dst;
     uint32_t               cp;
+    njs_bool_t             consumed;
     const u_char          *p, *end;
     njs_unicode_decode_t   ctx;
 
@@ -6153,13 +6154,18 @@ ngx_http_qjs_query_string_decode(njs_chb_t *chain, const u_char *start,
 
     while (p < end) {
         if (*p == '%' && end - p > 2 && hex[p[1]] >= 0 && hex[p[2]] >= 0) {
-            cp = njs_utf8_consume(&ctx, (hex[p[1]] << 4) | hex[p[2]]);
-            p += 3;
+            cp = njs_utf8_consume(&ctx, (hex[p[1]] << 4) | hex[p[2]],
+                                  &consumed);
+            if (consumed) {
+                p += 3;
+            }
 
         } else {
             if (*p == '+') {
-                cp = ' ';
-                p++;
+                cp = njs_utf8_consume(&ctx, ' ', &consumed);
+                if (consumed) {
+                    p++;
+                }
 
             } else {
                 cp = njs_utf8_decode(&ctx, &p, end);

diff --git a/nginx/t/js_args.t b/nginx/t/js_args.t
index bb5cf4ff5020282541ad25c27ea56ffdbe72d247..0adfddd888f95bffb3792401c38a988be4d8ce8d 100644 (file)
--- a/nginx/t/js_args.t
+++ b/nginx/t/js_args.t
@@ -94,7 +94,7 @@ $t->write_file('test.js', <<EOF);
 
 EOF
 
-$t->try_run('no njs')->plan(15);
+$t->try_run('no njs')->plan(20);
 
 ###############################################################################
 
@@ -145,6 +145,18 @@ is(get_json('/object?a=%6&b=%&c=%%&d=%zz'),
        '{"a":"%6","b":"%","c":"%%","d":"%zz"}',
        'values percent-encoded broken object');
 
+is(get_json('/object?%D8%41%42=%E2%82%41%42'),
+       qq/{"\x{fffd}AB":"\x{fffd}AB"}/,
+       'percent-encoded units after invalid UTF-8');
+is(get_json('/object?a=%D8+%41'), qq/{"a":"\x{fffd} A"}/,
+       'plus after invalid UTF-8');
+is(get_json('/object?a=%E2%82+'), qq/{"a":"\x{fffd} "}/,
+       'final plus after invalid UTF-8');
+is(get_json('/object?a=%D8%C2%A2'), qq/{"a":"\x{fffd}\x{a2}"}/,
+       'valid UTF-8 after invalid UTF-8');
+is(get_json('/object?a=%ED%A0%80'), qq/{"a":"\x{fffd}\x{fffd}\x{fffd}"}/,
+       'UTF-8 encoded surrogate');
+
 ###############################################################################
 
 sub has_version {

diff --git a/src/njs_utf8.h b/src/njs_utf8.h
index 5f2f81e27f6c8149c1f48af5161af5d411ccb770..7386b4b7be3439f37cf64d7d437e85cb0a5fa7b4 100644 (file)
--- a/src/njs_utf8.h
+++ b/src/njs_utf8.h
@@ -27,13 +27,19 @@ NJS_EXPORT njs_bool_t njs_utf8_is_valid(const u_char *p, size_t len);
 
 
 njs_inline uint32_t
-njs_utf8_consume(njs_unicode_decode_t *ctx, u_char byte)
+njs_utf8_consume(njs_unicode_decode_t *ctx, u_char byte, njs_bool_t *consumed)
 {
+    uint32_t      cp;
     const u_char  *p;
 
     p = &byte;
 
-    return njs_utf8_decode(ctx, &p, p + 1);
+    cp = njs_utf8_decode(ctx, &p, p + 1);
+
+    /* An error may leave the byte to be decoded again. */
+    *consumed = (p != &byte);
+
+    return cp;
 }
 
 

diff --git a/test/querystring.t.mjs b/test/querystring.t.mjs
index 8ea14050e8611a8159c57ab84c1e1b4bc2884230..d9ebc0a315830e79e999041a7f9ceb766a6903f5 100644 (file)
--- a/test/querystring.t.mjs
+++ b/test/querystring.t.mjs
@@ -117,6 +117,13 @@ let parse_tsuite = {
         { value: 'baz=%F', expected: { baz:'%F' } },
         { value: 'baz=%', expected: { baz:'%' } },
         { value: 'ba+z=f+uz', expected: { 'ba z':'f uz' } },
+        { value: '%D8%41%42=%E2%82%41%42',
+          expected: { '�AB':'�AB' } },
+        { value: 'baz=%D8+%41', expected: { baz:'� A' } },
+        { value: 'baz=%E2%82+', expected: { baz:'� ' } },
+        { value: 'baz=%D8%C2%A2', expected: { baz:'�¢' } },
+        { value: 'baz=%D8%F0%9F%92%A9', expected: { baz:'�💩' } },
+        { value: 'baz=%ED%A0%80', expected: { baz:'���' } },
         { value: 'X=' + 'α'.repeat(33), expected: { X:'α'.repeat(33) } },
         { value: 'X=' + 'α1'.repeat(33), expected: { X:'α1'.repeat(33) } },
 
@@ -214,6 +221,12 @@ let unescape_tsuite = {
         { value: '', expected: '' },
         { value: 'baz%3Dfuz', expected: 'baz=fuz' },
         { value: 'abc%CE%B1def', expected: 'abcαdef' },
+        { value: '%D8%41%42', expected: '�AB' },
+        { value: '%E2%41%42', expected: '�AB' },
+        { value: '%E2%82%41%42', expected: '�AB' },
+        { value: '%D8%C2%A2', expected: '�¢' },
+        { value: '%D8%F0%9F%92%A9', expected: '�💩' },
+        { value: '%ED%A0%80', expected: '���' },
 ]};
 
 run([