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

Crypto Hashm/Hmac digest segfault on bad input #9819

Closed
deian opened this issue Nov 28, 2016 · 5 comments
Closed

Crypto Hashm/Hmac digest segfault on bad input #9819

deian opened this issue Nov 28, 2016 · 5 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@deian
Copy link
Member

deian commented Nov 28, 2016

  • Version: 6.4.0 - 8.0.0
  • Platform:
  • Subsystem:

Both Hash's and Hmac's digest binding functions hard crash when given an object
that either defines a throwing getter or throwing toString. For example:

  crypto.createHash('sha256').digest({ toString: () => { throw 'w00t'; }});

and:

  crypto.Hmac("sha256", "message").digest({ toString: () => { throw 'w00t'; }});

both crash because they call ParseEncoding with an empty v8::Value:

    ParseEncoding(env->isolate(),
                  args[0]->ToString(env->isolate()),
                  BUFFER);

Internally, PraseEncoding calls encoding_v->IsString() without checking if
the value is Empty, hence the crash.

May be worth checking other callsites for ParseEncoding. The binding code for
verify.verify() calls ParseEncoding too, but the actual encoding argument
from JS land is never passed in. (This is similar to the unused code I
mentioend in #9817, but for sign().)

+@mlfbrown for joint work.

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Nov 28, 2016
@tniessen
Copy link
Member

tniessen commented Apr 2, 2017

After fixing this, I noticed that the crypto API is more or less the only part of node which uses ToString() on encodings. It might be better to simply drop support for toString(). The only edge case is

> crypto.createHash('sha256').digest('hex')
'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'
> crypto.createHash('sha256').digest(new String('hex'))
<Buffer e3 b0 c4 42 98 fc 1c 14 9a fb f4 c8 99 6f b9 24 27 ae 41 e4 64 9b 93 4c a4 95 99 1b 78 52 b8 55>

but this is consistent with other APIs:

> Buffer.from('abcd', 'hex')
<Buffer ab cd>
> Buffer.from('abcd', new String('hex'))
<Buffer 61 62 63 64>

If this is not okay, I can create a PR which correctly handles exceptions thrown when evaluating toString().

@addaleax Hope it's okay to ping you about this, what do you think? :)

@addaleax
Copy link
Member

addaleax commented Apr 2, 2017

I don’t have a strong preference, but dropping toString() support might be semver-major – I find it hard to think of a scenario in which somebody might rely on it, but it’s hard to know for sure.

The version of ToString() that returns a Local is being deprecated exactly because of these error handling pitfalls (at least that’s what I assume), so the most direct approach would probably be switching to the MaybeLocal version and returning back to JS if the result is empty.

And yes, it’s definitely okay to ping me. :)

@tniessen
Copy link
Member

tniessen commented Apr 2, 2017

so the most direct approach would probably be switching to the MaybeLocal version and returning back to JS if the result is empty.

This is exactly how I fixed it.

Alternatively, we could do the conversion in crypto.js (String(outputEncoding) should be enough) and drop the additional logic in crypto.cc.

ParseEncoding will silently ignore all invalid values, unless they are empty. I'd suggest to add CHECK(!encoding_v.IsEmpty()) at the beginning of ParseEncoding even if we fix this specific problem in the crypto module.

@addaleax
Copy link
Member

addaleax commented Apr 2, 2017

Alternatively, we could do the conversion in crypto.js (String(outputEncoding) should be enough) and drop the additional logic in crypto.cc.

I feel like @bnoordhuis might be into that option. 😛

But really, everything you suggest here sounds okay to me – want to go ahead and open a PR?

tniessen added a commit to tniessen/node that referenced this issue Apr 2, 2017
Forced conversion of the encoding parameter to a string within
crypto.js, fixing segmentation faults in node_crypto.cc.

Fixes: nodejs#9819
@deian
Copy link
Member Author

deian commented Apr 2, 2017

@tniessen I think handling it properly in C++ as you did is the best way to do it, even if you end up doing casts in crypto.js. It's pretty easy to get at functions indirectly even if process.binding is hidden at some point.

Thank you both for going through these!

italoacasas pushed a commit to italoacasas/node that referenced this issue Apr 10, 2017
Forced conversion of the encoding parameter to a string within
crypto.js, fixing segmentation faults in node_crypto.cc.

Fixes: nodejs#9819
PR-URL: nodejs#12164
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 15, 2017
Forced conversion of the encoding parameter to a string within
crypto.js, fixing segmentation faults in node_crypto.cc.

Fixes: #9819
PR-URL: #12164
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Forced conversion of the encoding parameter to a string within
crypto.js, fixing segmentation faults in node_crypto.cc.

Fixes: nodejs/node#9819
PR-URL: nodejs/node#12164
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
ryzokuken added a commit to ryzokuken/node that referenced this issue Mar 13, 2018
Rename the tests appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the tests conform
to the standard test structure

- Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault
- Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress
- Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port
- Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe
- Rename test-regress-nodejsGH-6235 to test-v8-global-setter

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
lpinca pushed a commit that referenced this issue Mar 13, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault
- Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress
- Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port
- Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe
- Rename test-regress-nodejsGH-6235 to test-v8-global-setter

PR-URL: nodejs#19275
Refs: nodejs#19105
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants