Buffer: fix type confusion in concat() with element getters

njs_buffer_concat() validated each list element as a typed array in the
length pass, then re-read the same elements with njs_value_property_i64()
in the copy pass and cast them with njs_typed_array() without rechecking.
For a non-fast array with an accessor element, a getter returning a typed
array during validation and a non-typed-array (or a detached buffer)
during the copy yielded a wild pointer and an out-of-bounds read.

Revalidate the type and detached state in the copy pass, matching the
length pass and the QuickJS implementation.

diff --git a/src/njs_buffer.c b/src/njs_buffer.c
index e88d108ce956091da08e93971cc76b629a4de0af..988919fb8840eb407524ff2258a5f73ba20a4752 100644 (file)
--- a/src/njs_buffer.c
+++ b/src/njs_buffer.c
@@ -918,7 +918,20 @@ njs_buffer_concat(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
                 return ret;
             }
 
+            /* The getter above may have changed the element type. */
+
+            if (njs_slow_path(!njs_is_typed_array(&val))) {
+                njs_type_error(vm, "\"list[%L]\" argument must be an "
+                                   "instance of Buffer or Uint8Array", i);
+                return NJS_ERROR;
+            }
+
             arr = njs_typed_array(&val);
+            if (njs_slow_path(njs_is_detached_buffer(arr->buffer))) {
+                njs_type_error(vm, "detached buffer");
+                return NJS_ERROR;
+            }
+
             n = njs_min((size_t) len, arr->byte_length);
             src = &njs_typed_array_buffer(arr)->u.u8[arr->offset];
 

diff --git a/test/buffer.t.js b/test/buffer.t.js
index be74083a3ab737ed07790c71b1d8dcc002e86599..898e259a85cdb0b35dbe96e40121d181016767ab 100644 (file)
--- a/test/buffer.t.js
+++ b/test/buffer.t.js
@@ -98,6 +98,59 @@ let concat_tsuite = {
 };
 
 
+let concatRevalidate_tsuite = {
+    name: "Buffer.concat() element revalidation tests",
+    skip: () => (!has_buffer()),
+    T: async (params) => {
+        let stage = 0;
+        let ta = Buffer.from('abcdef');
+
+        /*
+         * A real but non-fast array (accessor on index 0) whose getter
+         * returns a valid typed array during the length pass and something
+         * else during the copy pass (TOCTOU).
+         */
+        let list = [];
+        Object.defineProperty(list, 0, {
+            enumerable: true,
+            configurable: true,
+            get() {
+                if (stage++ === 0) {
+                    return ta;
+                }
+
+                if (params.detach) {
+                    detach(ta.buffer);
+                    return ta;
+                }
+
+                return params.evil;
+            },
+        });
+
+        try {
+            Buffer.concat(list);
+
+        } catch (e) {
+            if (e instanceof TypeError) {
+                return 'SUCCESS';
+            }
+
+            throw e;
+        }
+
+        throw Error('concat() did not revalidate the swapped element');
+    },
+
+    tests: [
+        { evil: {} },
+        { evil: "not a typed array" },
+        { evil: [0, 1, 2] },
+        { skip: () => !is_detach_available(), detach: true },
+    ],
+};
+
+
 let compare_tsuite = {
     name: "Buffer.compare() tests",
     skip: () => (!has_buffer()),
@@ -1069,6 +1122,7 @@ run([
     alloc_tsuite,
     byteLength_tsuite,
     concat_tsuite,
+    concatRevalidate_tsuite,
     compare_tsuite,
     comparePrototype_tsuite,
     copy_tsuite,