buffer: throw if both length and enc are passed by mafintosh · Pull Request #4514 · nodejs/node · GitHub

We just fixed a security issue in the bittorrent-dht module that allowed a peer to craft a specific message that would make a remote peer disclose memory.

The root cause of the issue was this snippet:

var nodeId = new Buffer(nodeIdString, 'hex')

The nodeIdString was received over the network and by sending back a number instead of a string the new Buffer constructor would return a non-zeroed out buffer. We fixed it in our code base by type checking the argument (which we of course should have done from the beginning) but it would have been easier to catch if node had thrown an exception.

I'm sure that are other modules out there that have the same kind of vulnerability that would benefit from this PR as well.

Missing semicolon

Not sure how it can be a vulnerability. If typeof arg === 'number', encoding is not used, so what's the point of the check?

The problem is that Buffer(arg, encoding) implies that arg is gonna be a string but if you pass a number as arg it'll allocate a non-zeroed out buffer instead. This means that if you have code that forgets to check if arg is a string and you get the arg from a third-party you might end up exposing internal memory.

Consider the following example

// a service that takes a json payload {hexString: str} and converts it to base64
var server = http.createServer(function (req, res) {
  var buf = ''
  req.setEncoding('utf-8')
  req.on('data', function (data) {
    buf += data
  })
  req.on('end', function () {
    var body = JSON.parse(buf)
    res.end(new Buffer(body.hexString, 'hex').toString('base64'))
  })
})

server.listen(8080)

If you post {hexString: 'aa'} to it, it will return qq==. However if you post {hexString: 20} it will return 20 bytes of internal memory as base64 since that will invoke the new Buffer(number) constructor.

Like I mentioned above this is fixable by explicitly checking if hexString is a string but since we're passing hex as the encoding this is implied and therefore it would be a help if node would throw.

@mscdex fixed the missing semicolon

@mafintosh I see. so, just to make things clear, it's not a security issue with node, just an enhancement that doesn't allow dangerous behaviour in some cases. It doesn't help when default encoding is used though.

Anyway, the change LGTM. Could you add a test for this?

@vkurchatkin yep not an issue with node itself but an easy mistake to make if you're writing an application that can have critical consequences. like i mentioned we made this mistake in the bittorrent-dht module even though the contributor list for that module include some of the most talented node developers i know.

i'll add a test

cc @nodejs/ctc

My current inclination is to LGTM this and to endorse @vkurchatkin's semver-major label rather than treat this as a bug given that it's likely being used in the wild in places where there may not be security concerns (e.g. in situations where you have full control over the source of the inputs).

If we go with semver-major I'd vote for adding a deprecation warning in v5 and v4 when you supply an enc as well as a 'number' as the first argument.

I don't think that this is a semver-major.
It looks more like a bugfix to me that is security-related (but not a security issue on its own).

The only case which this breaks is when the actual user code most probably does other thing from what the user wanted it to do, and in a way that is extremely dangereous.

Also note that just printing warning to console (if you decide to do that in LTS) won't fix anything, because if the users code contains this bug and it's not triggered under normal workload, but can be triggered by a malicious user — no one will notice those warnings until it's too late.

Also, LGTM. I would prefer a bit clearer error message though, but it's not that important.

Do not use pronouns in comments or error messages. Instead it should read something like "If encoding is specified then the first argument must be a string".

This should also include tests.

The concept of throwing in this case LGTM.

Also the commit message isn't totally accurate. It's when encoding is a string. Not that it was simply passed.

EDIT: Doing this from phone and missed that it only checks if encoding exists. That should be changed to check for string. Future API additions may make use of passing additional types as the second argument.

@trevnorris i'll update the error message + tests. if we wanted to change the api at a later stage couldn't we just add the encoding string check then? i'm thinking this will catch more weird unintentional overloading bugs if we don't explicitly check that encoding is a string

@trevnorris okay fixed the issues you mentioned and added some tests

asserts in catch block are kind of useless as they are not evaluated if nothing is thrown. Also, this will never throw anyway.

const please :-)

also, you can shorten var common = require('../common'); to simply require('../common'); since common is not directly used.

Couple of nits, otherwise LGTM

