Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
feross opened this issue May 29, 2016 · 4 comments
Closed

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

feross opened this issue May 29, 2016 · 4 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@feross
Copy link
Contributor

feross commented May 29, 2016

  • Version: v6.2.0
  • Platform: Windows 10, possibly others
  • Subsystem: buffer

I've had multiple users report a RangeError: Attempt to write outside buffer bounds error that originates inside the Buffer constructor.

Here's some code that triggers it, according to the bug reports that I've received.

var buf = new Buffer(infoHash, 'hex')

Example stack traces:

Uncaught RangeError: Attempt to write outside buffer bounds
Buffer.write @ buffer.js:772
fromString @ buffer.js:238
Buffer.from @ buffer.js:131
Buffer @ buffer.js:112
Wire.handshake @ .../node_modules/bittorrent-protocol/index.js:192
RangeError: Attempt to write outside buffer bounds
    at Buffer.write (buffer.js:772:11)
    at fromString (buffer.js:238:26)
    at Function.Buffer.from (buffer.js:131:12)
    at new Buffer (buffer.js:112:17)
    at Function.encode.buffer (../webtorrent-cli/node_modules/bencode/lib/encode.js:61:17)

The call to buf.write that's crashing is in core, and it's here. Is the buffer pool offset incorrect somehow?

I don't understand how this could happen. Apologies for not having a reproducible test case. I haven't been able to reproduce it yet and I'm out of ideas.

@targos targos added the buffer Issues and PRs related to the buffer subsystem. label May 29, 2016
@addaleax
Copy link
Member

I think I have narrowed this down a bit… is there a chance that something tried to allocate a buffer of negative size using allocUnsafe()/new Buffer(n)? That would make poolSize negative and thereby out of bounds.

PR for that: #7051

@feross
Copy link
Contributor Author

feross commented May 30, 2016

@addaleax That must be what was happening. There must be someplace where I'm passing a negative number received from a remote peer into the Buffer constructor.

This is actually a potential security issue because:

// Make buffer, fill it with a's
var buf1 = Buffer(10).fill('a')

// Reset the pool offset back to where it was before. No exception thrown!
// This happens if the programmer passes in unvalidated user data into the Buffer constructor
// (same as the previous Buffer issue, fixed with Buffer.from, etc.)
Buffer(-20)

// Makes a buffer that OVERLAPS with buf1! Write b's into it.
var buf2 = Buffer(10).fill('b')

console.log(buf1.toString()) // prints out 'bbbbbbbbbb'

@feross
Copy link
Contributor Author

feross commented May 30, 2016

Even if the user uses Buffer.from, etc. throughout most of their code, if there's just one place where they do Buffer(userJson.prop) and a negative number gets passed in from user input, then future Buffer.from calls could overlap a previous buffer without warning.

@rvagg
Copy link
Member

rvagg commented May 30, 2016

In future, if you think there are security implications to a bug you're seeing then it's best to use security@nodejs.org so we can handle it in a more controlled manner. In this case it's not clear that we would handle it differently to what's being done in public because it falls in the same category as what .from and .alloc* were introduced to deal with but it might have been best for us to have this conversation in private anyway.

Fishrock123 pushed a commit that referenced this issue Jun 1, 2016
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>
rvagg pushed a commit that referenced this issue Jun 2, 2016
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>
ChALkeR pushed a commit to ChALkeR/io.js that referenced this issue Jun 17, 2016
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>
ChALkeR pushed a commit to ChALkeR/io.js that referenced this issue Jun 19, 2016
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>
ChALkeR pushed a commit to ChALkeR/io.js that referenced this issue Jul 8, 2016
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>
MylesBorins pushed a commit that referenced this issue Jul 8, 2016
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>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
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>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants