crypto: check for OpenSSL errors when signing

[pmq20]

When calling crypto.prototype.sign, implicit errors might be injected into OpenSSL's error stack without being thrown causing spurious afterward failures, e.g. when reusing a kept-alive HTTPS connection as reported in [1].

This patch makes Sign::SignFinal check for OpenSSL errors during the signing process.

[1] node-modules/urllib#73

[fengmk2]

+1

[thefourtheye]

@pmq20 Can you please explain why this was closed?

[pmq20]

@thefourtheye I'm sorry, it's automatically triggered by an empty force-push. It's now open.

[Fishrock123]

@nodejs/crypto

[bnoordhuis]

I'm not sure if this is correct. We normally use ClearErrorOnReturn clear_error_on_return, like this:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 55c4703..2ec2c6b 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -3563,9 +3563,6 @@ SignBase::Error Sign::SignFinal(const char* key_pem,
                                 const char* passphrase,
                                 unsigned char** sig,
                                 unsigned int *sig_len) {
-  // We have to first clear all stale errors from the shared error stack
-  ERR_clear_error();
-
   if (!initialised_)
     return kSignNotInitialised;

@@ -3629,6 +3626,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,

But that breaks test/parallel/test-crypto-rsa-dsa.js with the following exception:

crypto.js:309
  var ret = this._handle.sign(toBuf(key), null, passphrase);
                         ^

Error: error:0906A065:PEM routines:PEM_do_header:bad decrypt
    at Error (native)
    at Sign.sign (crypto.js:309:26)
    at Object.<anonymous> (/home/bnoordhuis/src/v1.x/test/parallel/test-crypto-rsa-dsa.js:140:28)
    at Module._compile (module.js:430:26)
    at Object.Module._extensions..js (module.js:448:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:471:10)
    at startup (node.js:117:18)  
    at node.js:952:3

And I can't decide whether that's legitimate or not.

[indutny]

Usually we clear the error at the place of its origin instead, it is very easy to imagine that it will break other chunks of code or even user-land C++ bindings. Where does original ERR came from?

[pmq20]

@bnoordhuis Thanks for discovering this new error, let me look into it

[pmq20]

@bnoordhuis This error was found produced in PublicKeyCipher::Cipher, which is exactly another example of stale errors produced in procedure A was later found popping up in procedure B.

I thus added an error cleaner onto PublicKeyCipher::Cipher.

[pmq20]

@indutny Thanks for pointing this out and I agree with you. I have revised the commit to adopt the conventional way of cleaning errors.

[indutny]

Is ERR_peek_error() really necessary here? man page says that it can be used to obtain error code, nothing about the indication of error itself.

[pmq20]

There being an error code indicates that there is an error.

[indutny]

But in what situations does EVP_SignFinal return 1 and ERR_peek_error() is not zero?

[pmq20]

In the test provided with this PR.

[indutny]

I just did some debugging and the error itself happens in PEM_read_bio_PrivateKey. May I ask you to move the ERR_peek_error() check there along with pkey == nullptr?

A note with explanation about the weird behavior of PEM_read_bio_PrivateKey would be twice as welcome!

[pmq20]

@indutny Moving ERR_peek_error() and pkey == nullptr checks to PEM_read_bio_PrivateKey would dictate modifications to deps/openssl which is just an external dependency that we copied from other projects like deps/v8. I am not sure if a change to a dependency is desirable.

For the explanation for this test: The correct header for this 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-----. I have attached some detailed information below.

BEGIN RSA PRIVATE KEY is PKCS#1 and is just an RSA key. It is essentially just the key object from PKCS#8, but without the version or algorithm identifier in front. BEGIN PRIVATE KEY is PKCS#8 and indicates that the key type is included in the key data itself. From the link:

The unencrypted PKCS#8 encoded data starts and ends with the tags:

-----BEGIN PRIVATE KEY-----
BASE64 ENCODED DATA
-----END PRIVATE KEY-----
Within the base64 encoded data the following DER structure is present:

PrivateKeyInfo ::= SEQUENCE {
version Version,
algorithm AlgorithmIdentifier,
PrivateKey BIT STRING
}

AlgorithmIdentifier ::= SEQUENCE {
algorithm OBJECT IDENTIFIER,
parameters ANY DEFINED BY algorithm OPTIONAL
}
So for an RSA private key, the OID is 1.2.840.113549.1.1.1 and there is a RSAPrivateKey as the PrivateKey key data bitstring.
As opposed to BEGIN RSA PRIVATE KEY, which always specifies an RSA key and therefore doesn't include a key type OID. BEGIN RSA PRIVATE KEY is PKCS#1:

RSA Private Key file (PKCS#1)

The RSA private key PEM file is specific for RSA keys.

It starts and ends with the tags:

-----BEGIN RSA PRIVATE KEY-----
BASE64 ENCODED DATA
-----END RSA PRIVATE KEY-----

[indutny]

Wait, I wasn't talking about moving this check to the OpenSSL. There is PEM... call few lines above that I was talking about.

[pmq20]

@indutny Modified as instructed. Rebased to upon the latest master.

[indutny]

LGTM

[indutny]

@pmq20 one very last thing. May I ask you to put some description into commit message? You have a beautiful comment above the ERR_peek_error() that explains a lot of the problem. I just want to be able to remember what was this commit about without opening the diff ;) Thanks!

[indutny]

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/99/

[indutny]

We have some odd CI failure on Fedora: https://jenkins-iojs.nodesource.com/job/node-test-commit-linux/161/nodes=fedora21/tapTestReport/test.tap-113/ May I ask you to take a look at it?

[pmq20]

@indutny Thanks for starting CI on this patch! I have looked into the failure and it turned out that my formulated test-case certificate contained more than one error. I'll take some time to devise a cleaner new one.

[indutny]

Ouch! Interesting! I wonder why it do not show up on other platforms?

[pmq20]

It turned out that a certificate generated in this way is sufficient:

1. openssl genrsa -out mykey.pem 512
2. openssl pkcs8 -topk8 -inform PEM -outform PEM -in mykey.pem -out private_key.pem -nocrypt
3. open private_key.pem and change its header and footer

Then calling crypto.createSign('sha1').sign(sha1_privateKey); just once in the patched io.js triggers the ASN1 wrong tag error, yet not in the un-patched io.js.

[pmq20]

@indutny CI failure fixed and commit message added. Please start the CI again.

[indutny]

@pmq20 here you go: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/103/

[pmq20]

@indutny CI reported some errors, but they are probably introduced by the latest master branch onto which I rebased today in the latest revised commit. Tests on test-crypto.js shows no errors though. Should I undo the rebase or just keep them this way?

smartos14-64:

• test-debug-signal-cluster.js
  AssertionError: test timed out.

win2008r2:

• FATAL: java.io.IOException: Connection aborted: org.jenkinsci.remoting.nio.NioChannelHub$MonoNioTransport@6f946ca[name=iojs-rackspace-win2008r2-1]

armv7-wheezy:

• test-tick-processor.js
  const Transform = require('_stream_transform');
  TypeError: require is not a function
• not ok 242 - test-fs-readfile-error.js

[indutny]

Great, landing!

[indutny]

Thank you very much for your efforts, landed in 00bffa6

[tolerious]

👍

[bnoordhuis]

test-tick-processor.js # const Transform = require('_stream_transform'); # TypeError: require is not a function

For posterity, I've seen similar errors pop up recently (but infrequently) and I'm reasonably sure it's a V8 bug.

[bnoordhuis]

Added the land-on-v3.x tag. The release notes should mention that this may unearth bugs in a user's application that were previously hidden.