Permalink
Browse files

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: #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>
  • Loading branch information...
trevnorris authored and jasnell committed Oct 18, 2016
1 parent 3ab8be0 commit 60042ca70ee06983edd385d01f1617cf5ff64444
Showing with 78 additions and 1 deletion.
  1. +2 −1 src/node_buffer.cc
  2. +76 −0 test/parallel/test-buffer-fill.js
@@ -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])) {
@@ -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('');
});

0 comments on commit 60042ca

Please sign in to comment.