buffer: fix range checks for slice()

Using the black magic of Symbol.toPrimitive the numeric value of
start/end can be changed when Uint32Value() is called once
Buffer::Fill() is entered. Allowing the CHECK() to be bypassed.

The bug report was only for "start", but the same can be done with
"end". Perform checks for both in node::Buffer::Fill() to make sure the
issue can't be triggered, even if process.binding is used directly.

Include tests for each case. Along with a check to make sure the last
time the value is accessed returns -1. This should be enough to make
sure Buffer::Fill() is receiving the correct value. Along with two tests
against process.binding directly.

Fixes: nodejs#9149
PR-URL: nodejs#9174
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

src/node_buffer.cc

@@ -585,7 +585,8 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
   Local<String> str_obj;
   size_t str_length;
   enum encoding enc;
-  CHECK(fill_length + start <= ts_obj_length);
+  THROW_AND_RETURN_IF_OOB(start <= end);
+  THROW_AND_RETURN_IF_OOB(fill_length + start <= ts_obj_length);
 
   // First check if Buffer has been passed.
   if (Buffer::HasInstance(args[1])) {

test/parallel/test-buffer-fill.js

@@ -314,3 +314,79 @@ Buffer.alloc(8, '');
   buf.fill('է');
   assert.strictEqual(buf.toString(), 'էէէէէ');
 }
+
+// Testing public API. Make sure "start" is properly checked, even if it's
+// magically mangled using Symbol.toPrimitive.
+{
+  let elseWasLast = false;
+  assert.throws(() => {
+    var ctr = 0;
+    const start = {
+      [Symbol.toPrimitive]() {
+        // We use this condition to get around the check in lib/buffer.js
+        if (ctr <= 0) {
+          elseWasLast = false;
+          ctr = ctr + 1;
+          return 0;
+        } else {
+          elseWasLast = true;
+          // Once buffer.js calls the C++ implemenation of fill, return -1
+          return -1;
+        }
+      }
+    };
+    Buffer.alloc(1).fill(Buffer.alloc(1), start, 1);
+  }, /out of range index/);
+  // Make sure -1 is making it to Buffer::Fill().
+  assert.ok(elseWasLast,
+            'internal API changed, -1 no longer in correct location');
+}
+
+// Testing process.binding. Make sure "start" is properly checked for -1 wrap
+// around.
+assert.throws(() => {
+  process.binding('buffer').fill(Buffer.alloc(1), 1, -1, 0, 1);
+}, /out of range index/);
+
+// Make sure "end" is properly checked, even if it's magically mangled using
+// Symbol.toPrimitive.
+{
+  let elseWasLast = false;
+  assert.throws(() => {
+    var ctr = 0;
+    const end = {
+      [Symbol.toPrimitive]() {
+        // We use this condition to get around the check in lib/buffer.js
+        if (ctr <= 1) {
+          elseWasLast = false;
+          ctr = ctr + 1;
+          return 1;
+        } else {
+          elseWasLast = true;
+          // Once buffer.js calls the C++ implemenation of fill, return -1
+          return -1;
+        }
+      }
+    };
+    Buffer.alloc(1).fill(Buffer.alloc(1), 0, end);
+  });
+  // Make sure -1 is making it to Buffer::Fill().
+  assert.ok(elseWasLast,
+            'internal API changed, -1 no longer in correct location');
+}
+
+// Testing process.binding. Make sure "end" is properly checked for -1 wrap
+// around.
+assert.throws(() => {
+  process.binding('buffer').fill(Buffer.alloc(1), 1, 1, -2, 1);
+}, /out of range index/);
+
+// Test that bypassing 'length' won't cause an abort.
+assert.throws(() => {
+  const buf = new Buffer('w00t');
+  Object.defineProperty(buf, 'length', {
+    value: 1337,
+    enumerable: true
+  });
+  buf.fill('');
+});