Crypto Hashm/Hmac digest segfault on bad input

[deian]

• 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.

[tniessen]

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]

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]

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]

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?

[deian]

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