buffer: ignore negative allocation lengths by addaleax · Pull Request #7051 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

Treat negative length arguments to Buffer()/allocUnsafe() as if they were zero so the allocation does not affect the pool’s offset.

Ref: #7047

I decided to not throw an error here because there’s at least one existing test expecting Buffer(-1) to not throw, but I’d like to make a semver-major follow-up PR for adding a < 0 check to assertSize. (That would implement this suggestion by @thefourtheye, I think.)

/cc @nodejs/buffer

CI: https://ci.nodejs.org/job/node-test-commit/3570/

Would it be better if this check is in the allocate itself?

Updated :)

LGTM. Let's hear from @jasnell and @trevnorris as well.

LGTM

I believe this should get released soon because it may affect the security of user code. See #7047 (comment)

LGTM

Observation: Buffer.allocUnsafe(-1) returns an empty buffer, Buffer.allocUnsafeSlow(-1) throws an exception. Not very consistent.

It would be better if the negative allocation also done within this test itself. If the code is moved around later, it will not fail

Maybe wrap in a { } closure?

LGTM

@bnoordhuis Those should probably both throw, but changing that would be a semver-major.

@nodejs/ctc ptal, especially re: the above

Updated with @Fishrock123’s suggestion of wrapping the test block in { }

Has the problematic commit been bisected? I fixed a similar issue a couple years ago and it should have been throwing (@bnoordhuis may remember this as one of my first contributions, finding how to read from random locations in memory).

I'd change this PR to throw. We already do if the range is to large, and in the case Ben already mentioned.

Edit: I see an existing test where it returned a zero length Buffer on negative number. I'll need to verify something.

@trevnorris I’m pretty sure this comes from the introduction of Buffer.alloc*, see the this comment that’s also linked above in the description (also strongly supported by the fact that this bug popped up in v5.10.0).

I’d be happy to change this PR to throwing if there’s a clear majority here that thinks that would pass as a semver-patch; as mentioned, there’s at least one test expecting the old behaviour (from d157131).

Alright, seems the case I'm thinking of is in 498200b (thanks Ben for fixing that) and has to do with SlowBuffer.

For expediency, LGTM for the presented fix, but I'd also suggest we consider throwing in the next semver-major.

Now, i'm back off to my vacation. Thanks all for taking care of this.

@bnoordhuis

Observation: Buffer.allocUnsafe(-1) returns an empty buffer, Buffer.allocUnsafeSlow(-1) throws an exception. Not very consistent.

I'm sure you realize this, but for posterity, that's pretty much because allocUnsafeSlow() is basically an extension of SlowBuffer, which you made to throw on invalid size in the commit listed just above. I agree this is the behavior Buffer should follow, but also I'm sure there'll be plenty of debate about whether a breaking change should be allowed back into v6. So I say we introduce that behavior in v7, land this in v6 and call it a day.

@trevnorris A semver-major follow-up is exactly the plan, yep. Have fun on your vacation!

EDIT: One more CI: https://ci.nodejs.org/job/node-test-commit/3587/

LGTM, ignore that ARM failure, those are new machines and the error is very unlikely to be Node specific (same OOM errors showing up for other runs too). Also 👍 from me for the approach of following up with a semver-major @addaleax.

For posterity, the regression was introduced in c1534e7. Making, I think, a good case for why larger changes like that don't, at least in the short or medium term, get backported to LTS releases.

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: nodejs#7047 PR-URL: nodejs#7051 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Thanks for the reviews, everyone.

PR for throwing upon encountering a negative argument: #7079

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: #7047 PR-URL: #7051 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

# Notable changes ## Notable changes * **buffer**: Ignore negative lengths in calls to `Buffer()` and `Buffer.allocUnsafe()`. This fixes a possible security concern (reported by Feross Aboukhadijeh) where user input is passed unchecked to the Buffer constructor or `allocUnsafe()` as it can expose parts of the memory slab used by other Buffers in the application. Note that negative lengths are not supported by the Buffer API and user input to the constructor should always be sanitised and type-checked. (Anna Henningsen) [#7051](nodejs/node#7051) * **npm**: Upgrade npm to 3.9.3 (Kat Marchán) [#7030](nodejs/node#7030) - [`npm/npm@42d71be`](npm/npm@42d71be) [npm/npm#12685](npm/npm#12685) When using `npm ls <pkg>` without a semver specifier, `npm ls` would skip any packages in your tree that matched by name, but had a prerelease version in their `package.json`. ([@zkat](https://github.com/zkat)) - [`npm/npm@f04e05`](npm/npm@df04e05) [npm/npm#10013](npm/npm#10013) `read-package-tree@5.1.4`: Fixes an issue where `npm install` would fail if your `node_modules` was symlinked. ([@iarna](https://github.com/iarna)) - [`b894413`](npm/npm@b894413) [#12372](npm/npm#12372) Changing a nested dependency in an `npm-shrinkwrap.json` and then running `npm install` would not get up the updated package. This corrects that. ([@misterbyrne](https://github.com/misterbyrne)) - This release includes `npm@3.9.0`, which is the result of our Windows testing push -- the test suite (should) pass on Windows now. We're working on getting AppVeyor to a place where we can just rely on it like Travis. * **tty**: Default to blocking mode for stdio on OS X. A bug fix in libuv 1.9.0, introduced in Node.js v6.0.0, exposed problems with Node's use of non-blocking stdio, particularly on OS X which has a small output buffer. This change should fix CLI applications that have been having problems with output since Node.js v6.0.0 on OS X. The core team is continuing to address stdio concerns that exist across supported platforms and progress can be tracked at <nodejs/node#6980>. (Jeremiah Senkpiel) [#6895](nodejs/node#6895) * **V8**: Upgrade to V8 5.0.71.52. This includes a fix that addresses problems experienced by users of node-inspector since Node.js v6.0.0, see <node-inspector/node-inspector#864> for details. (Michaël Zasso) [#6928](nodejs/node#6928)

