diff options
author | Trevor Norris <trev.norris@gmail.com> | 2016-10-18 16:42:17 -0600 |
---|---|---|
committer | Trevor Norris <trev.norris@gmail.com> | 2016-10-20 13:40:47 -0600 |
commit | 7bffe23da01457410a11ae40ee3a9d3be97e6c7b (patch) | |
tree | 380cf05edd7bf5e6f90dcb3e6acb58e739841460 | |
parent | 6845d6e3189cb6f7d2c847ab34a4f5b2eaf7a77f (diff) | |
download | android-node-v8-7bffe23da01457410a11ae40ee3a9d3be97e6c7b.tar.gz android-node-v8-7bffe23da01457410a11ae40ee3a9d3be97e6c7b.tar.bz2 android-node-v8-7bffe23da01457410a11ae40ee3a9d3be97e6c7b.zip |
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: https://github.com/nodejs/node/issues/9149
PR-URL: https://github.com/nodejs/node/pull/9174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
-rw-r--r-- | src/node_buffer.cc | 3 | ||||
-rw-r--r-- | test/parallel/test-buffer-fill.js | 76 |
2 files changed, 78 insertions, 1 deletions
diff --git a/src/node_buffer.cc b/src/node_buffer.cc index d7c27295e9..07a4106642 100644 --- a/src/node_buffer.cc +++ b/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])) { diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js index e5581b2d83..c61ad59d7e 100644 --- a/test/parallel/test-buffer-fill.js +++ b/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(''); +}); |