-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: fix rsa key gen with non-default exponent #27092
crypto: fix rsa key gen with non-default exponent #27092
Conversation
EVP_PKEY_CTX_set_rsa_keygen_pubexp() accepts ownership of the exponent on success, so do not free it. Fixes: nodejs#27087
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.
LGTM. Good catch.
@@ -146,7 +146,7 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher); | |||
|
|||
// Now do the same with an encrypted private key. | |||
generateKeyPair('rsa', { | |||
publicExponent: 0x10001, |
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.
Can we keep this exponent in both tests (here and the change above) as an additional test parameter?
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.
@mscdex exponent 0x10001 is tested 4 times in test-crypto-keygen.js, I don't mind copy and pasting the tests with non-default exponents, but don't think it will increase coverage. Can you confirm you want me to do this? For your reference:
core/node (fix-non-default-exp-rsa-keygen $%) % grep publicExponent test/parallel/test-crypto-keygen.js
publicExponent: 3, // I changed this from 0x0001
publicExponent: 0x10001,
publicExponent: 0x1001, // I changed this from 0x0001
publicExponent: 0x10001,
publicExponent: 0x10001,
publicExponent: 0x10001,
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 see, either way is fine then.
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.
Doesn't release
return the pointer? I am not sure, maybe it doesn't :)
yes, release both returns the pointer, and nulls it (so the auto ptr doesn't own it). But its the only function to use (I think). |
Landed in 0911e88 |
EVP_PKEY_CTX_set_rsa_keygen_pubexp() accepts ownership of the exponent on success, so do not free it. Fixes: #27087 PR-URL: #27092 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
FWIW this fixes a crash that currently exists on latest v10.x. |
EVP_PKEY_CTX_set_rsa_keygen_pubexp() accepts ownership of the exponent on success, so do not free it. Fixes: #27087 Fixes: #29433 PR-URL: #27092 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
EVP_PKEY_CTX_set_rsa_keygen_pubexp() accepts ownership of the exponent
on success, so do not free it.
Fixes: #27087
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes