From 7fe6dd8f5d60b0a403abe6f94d8e60974c359ea7 Mon Sep 17 00:00:00 2001 From: "P.S.V.R" Date: Tue, 18 Aug 2015 10:32:21 +0800 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/2342 Reviewed-By: Fedor Indutny --- src/node_crypto.cc | 12 +++++++++++- test/fixtures/test_bad_rsa_privkey.pem | 10 ++++++++++ test/parallel/test-crypto.js | 16 ++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/test_bad_rsa_privkey.pem diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c95e0fa31d787f..1af462490f096b 100644 --- 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(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& 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& 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( kbuf, klen, diff --git a/test/fixtures/test_bad_rsa_privkey.pem b/test/fixtures/test_bad_rsa_privkey.pem new file mode 100644 index 00000000000000..cc84a6fc6d67de --- /dev/null +++ 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 index 55b57e65154700..57191b24ae351a 100644 --- 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));