@jasnell fixed your comments

CI: https://ci.nodejs.org/job/node-test-pull-request/1150/

LGTM if CI is green.

LGTM

Checking the actual error message here would be better.

@mafintosh ... a couple linting issues: https://ci.nodejs.org/job/node-test-linter/758/console
Otherwise looks fine

@jasnell ah oops. should be fixed now

PR-URL: #4514 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Landed in 3b27dd5.
thanks @mafintosh !

Did this really land as a semver-major, not as a bugfix? Sigh.

How about just converting any value to string if encoding is passed?

@ChALkeR ... because of the additional error throw, it has to be semver-major. I know it's a pain tho.

PR-URL: nodejs#4514 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jan 2, 2016

mscdex reviewed Jan 2, 2016
View reviewed changes

vkurchatkin added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 2, 2016

feross mentioned this pull request Jan 2, 2016

Important: Critical security issue fixed in 5.1.3. All users should upgrade. webtorrent/bittorrent-dht#87

Closed

mafintosh force-pushed the patch-2 branch from 14617bd to 0189a28 Compare January 2, 2016 22:29

mafintosh force-pushed the patch-2 branch from 0189a28 to 862df8d Compare January 6, 2016 14:41

vkurchatkin reviewed Jan 6, 2016
View reviewed changes

mafintosh force-pushed the patch-2 branch from 862df8d to aca5866 Compare January 6, 2016 15:56

jasnell reviewed Jan 6, 2016
View reviewed changes

mafintosh force-pushed the patch-2 branch from aca5866 to 3da7d67 Compare January 6, 2016 16:00

thefourtheye reviewed Jan 6, 2016
View reviewed changes

buffer: throw if both length and enc are passed

b630292

mafintosh force-pushed the patch-2 branch from 3da7d67 to b630292 Compare January 6, 2016 16:56

jasnell closed this Jan 6, 2016

feross mentioned this pull request Jan 13, 2016

Buffer(number) is unsafe #4660

Closed

mafintosh deleted the patch-2 branch January 15, 2016 14:33

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

vitaly-t mentioned this pull request Apr 27, 2016

Doesn't work under Node.js 6.0.0 brianc/node-postgres#1000

Closed

buffer: throw if both length and enc are passed by mafintosh · Pull Request #4514 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: throw if both length and enc are passed - #4514

buffer: throw if both length and enc are passed#4514mafintosh wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mafintosh:patch-2mafintosh/io.js:patch-2Copy head branch name to clipboard

Conversation

mafintosh commented Jan 2, 2016

Uh oh!

mscdex Jan 2, 2016

Choose a reason for hiding this comment

Uh oh!

vkurchatkin commented Jan 2, 2016

Uh oh!

mafintosh commented Jan 2, 2016

Uh oh!

mafintosh commented Jan 2, 2016

Uh oh!

vkurchatkin commented Jan 2, 2016

Uh oh!

mafintosh commented Jan 2, 2016

Uh oh!

Fishrock123 commented Jan 3, 2016

Uh oh!

rvagg commented Jan 3, 2016

Uh oh!

ChALkeR commented Jan 3, 2016

Uh oh!

ChALkeR commented Jan 3, 2016

Uh oh!

trevnorris commented Jan 4, 2016

Uh oh!

trevnorris commented Jan 4, 2016

Uh oh!

mafintosh commented Jan 6, 2016

Uh oh!

mafintosh commented Jan 6, 2016

Uh oh!

vkurchatkin Jan 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 6, 2016

Uh oh!

mafintosh commented Jan 6, 2016

Uh oh!

jasnell commented Jan 6, 2016

Uh oh!

jasnell commented Jan 6, 2016

Uh oh!

vkurchatkin commented Jan 6, 2016

Uh oh!

thefourtheye Jan 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 6, 2016

Uh oh!

mafintosh commented Jan 6, 2016

Uh oh!

jasnell commented Jan 6, 2016

Uh oh!

ChALkeR commented Jan 7, 2016

Uh oh!

alex7kom commented Jan 15, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

buffer: throw if both length and enc are passed#4514
mafintosh wants to merge 1 commit into
nodejs:masterfrom
mafintosh:patch-2