Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

crypto: handle initEDRaw pkey failure #40188

Closed
wants to merge 2 commits into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Sep 23, 2021

Refs electron/electron#31087.

It's possible that

EVPKeyPointer pkey(fn(id, nullptr, key_data.data(), key_data.size()));
will fail, and KeyObjectHandle::InitEDRaw will set the return value to false. This means that the data_ member will never be assigned. However, the return value of this function is never checked by any of its callsites, meaning that a keyObject is returned but that calling any functions on it, e.g. export or asymmetricKeyType will cause a nullptr crash when they try to access key->Data().

Fix this by checking the return value from handle.initEdRaw() and throwing an error on false.

I used the error from above the callsites but please let me know if there's another preferable one!

@codebytere codebytere added the crypto Issues and PRs related to the crypto subsystem. label Sep 23, 2021
@codebytere codebytere requested a review from jasnell September 23, 2021 11:49
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 23, 2021
@codebytere codebytere requested a review from tniessen September 23, 2021 11:51
@codebytere codebytere force-pushed the handle-initedraw-failure branch from ab79e03 to 323cafb Compare September 23, 2021 11:51
@nodejs-github-bot

This comment has been minimized.

@codebytere codebytere force-pushed the handle-initedraw-failure branch from 323cafb to b73c0a1 Compare September 24, 2021 10:16
@panva panva removed the needs-ci PRs that need a full CI run. label Sep 24, 2021
@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2021
@nodejs-github-bot

This comment has been minimized.

@codebytere codebytere force-pushed the handle-initedraw-failure branch from b73c0a1 to f7f01b6 Compare September 24, 2021 18:50
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2021
@nodejs-github-bot

This comment has been minimized.

Co-authored-by: Filip Skokan <panva.ip@gmail.com>
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 25, 2021

jasnell pushed a commit that referenced this pull request Sep 25, 2021
PR-URL: #40188
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 25, 2021

Landed in 17bb7b2

@jasnell jasnell closed this Sep 25, 2021
@codebytere codebytere deleted the handle-initedraw-failure branch September 25, 2021 14:51
targos pushed a commit that referenced this pull request Oct 4, 2021
PR-URL: #40188
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Oct 11, 2021
codebytere added a commit to electron/electron that referenced this pull request Oct 11, 2021
codebytere added a commit to electron/electron that referenced this pull request Oct 11, 2021
codebytere added a commit to electron/electron that referenced this pull request Oct 11, 2021
codebytere added a commit to electron/electron that referenced this pull request Oct 13, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants