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

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

Merged

Conversation

tniessen
Copy link
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 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 tniessen requested a review from jasnell January 26, 2021 14:12
@nodejs-github-bot 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 tniessen removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 26, 2021
@jasnell
Copy link
Member

jasnell commented Jan 26, 2021

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.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

@nodejs-github-bot

This comment has been minimized.

@tniessen
Copy link
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.)

@nodejs-github-bot

This comment has been minimized.

@tniessen tniessen force-pushed the crypto-fix-error-handling-for-toresult branch from 49a1a79 to 313002a Compare January 26, 2021 15:30
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 26, 2021

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

@richardlau
Copy link
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
Copy link
Member Author

Thanks @richardlau, test should pass now.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2021
src/crypto/crypto_keys.cc Outdated Show resolved Hide resolved
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2021
@addaleax
Copy link
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.

@nodejs-github-bot

This comment has been minimized.

@tniessen tniessen force-pushed the crypto-fix-error-handling-for-toresult branch from 10956b0 to c542548 Compare April 1, 2021 15:44
@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

tniessen commented Apr 1, 2021

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM :)

@tniessen
Copy link
Member Author

tniessen commented Apr 1, 2021

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

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2021
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 Trott force-pushed the crypto-fix-error-handling-for-toresult branch from c542548 to feb60f8 Compare April 3, 2021 04:53
@Trott Trott merged commit feb60f8 into nodejs:master Apr 3, 2021
@Trott
Copy link
Member

Trott commented Apr 3, 2021

Landed in feb60f8

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2021
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
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 MylesBorins mentioned this pull request Apr 4, 2021
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants