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

crypto: reject public keys properly #29913

Closed
wants to merge 2 commits into from
Closed

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Oct 10, 2019

If the underlying operation requires a private key, isPublic must be set to false instead of undefined. (The latter means that both public and private keys are accepted.)

Fixes: #29904

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Oct 10, 2019

// This should not abort either: https://github.com/nodejs/node/issues/29904
assert.throws(() => {
createPrivateKey({ key: Buffer.alloc(0), format: 'der', type: 'spki' });
Copy link
Contributor

Choose a reason for hiding this comment

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

There are other formats for public keys, perhaps its worth testing them all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the only other supported type is pkcs1 which behaves differently, but I added a test case for that as well.

@sam-github
Copy link
Contributor

CHECK_EQ(config.type_.ToChecked(), kKeyEncodingSEC1);
I think is still a problem, its not necessarily true, the conditionals don't handle new values being added to the enum PKEncodingType. I suggest changing the if..if else ..else to a switch(config.type_.ToChecked()) { case .... with either a default that returns ParseKeyResult::kParseKeyFailed, or probably better, no default. I think gcc will warn when a switch doesn't handle all values of an enum in its case statements, so the code will be more future proof. Just a suggestion.

@tniessen
Copy link
Member Author

I understand your point. My main problem with a switch without a default case is that relying on a compiler warning seems dangerous to me, the process would not abort anymore.

In my opinion, the current behavior of ParsePrivateKey is correct, it handles all valid values (PKCS#1, PKCS#8, SEC1) and aborts on invalid values (SPKI). The only problem is that we won't notice the possibility of passing invalid values to CreatePrivateKey without running into the situation (which is exactly what happened in #29904), but that wouldn't be fixed by a switch that handles those cases.

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

Aborts at the js-to-c++ layer make a lot of sense to me, where its easy to add type check calls in js just before calling C++ it should be very difficult to reach those abort arg checks.

But since users are managing to get unexpected things to happen multiple layers down in the C++, then actual error returns make more sense. I don't feel strongly about it, but I notice that changing the fallback in the code in question to an error return, see #29904 (comment), prevented the abort. The user would still get an error, of course, but that was already possible for reasons of malformed input. That was exploratory code, I'm not suggesting using it as-is, but having the return be the default/final-else case would make the code more robust.

@tniessen
Copy link
Member Author

But since users are managing to get unexpected things to happen multiple layers down in the C++, then actual error returns make more sense. I don't feel strongly about it, but I notice that changing the fallback in the code in question to an error return, see #29904 (comment), prevented the abort. The user would still get an error, of course, but that was already possible for reasons of malformed input. That was exploratory code, I'm not suggesting using it as-is, but having the return be the default/final-else case would make the code more robust.

It would be more robust in the sense that the process would not abort, but what kind of error should we throw here? Reaching this point is definitely a bug, and an error code that indicates otherwise will only be misleading. So the error should very clearly say: "Hey, this is a bug in node!". However, if we just throw the error, it might get lost, whereas aborting the process will send a very clear message.

@Trott
Copy link
Member

Trott commented Oct 12, 2019

Landed in c64ed10

@Trott Trott closed this Oct 12, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 12, 2019
Fixes: nodejs#29904

PR-URL: nodejs#29913
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Nov 8, 2019
Fixes: #29904

PR-URL: #29913
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Nov 10, 2019
Fixes: #29904

PR-URL: #29913
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@tniessen tniessen deleted the fix-29904 branch January 23, 2020 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto: Assertation failed when createPrivateKey(PUBLIC)
7 participants