This should be backported to 5.x, perhaps — 5.x is still alive.

fwiw, the commit here lands cleanly on v5.x, so the rest is mostly on @nodejs/release I think?

@addaleax I'm going to file a PR to 5.x with several buffer-related backports in several minutes (after the tests pass).

@addaleax Backport in #7169.

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: nodejs#7047 Refs: nodejs#7051 PR-URL: nodejs#7221 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: nodejs#7047 Refs: nodejs#7051 Refs: nodejs#7221 Refs: nodejs#7475 PR-URL: nodejs#7562 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: #7047 Refs: #7051 Refs: #7221 Refs: #7475 PR-URL: #7562 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label May 29, 2016

addaleax mentioned this pull request May 29, 2016

Inexplicable "RangeError: Attempt to write outside buffer bounds" #7047

Closed

thefourtheye reviewed May 29, 2016
View reviewed changes

addaleax force-pushed the buffer-negative-allocunsafe branch from 8196086 to 6796278 Compare May 29, 2016 19:13

thefourtheye reviewed May 30, 2016
View reviewed changes

Fishrock123 mentioned this pull request May 30, 2016

Propose v6.2.1 #6934

Closed

Fishrock123 added this to the 6.2.1 milestone May 30, 2016

addaleax force-pushed the buffer-negative-allocunsafe branch from 1902371 to ef9a8fa Compare May 31, 2016 17:50

addaleax merged commit ef9a8fa into nodejs:master May 31, 2016

addaleax deleted the buffer-negative-allocunsafe branch May 31, 2016 17:54

yorkie mentioned this pull request May 31, 2016

buffer: throw on negative .allocUnsafe() argument #7079

Closed

3 tasks

addaleax mentioned this pull request May 31, 2016

buffer: improve creation performance #6893

Closed

2 tasks

rvagg mentioned this pull request Jun 2, 2016

Release proposal: v6.2.1 #7108

Closed

cjean-fr mentioned this pull request Jun 4, 2016

Update NodeJs to 6.2.1 (security update) ScoopInstaller/Scoop#891

Merged

addaleax added the dont-land-on-v4.x label Jun 5, 2016

ChALkeR mentioned this pull request Jun 5, 2016

Backport important Buffer changes to v5.x #7169

Merged

ChALkeR mentioned this pull request Jun 8, 2016

Backport «buffer: ignore negative allocation lengths» to v5.x #7221

Merged

ChALkeR mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

buffer: ignore negative allocation lengths by addaleax · Pull Request #7051 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: ignore negative allocation lengths - #7051

buffer: ignore negative allocation lengths#7051addaleax merged 1 commit intonodejs:masternodejs/node:masterfrom addaleax:buffer-negative-allocunsafeaddaleax/node:buffer-negative-allocunsafeCopy head branch name to clipboard

Conversation

addaleax commented May 29, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax commented May 29, 2016

Uh oh!

thefourtheye May 29, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax May 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented May 29, 2016

Uh oh!

feross commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented May 30, 2016

Uh oh!

bnoordhuis commented May 30, 2016

Uh oh!

thefourtheye May 30, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 May 30, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented May 30, 2016

Uh oh!

ChALkeR commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented May 30, 2016

Uh oh!

addaleax commented May 30, 2016

Uh oh!

trevnorris commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 30, 2016

Uh oh!

trevnorris commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trevnorris commented May 30, 2016

Uh oh!

addaleax commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvagg commented May 31, 2016

Uh oh!

trevnorris commented May 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 31, 2016

Uh oh!

ChALkeR commented Jun 5, 2016

Uh oh!

addaleax commented Jun 5, 2016

Uh oh!

ChALkeR commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChALkeR commented Jun 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

buffer: ignore negative allocation lengths#7051
addaleax merged 1 commit into
nodejs:masterfrom
addaleax:buffer-negative-allocunsafe

addaleax May 29, 2016 •
edited
Loading

feross commented May 30, 2016 •
edited
Loading

ChALkeR commented May 30, 2016 •
edited
Loading

trevnorris commented May 30, 2016 •
edited
Loading

trevnorris commented May 30, 2016 •
edited
Loading

addaleax commented May 30, 2016 •
edited
Loading

trevnorris commented May 31, 2016 •
edited
Loading

ChALkeR commented Jun 5, 2016 •
edited
Loading