crypto: check for OpenSSL errors when signing

Errors might be injected into OpenSSL's error stack without the return value of `PEM_read_bio_PrivateKey` being set to `nullptr`. See the test of `test_bad_rsa_privkey.pem` for an example. PR-URL: #2342 Reviewed-By: Fedor Indutny <[email protected]>
nodejs · Aug 19, 2015 · 7fe6dd8 · 7fe6dd8
1 parent cbb75c4
commit 7fe6dd8
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 1 deletion.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -3583,7 +3583,11 @@ SignBase::Error Sign::SignFinal(const char* key_pem,
                                  nullptr,
                                  CryptoPemCallback,
                                  const_cast<char*>(passphrase));
-  if (pkey == nullptr)
+
+  // Errors might be injected into OpenSSL's error stack
+  // without `pkey` being set to nullptr;
+  // cf. the test of `test_bad_rsa_privkey.pem` for an example.
+  if (pkey == nullptr || 0 != ERR_peek_error())
     goto exit;
 
   if (EVP_SignFinal(&mdctx_, *sig, sig_len, pkey))
@@ -3631,6 +3635,9 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) {
   md_len = 8192;  // Maximum key size is 8192 bits
   md_value = new unsigned char[md_len];
 
+  ClearErrorOnReturn clear_error_on_return;
+  (void) &clear_error_on_return;  // Silence compiler warning.
+
   Error err = sign->SignFinal(
       buf,
       buf_len,
@@ -3965,6 +3972,9 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
   unsigned char* out_value = nullptr;
   size_t out_len = 0;
 
+  ClearErrorOnReturn clear_error_on_return;
+  (void) &clear_error_on_return;  // Silence compiler warning.
+
   bool r = Cipher<operation, EVP_PKEY_cipher_init, EVP_PKEY_cipher>(
       kbuf,
       klen,

diff --git a/test/fixtures/test_bad_rsa_privkey.pem b/test/fixtures/test_bad_rsa_privkey.pem
@@ -0,0 +1,10 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIBUwIBADANBgkqhkiG9w0BAQEFAASCAT0wggE5AgEAAkEAz0ZHmXyxQSdWk6NF
+GRotTax0O94iHv843su0mOynV9QLvlAwMrUk9k4+/SwyLu0eE3iYsYgXstXi3t2u
+rDSIMwIDAQABAkAH4ag/Udp7m79TBdZOygwG9BPHYv7xJstGzYAkgHssf7Yd5ZuC
+hpKtBvWdPXZaAFbwF8NSisMl98Q/9zgB/q5BAiEA5zXuwMnwt4hE2YqzBDRFB4g9
+I+v+l1soy6x7Wdqo9esCIQDlf15qDb26uRDurBioE3IpZstWIIvLDdKqviZXKMs8
+2QIgWeC5QvA9RtsOCJLGLCg1fUwUmFYwzZ1+Kk6OVMuPSqkCIDIWFSXyL8kzoKVm
+O89axxyQCaqXWcsMDkEjVLzK82gpAiB7lzdDHr7MoMWwV2wC/heEFC2p0Rw4wg9j
+1V8QbL0Q0A==
+-----END RSA PRIVATE KEY-----
diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js
@@ -124,5 +124,21 @@ assert.throws(function() {
   crypto.createSign('RSA-SHA256').update('test').sign(priv);
 }, /RSA_sign:digest too big for rsa key/);
 
+assert.throws(function() {
+  // The correct header inside `test_bad_rsa_privkey.pem` should have been
+  // -----BEGIN PRIVATE KEY----- and -----END PRIVATE KEY-----
+  // instead of
+  // -----BEGIN RSA PRIVATE KEY----- and -----END RSA PRIVATE KEY-----
+  // It is generated in this way:
+  //   $ openssl genrsa -out mykey.pem 512;
+  //   $ openssl pkcs8 -topk8 -inform PEM -outform PEM -in mykey.pem \
+  //     -out private_key.pem -nocrypt;
+  //   Then open private_key.pem and change its header and footer.
+  var sha1_privateKey = fs.readFileSync(common.fixturesDir +
+                                        '/test_bad_rsa_privkey.pem', 'ascii');
+  // this would inject errors onto OpenSSL's error stack
+  crypto.createSign('sha1').sign(sha1_privateKey);
+}, /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/);
+
 // Make sure memory isn't released before being returned
 console.log(crypto.randomBytes(16));