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.

Sign up for GitHub

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

Jump to bottom

fix private key export without cipher crash #27041

Closed
wants to merge 3 commits into from
Closed

fix private key export without cipher crash #27041

wants to merge 3 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Apr 1, 2019 •
edited
Loading

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

Fixes the following by type checking the cipher before sending of to node_crypto.cc

const { generateKeyPairSync } = require('crypto')
const { privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 })
privateKey.export({ format: 'pem', type: 'pkcs8', passphrase: 'super-secret' })
node[13162]: ../src/node_crypto.cc:3096:NonCopyableMaybe<node::crypto::PrivateKeyEncodingConfig> node::crypto::GetPrivateKeyEncodingFromJs(const FunctionCallbackInfo<v8::Value> &, unsigned int *, node::crypto::KeyEncodingContext): Assertion `!(context != kKeyContextInput) || (result.cipher_ != nullptr)' failed.
 1: 0x10006778e node::Abort() [/.nvm/versions/node/v11.13.0/bin/node]
 2: 0x1000676c0 node::PrintErrorString(char const*, ...) [/.nvm/versions/node/v11.13.0/bin/node]
 3: 0x10011f2d8 node::crypto::KeyObject::ExportPrivateKey(node::crypto::PrivateKeyEncodingConfig const&) const [/.nvm/versions/node/v11.13.0/bin/node]
 4: 0x10011e16f node::crypto::KeyObject::Export(v8::FunctionCallbackInfo<v8::Value> const&) [/.nvm/versions/node/v11.13.0/bin/node]
 5: 0x10023f927 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [/.nvm/versions/node/v11.13.0/bin/node]
 6: 0x10023eef6 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/.nvm/versions/node/v11.13.0/bin/node]
 7: 0x10023e5f0 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/.nvm/versions/node/v11.13.0/bin/node]
 8: 0xd92061cfc7d 
 9: 0xd920618e458 
10: 0xd920618e458 
11: 0xd920618ba89 
[1]    13162 abort      node

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Apr 1, 2019
@panva
Copy link
Member Author

panva commented Apr 1, 2019

cc @tniessen

@tniessen tniessen added the confirmed-bug Issues with confirmed bugs. label Apr 1, 2019
bnoordhuis
bnoordhuis approved these changes Apr 2, 2019
View reviewed changes
lib/internal/crypto/keys.js Outdated Show resolved Hide resolved
@panva
Copy link
Member Author

panva commented Apr 2, 2019

@bnoordhuis @BridgeAR done

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR
Copy link
Member

BridgeAR commented Apr 4, 2019

Landed in b2bb6c2 🎉

@BridgeAR BridgeAR closed this Apr 4, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 4, 2019
@panva @BridgeAR
crypto: fix crash of encrypted private key export without cipher
b2bb6c2 
PR-URL: nodejs#27041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@panva @BethGriggs
crypto: fix crash of encrypted private key export without cipher
1d2f4c4 
PR-URL: #27041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@panva @BethGriggs
crypto: fix crash of encrypted private key export without cipher
2b791d8 
PR-URL: #27041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
@panva @BethGriggs
crypto: fix crash of encrypted private key export without cipher
a71e88b 
PR-URL: #27041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
@panva @BethGriggs
crypto: fix crash of encrypted private key export without cipher
066cd60 
PR-URL: #27041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
@panva @BethGriggs
crypto: fix crash of encrypted private key export without cipher
a00d840 
PR-URL: #27041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. confirmed-bug Issues with confirmed bugs. 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.
5 participants
@panva @nodejs-github-bot @BridgeAR @bnoordhuis @tniessen