-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 unencrypted DER PKCS8 parsing #26236
crypto: fix unencrypted DER PKCS8 parsing #26236
Conversation
The previously used OpenSSL call only supports encrypted PKCS8, this commit adds support for unencrypted PKCS8.
@nodejs/crypto |
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.
You can test by running the decoding in a loop over a valid key, something like this pseudo-code.
valid = valid_der;
for (i = 0; i < valid.size; i++) {
invalid = valid.truncate(i)
let err
try {
parse(invalid)
} catch (e) {
err = e;
}
if (i < valid.size()) assert(err !== null)
else assert.ifError(err)
}
We don't usually do this, because we assume that OpenSSL has a correct bug-free decoder, but if we have our own, I think we should be careful to test invalid data.
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 checked out this branch, and this change literally is exactly what was needed to make easy-crypto work and what not.
Great work, @tniessen! 🎉
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
Thanks for reviewing, landed in 8d69fdd. |
The previously used OpenSSL call only supports encrypted PKCS8, this commit adds support for unencrypted PKCS8. PR-URL: #26236 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The previously used OpenSSL call only supports encrypted PKCS8, this commit adds support for unencrypted PKCS8. PR-URL: #26236 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@addaleax this should land on all release lines, right? |
I think that’s a question for @tniessen :) |
The previously used OpenSSL call only supports encrypted PKCS8, this commit adds support for unencrypted PKCS8. PR-URL: #26236 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ryzokuken I believe the key object API (which also added support for DER) has only landed on v11 so far so this only applies to v11. |
The previously used OpenSSL call only supports encrypted PKCS8, this commit adds support for unencrypted PKCS8. This only affects keys that are encoded as DER using PKCS#8 without encryption, which probably explains why nobody noticed this earlier.
cc @nodejs/crypto
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes