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

fix: crash creating private key with unsupported algorithm #31087

Merged
merged 2 commits into from Sep 27, 2021

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Sep 23, 2021

Description of Change

Closes #31064.

Fixed an issue where some calls to crypto.createPrivateKey made with algorithms unsupported by BoringSSL cause a crash when invoking methods on their return values. This was happening because BoringSSL shims some algorithms but doesn't implement them and so attempts to create keys with them will fail (see: evp.h#L835-837)

Node.js returns false in initEdRaw (see crypto_keys.cc#L1106)
but then does nothing with the return value, meaning that if no pkey is created successfully that a key object will still be returned. However, attempts to use the data_ field will crash the process as data_ is never assigned. This is fixed by checking the return value of initEdRaw at the JavaScript level and throwing an error if the function returns false.

cc @deermichel

Checklist

Release Notes

Notes: Fixed an issue where some calls to crypto.createPrivateKey made with algorithms unsupported by BoringSSL cause a crash when invoking methods on their return values.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/15-x-y labels Sep 23, 2021
@codebytere codebytere requested a review from a team as a code owner September 23, 2021 10:40
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 23, 2021
@BlackHole1
Copy link
Member

I would like to ask a question, why does nodejs 16.4.1 not raise crashes.

In my results, it is correct, e.g:

图片

Does that mean that the problem has been fixed in the new version of the Nodejs code?
If so, why didn't we just bring in the patch for the nodejs fix, instead of introducing a patch we wrote ourselves?

@codebytere
Copy link
Member Author

@BlackHole1 this is outlined in my PR body - Electron uses BoringSSL and Node.js uses OpenSSL. So this will fail in Electron where it would work in Node.js because the underlying crypto libraries differ.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 24, 2021
@codebytere codebytere merged commit 25d0963 into main Sep 27, 2021
@codebytere codebytere deleted the fix-jwk-crash branch September 27, 2021 13:02
@release-clerk
Copy link

release-clerk bot commented Sep 27, 2021

Release Notes Persisted

Fixed an issue where some calls to crypto.createPrivateKey made with algorithms unsupported by BoringSSL cause a crash when invoking methods on their return values.

@trop
Copy link
Contributor

trop bot commented Sep 27, 2021

I have automatically backported this PR to "16-x-y", please check out #31136

@trop
Copy link
Contributor

trop bot commented Sep 27, 2021

I have automatically backported this PR to "15-x-y", please check out #31137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: crypto JWK Key Export results in SIGSEGV
3 participants