Skip to content

Commit

Permalink
buffer: fix range checks for slice()
Browse files Browse the repository at this point in the history
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 MylesBorins committed Dec 20, 2016
1 parent fa7d378 commit a9d528b
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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])) {
Expand Down
76 changes: 76 additions & 0 deletions test/parallel/test-buffer-fill.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 a9d528b

Please sign in to comment.