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: Assertation failed when createPrivateKey(PUBLIC) #29904

Closed
tespent opened this issue Oct 9, 2019 · 6 comments
Closed

crypto: Assertation failed when createPrivateKey(PUBLIC) #29904

tespent opened this issue Oct 9, 2019 · 6 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@tespent
Copy link

tespent commented Oct 9, 2019

  • Version: 12.11.1 and 12.7.0 (and more, i believe)
  • Platform: Linux dev 5.3.5-arch1-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Mon Oct 7 19:03:08 UTC 2019 x86_64 GNU/Linux

crypto.createPrivateKey raise an assertation failure when trying to import a public key.

Output of v12.11.1:

> keyPair.publicKey.export({format:'der',type:'spki'}).toString('base64')
'PUBLIC_KEY_IN_BASE64'
> pubkey=require('crypto').createPrivateKey({key:Buffer.from('PUBLIC_KEY_IN_BASE64','base64'),format:'der',type:'spki'})
node[8267]: ../src/node_crypto.cc:3299:node::crypto::ParseKeyResult node::crypto::ParsePrivateKey(node::crypto::EVPKeyPointer*, const node::crypto::PrivateKeyEncodingConfig&, const char*, size_t): Assertion `(config.type_.ToChecked()) == (kKeyEncodingSEC1)' failed.
 1: 0x9d33e0 node::Abort() [node]
 2: 0x9d3467  [node]
 3: 0xace6ea  [node]
 4: 0xadf83e  [node]
 5: 0xae03e7 node::crypto::KeyObject::Init(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 6: 0xb9ec19  [node]
 7: 0xba0a07 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 8: 0x136d639  [node]
Aborted (core dumped)

I have tried keyPair=require('crypto').generateKeyPairSync('rsa',{modulusLength:4096}) and keyPair=require('crypto').generateKeyPairSync('ec',{namedCurve:'P-256'}), the result are the same.

@addaleax addaleax added the crypto Issues and PRs related to the crypto subsystem. label Oct 9, 2019
@addaleax
Copy link
Member

addaleax commented Oct 9, 2019

@nodejs/crypto @tniessen

@sam-github
Copy link
Contributor

@tespent can you provide a runnable reproduction, I can't repro:

% node --version; node -p "require('crypto').generateKeyPairSync('ec',{namedCurve:'P-256'}).publicKey.export({format:'der',type:'spki'}).toString('base64')" 
v12.11.1
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEBEGMUwSkAYZNhIctHCL6VV/ufsVjGax0gWxBhCP9rZckKlcI1HQhIMv1qPzIS5zbFpud0YgrvZnYFkgWlJs5Ig==

@tespent
Copy link
Author

tespent commented Oct 9, 2019

@tespent can you provide a runnable reproduction, I can't repro:

% node --version; node -p "require('crypto').generateKeyPairSync('ec',{namedCurve:'P-256'}).publicKey.export({format:'der',type:'spki'}).toString('base64')" 
v12.11.1
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEBEGMUwSkAYZNhIctHCL6VV/ufsVjGax0gWxBhCP9rZckKlcI1HQhIMv1qPzIS5zbFpud0YgrvZnYFkgWlJs5Ig==

node -p "require('crypto').createPrivateKey({key:Buffer.from('MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEBEGMUwSkAYZNhIctHCL6VV/ufsVjGax0gWxBhCP9rZckKlcI1HQhIMv1qPzIS5zbFpud0YgrvZnYFkgWlJs5Ig==','base64'),format:'der',type:'spki'})"

@sam-github

@sam-github sam-github added the confirmed-bug Issues with confirmed bugs. label Oct 9, 2019
@sam-github
Copy link
Contributor

Thanks, I can repro

@tniessen tniessen self-assigned this Oct 9, 2019
@sam-github
Copy link
Contributor

Its because spki is a public key format, and it isn't handled in the private key parsing code. I'm rebuildign to see if this fixes it:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 3405fbb5b4..c2a22d0d20 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -3345,6 +3345,8 @@ static ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
         if (p8inf)
           pkey->reset(EVP_PKCS82PKEY(p8inf.get()));
       }
+    } else if (config.type_.ToChecked() == kKeyEncodingSPKI) {
+      return ParseKeyResult::kParseKeyFailed;
     } else {
       CHECK_EQ(config.type_.ToChecked(), kKeyEncodingSEC1);
       const unsigned char* p = reinterpret_cast<const unsigned char*>(key);

tniessen added a commit to tniessen/node that referenced this issue Oct 10, 2019
@tniessen
Copy link
Member

Right, but I believe execution shouldn't even get to ParsePrivateKey in this case. Possible fix in #29913.

@Trott Trott closed this as completed in c64ed10 Oct 12, 2019
targos pushed a commit that referenced this issue 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 issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants