-
Notifications
You must be signed in to change notification settings - Fork 30k
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: handle exceptions in hmac/hash.digest #12164
Conversation
|
Forced conversion of the encoding parameter to a string within crypto.js, fixing segmentation faults in node_crypto.cc. Fixes: nodejs#9819
@vsemozhetbyt Thank you, fixed via force-push. CI looks good. |
CI failure might be related to force-push?
|
src/node_crypto.cc
Outdated
encoding = ParseEncoding(env->isolate(), | ||
args[0]->ToString(env->isolate()), | ||
BUFFER); | ||
encoding = ParseEncoding(env->isolate(), args[0], BUFFER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave a CHECK(args[i]->IsString());
line before the ParseEncoding
calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, most functions just ignore invalid encoding values and use the default value instead. Currently, we are forcing the conversion to a string, but just because it is a string doesn't make it a valid encoding value. Adding a CHECK
shouldn't do any harm, until we decide to finally drop support for toString()
for consistency with other APIs. However, if we do not add a chack and the parameter is not a string (which won't happen now, but might as soon as we decide to drop support for toString()
), there won't be any problems, the value will be ignored (ParseEncoding
will return instantly).
The above only applies to hmac
and hash
. For sign
and verify
, the encoding parameter seems to always be null
, so the CHECK
would fail, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tniessen Still, if we drop toString()
support it’s easy to just drop the checks again. (Likewise, it would be good if you included the test you said you had prepared in this PR – it’s easy to change things later, but for now it’s best to test the current behaviour.)
The above only applies to
hmac
andhash
. Forsign
andverify
, the encoding parameter seems to always benull
, so theCHECK
would fail, right?
It looks that way, yes… maybe you could fix that up and drop the unused code bits? That also simplifies SignFinal()
, right now the StringBytes::Encode()
encode call there seems like a completely unnecessary extra copy operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax You are right, I will add the CHECK
s and add the regression test.
maybe you could fix that up and drop the unused code bits?
Is it okay to do that in a separate PR or will that create too much organizational overhead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to do that in a separate PR or will that create too much organizational overhead?
That’s your call… you can also maintain multiple commits in a single PR, which may or may not be easier for you. :) I don’t think there are other open pull requests for this code that would generate merge conflicts, and everything we talked about (except maybe dropping toString()
support) semver-patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create a separate PR after this gets merged to avoid conflicts and keep it simple, thank you for your support :)
lib/crypto.js
Outdated
@@ -100,7 +100,8 @@ Hash.prototype.update = function update(data, encoding) { | |||
|
|||
Hash.prototype.digest = function digest(outputEncoding) { | |||
outputEncoding = outputEncoding || exports.DEFAULT_ENCODING; | |||
return this._handle.digest(outputEncoding); | |||
// Explicit conversion for backward compatibility | |||
return this._handle.digest(String(outputEncoding)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use `${outputEncoding}`
; otherwise symbols (which were not allowed) will be converted to string also.
@addaleax I added the checks and the test file, the latter requires to spawn new node processes in order to properly detect segmentation faults (or the lack thereof) as Windows builds seem to silently terminate on segmentation faults instead of producing an error. |
const scripts = [ | ||
'crypto.createHash("sha256").digest(enc)', | ||
'crypto.createHmac("sha256", "msg").digest(enc)' | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the code in a test/fixtures
file would be a bit more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean adding the code as a JSON file or do you mean a JS file which can then be executed within the spawned node process? We are talking about two lines of code here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the latter. I'm fine with this the way it is, I'd just generally prefer a fixture. Feel free to ignore tho :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhhh I think I will keep it this way unless someone expresses a strong opposing opinion, but thank you for your feedback :)
lib/crypto.js
Outdated
@@ -100,7 +100,8 @@ Hash.prototype.update = function update(data, encoding) { | |||
|
|||
Hash.prototype.digest = function digest(outputEncoding) { | |||
outputEncoding = outputEncoding || exports.DEFAULT_ENCODING; | |||
return this._handle.digest(outputEncoding); | |||
// Explicit conversion for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Femto-nit: can you punctuate the comment?
Landed in 88351a2, thanks for the PR! |
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>
I feel like this should maybe bake a bit longer before LTS. Thoughts? |
I don’t think this needs to wait longer than usual for LTS. |
I don't think we need to rush it in, but it should not cause any trouble either (unless someone depends on segfaults). |
This ought to be good to go for LTS at this point. |
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>
Hash.digest()
andHmac.digest()
lead to a segmentation fault if an error was thrown while converting the encoding parameter to a string, see #9819. As discussed there, we could alternatively handle errors incrypto.cc
or drop support fortoString()
(which is consistent with other core APIs, see #9819 again).Fixes: #9819
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
crypto
I prepared a regression test as well but refrained from committing it as it might promote the use of
toString()
for the encoding parameter, which we might decide not to support in the future.