buffer: throw if both length and enc are passed

[mafintosh]

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:

varnodeId=newBuffer(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.

[mscdex]

Missing semicolon

[vkurchatkin]

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

[mafintosh]

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 base64varserver=http.createServer(function(req,res){varbuf=''req.setEncoding('utf-8')req.on('data',function(data){buf+=data})req.on('end',function(){varbody=JSON.parse(buf)res.end(newBuffer(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.

[mafintosh]

@mscdex fixed the missing semicolon

[vkurchatkin]

@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?

[mafintosh]

@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

[Fishrock123]

cc @nodejs/ctc

[rvagg]

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.

[ChALkeR]

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.

[ChALkeR]

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

[trevnorris]

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.

[trevnorris]

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.

[mafintosh]

@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

[mafintosh]

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

[vkurchatkin]

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

[jasnell]

const please :-)

[jasnell]

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

[jasnell]

Couple of nits, otherwise LGTM

[mafintosh]

@jasnell fixed your comments

[jasnell]

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

[jasnell]

LGTM if CI is green.

[vkurchatkin]

LGTM

[thefourtheye]

Checking the actual error message here would be better.

[jasnell]

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

[mafintosh]

@jasnell ah oops. should be fixed now

[jasnell]

Landed in 3b27dd5.
thanks @mafintosh !

[ChALkeR]

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

[alex7kom]

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

[jasnell]

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