FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix error handling for CryptoJob::ToResult by tniessen · Pull Request #37076 · nodejs/node · GitHub

/ node Public

src: fix error handling for CryptoJob::ToResult - #37076

Merged
Trott merged 1 commit into
nodejs:masterfrom
tniessen:crypto-fix-error-handling-for-toresult
Apr 3, 2021
ConversationCommits (1)ChecksFiles changed
Merged

src: fix error handling for CryptoJob::ToResult#37076
Trott merged 1 commit into
nodejs:masterfrom
tniessen:crypto-fix-error-handling-for-toresult

Conversation

tniessen commented Jan 26, 2021

Copy link
Copy Markdown
Member

The added test case crashes in recent versions of Node.js 15 due to a change in OpenSSL and improper error handling in node core.

The first problem is that ManagedEVPPKey::ToEncodedPublicKey and ManagedEVPPKey::ToEncodedPrivateKey returned Just(false) to indicate that an exception was thrown, but KeyPairGenTraits::EncodeKey only checks IsNothing() and not FromJust(). This leads to JavaScript handles being empty, and, consequently, to a crash in V8.

The next problem is that some code paths in ToResult throw exceptions whereas others leave error handling to CryptoJob::AfterThreadPoolWork. In the test case added here, the function WritePublicKey throws an exception because the curve does not have an OID.

I am not entirely sure why these functions use Maybe<bool> as their return type. Overall, I am pretty sure we need to rework substantial parts of the error handling logic in the crypto subsystem. Hopefully, this PR can still provide a reasonable workaround to avoid crashes for now.

I tried to keep the existing behavior of ignoring any results that are equal to Just(false), meaning that returning Just(false) from ToResult causes the operation to never complete or fail.

When ToResult returns Nothing<bool>(), the AfterThreadPoolWork function now assumes that an exception was thrown, and passes it to the job callback.

In synchronous mode, both Just(false) and Nothing<bool>() are ignored, which leads to exceptions being propagated to JavaScript correctly.

@jasnell I'd love to hear your opinion. I am happy to implement and test another solution. We should clarify the semantics of the tristate Maybe<bool>.

tniessen added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 26, 2021
tniessen requested a review from jasnell January 26, 2021 14:12
nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 26, 2021
tniessen removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 26, 2021

jasnell commented Jan 26, 2021

Copy link
Copy Markdown
Member

Overall, I am pretty sure we need to rework substantial parts of the error handling logic in the crypto subsystem

Big +1 to this. It's been on my todo list for a while.

jasnell approved these changes Jan 26, 2021
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This LGTM but you're 100% correct that the error handling in the entire subsystem needs a good audit

This comment has been minimized.

Sign in to view

tniessen commented Jan 26, 2021

Copy link
Copy Markdown
Member Author

CI is failing on ubuntu1804_sharedlibs_openssl111_x64, most likely because it uses an older OpenSSL 1.1.1 release that exports keys on unnamed curves without curve information. (This is not desirable behavior, but not a fault on our side, and fixed in more recent OpenSSL 1.1.1 releases.)

This comment has been minimized.

Sign in to view
tniessen force-pushed the crypto-fix-error-handling-for-toresult branch from 49a1a79 to 313002a Compare January 26, 2021 15:30

nodejs-github-bot commented Jan 26, 2021 •
edited by tniessen
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35753/ (Unrelated OSX failure.)

richardlau commented Jan 26, 2021

Copy link
Copy Markdown
Member

CI is failing on ubuntu1804_sharedlibs_openssl111_x64, most likely because it uses an older OpenSSL 1.1.1 release that exports keys on unnamed curves without curve information. (This is not desirable behavior, but not a fault on our side, and fixed in more recent OpenSSL 1.1.1 releases.)

We're currently using OpenSSL 1.1.1g in the sharedlibs container in the CI: https://github.com/nodejs/build/blob/90e726898ac71c9c19690cd15b5b0027901c79cb/ansible/roles/docker/templates/ubuntu1804_sharedlibs.Dockerfile.j2#L51

tniessen commented Jan 26, 2021

Copy link
Copy Markdown
Member Author

Thanks @richardlau, test should pass now.

nodejs-github-bot commented Jan 26, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35756/

tniessen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 26, 2021
addaleax reviewed Jan 28, 2021
View reviewed changes
Comment thread src/crypto/crypto_keys.cc Outdated
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 28, 2021

addaleax commented Jan 28, 2021

Copy link
Copy Markdown
Member

We should clarify the semantics of the tristate Maybe<bool>.

It's the same as in V8: We use Maybe<bool> for methods that either throw or succeed and don't return any other information, because Maybe<void> is unfortunately not a thing (i.e. it's always either Just(true) or Nothing<bool>).

But it matters that we do, because this is the way that the V8 API and our code use to tell programmers "this is a method that may throw a JS exception", and a plain bool return type doesn't communicate that.

tniessen force-pushed the crypto-fix-error-handling-for-toresult branch from 313002a to 10956b0 Compare April 1, 2021 15:36

This comment has been minimized.

Sign in to view
tniessen force-pushed the crypto-fix-error-handling-for-toresult branch from 10956b0 to c542548 Compare April 1, 2021 15:44

nodejs-github-bot commented Apr 1, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37088/

tniessen commented Apr 1, 2021

Copy link
Copy Markdown
Member Author

@addaleax Thank you for the explanation and sorry about the delay. I tried to fix this based on your feedback :)

addaleax approved these changes Apr 1, 2021
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM :)

tniessen commented Apr 1, 2021

Copy link
Copy Markdown
Member Author

CI is green, but GitHub is not picking it up.

tniessen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 1, 2021
src: fix error handling for CryptoJob::ToResult
feb60f8 
PR-URL: nodejs#37076
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott force-pushed the crypto-fix-error-handling-for-toresult branch from c542548 to feb60f8 Compare April 3, 2021 04:53
Trott merged commit feb60f8 into nodejs:master Apr 3, 2021

Trott commented Apr 3, 2021

Copy link
Copy Markdown
Member

Landed in feb60f8

tniessen removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 3, 2021
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
src: fix error handling for CryptoJob::ToResult
2864ee9 
PR-URL: #37076
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Apr 4, 2021
Merged
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
src: fix error handling for CryptoJob::ToResult
6ad0b6f 
PR-URL: #37076
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau mentioned this pull request Apr 7, 2021
Closed
tniessen mentioned this pull request Mar 5, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
addaleax addaleax approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL