buffer: fix range checks for slice()

trevnorris · MylesBorins · commit a9d528be5bf6 · 2016-12-20T15:28:35.000-05:00
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: #9149 PR-URL: #9174 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -580,7 +580,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])) {
diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js
@@ -314,3 +314,79 @@ Buffer.alloc(8, '');
   buf.fill('&#1383;');
   assert.strictEqual(buf.toString(), '&#1383;&#1383;&#1383;&#1383;&#1383;');
 }
+
+// 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('');
+});
