crypto: fix Node_SignFinal

PR nodejs#11705 switched Node away from using using OpenSSL's legacy EVP_Sign*
and EVP_Verify* APIs. Instead, it computes a hash normally via
EVP_Digest* and then uses EVP_PKEY_sign and EVP_PKEY_verify to verify
the hash directly. This change corrects two problems:

1. The documentation still recommends the signature algorithm EVP_MD
   names of OpenSSL's legacy APIs. OpenSSL has since moved away from
   thosee, which is why ECDSA was strangely inconsistent. (This is why
   "ecdsa-with-SHA256" was missing.)

2. Node_SignFinal copied some code from EVP_SignFinal's internals. This
   is problematic for OpenSSL 1.1.0 and is missing a critical check
   that prevents pkey->pkey.ptr from being cast to the wrong type.

To resolve this, remove the non-EVP_PKEY_sign codepath. This codepath is
no longer necessary. PR nodejs#11705's verify half was already assuming all
EVP_PKEYs supported EVP_PKEY_sign and EVP_PKEY_verify. Also, in the
documentation, point users towards using hash function names which are
more consisent. This avoids an ECDSA special-case and some strangeness
around RSA-PSS ("RSA-SHA256" is the OpenSSL name of the
sha256WithRSAEncryption OID which is not used for RSA-PSS).

PR-URL: nodejs#15024
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Author: davidben

benchmark/crypto/rsa-sign-verify-throughput.js

@@ -18,7 +18,7 @@ keylen_list.forEach(function(key) {
 
 var bench = common.createBenchmark(main, {
   writes: [500],
-  algo: ['RSA-SHA1', 'RSA-SHA224', 'RSA-SHA256', 'RSA-SHA384', 'RSA-SHA512'],
+  algo: ['SHA1', 'SHA224', 'SHA256', 'SHA384', 'SHA512'],
   keylen: keylen_list,
   len: [1024, 102400, 2 * 102400, 3 * 102400, 1024 * 1024]
 });

doc/api/crypto.md

@@ -918,28 +918,31 @@ of two ways:
 - Using the [`sign.update()`][] and [`sign.sign()`][] methods to produce the
   signature.
 
-The [`crypto.createSign()`][] method is used to create `Sign` instances. `Sign`
-objects are not to be created directly using the `new` keyword.
+The [`crypto.createSign()`][] method is used to create `Sign` instances. The
+argument is the string name of the hash function to use. `Sign` objects are not
+to be created directly using the `new` keyword.
 
 Example: Using `Sign` objects as streams:
 
 ```js
 const crypto = require('crypto');
-const sign = crypto.createSign('RSA-SHA256');
+const sign = crypto.createSign('SHA256');
 
 sign.write('some data to sign');
 sign.end();
 
 const privateKey = getPrivateKeySomehow();
 console.log(sign.sign(privateKey, 'hex'));
-// Prints: the calculated signature
+// Prints: the calculated signature using the specified private key and
+// SHA-256. For RSA keys, the algorithm is RSASSA-PKCS1-v1_5 (see padding
+// parameter below for RSASSA-PSS). For EC keys, the algorithm is ECDSA.
 ```
 
 Example: Using the [`sign.update()`][] and [`sign.sign()`][] methods:
 
 ```js
 const crypto = require('crypto');
-const sign = crypto.createSign('RSA-SHA256');
+const sign = crypto.createSign('SHA256');
 
 sign.update('some data to sign');
 
@@ -948,27 +951,22 @@ console.log(sign.sign(privateKey, 'hex'));
 // Prints: the calculated signature
 ```
 
-A `Sign` instance can also be created by just passing in the digest
-algorithm name, in which case OpenSSL will infer the full signature algorithm
-from the type of the PEM-formatted private key, including algorithms that
-do not have directly exposed name constants, e.g. 'ecdsa-with-SHA256'.
+In some cases, a `Sign` instance can also be created by passing in a signature
+algorithm name, such as 'RSA-SHA256'. This will use the corresponding digest
+algorithm. This does not work for all signature algorithms, such as
+'ecdsa-with-SHA256'. Use digest names instead.
 
-Example: signing using ECDSA with SHA256
+Example: signing using legacy signature algorithm name
 
 ```js
 const crypto = require('crypto');
-const sign = crypto.createSign('sha256');
+const sign = crypto.createSign('RSA-SHA256');
 
 sign.update('some data to sign');
 
-const privateKey =
-`-----BEGIN EC PRIVATE KEY-----
-MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49
-AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2
-pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==
------END EC PRIVATE KEY-----`;
-
-console.log(sign.sign(privateKey).toString('hex'));
+const privateKey = getPrivateKeySomehow();
+console.log(sign.sign(privateKey, 'hex'));
+// Prints: the calculated signature
 ```
 
 ### sign.sign(privateKey[, outputFormat])
@@ -1051,7 +1049,7 @@ Example: Using `Verify` objects as streams:
 
 ```js
 const crypto = require('crypto');
-const verify = crypto.createVerify('RSA-SHA256');
+const verify = crypto.createVerify('SHA256');
 
 verify.write('some data to sign');
 verify.end();
@@ -1066,7 +1064,7 @@ Example: Using the [`verify.update()`][] and [`verify.verify()`][] methods:
 
 ```js
 const crypto = require('crypto');
-const verify = crypto.createVerify('RSA-SHA256');
+const verify = crypto.createVerify('SHA256');
 
 verify.update('some data to sign');
 

src/node_crypto.cc

@@ -3981,7 +3981,8 @@ void SignBase::CheckThrow(SignBase::Error error) {
 
 static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding,
                             int salt_len) {
-  if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
+  if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA ||
+      EVP_PKEY_id(pkey) == EVP_PKEY_RSA2) {
     if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
       return false;
     if (padding == RSA_PKCS1_PSS_PADDING) {
@@ -4090,33 +4091,23 @@ static int Node_SignFinal(EVP_MD_CTX* mdctx, unsigned char* md,
   if (!EVP_DigestFinal_ex(mdctx, m, &m_len))
     return rv;
 
-  if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
-    size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
-    pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
-    if (pkctx == nullptr)
-      goto err;
-    if (EVP_PKEY_sign_init(pkctx) <= 0)
-      goto err;
-    if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
-      goto err;
-    if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx->digest) <= 0)
-      goto err;
-    if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
-      goto err;
-    *sig_len = sltmp;
-    rv = 1;
-   err:
-    EVP_PKEY_CTX_free(pkctx);
-    return rv;
-  }
-
-  if (mdctx->digest->sign == nullptr) {
-    EVPerr(EVP_F_EVP_SIGNFINAL, EVP_R_NO_SIGN_FUNCTION_CONFIGURED);
-    return 0;
-  }
-
-  return mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,
-                             pkey->pkey.ptr);
+  size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
+  pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
+  if (pkctx == nullptr)
+    goto err;
+  if (EVP_PKEY_sign_init(pkctx) <= 0)
+    goto err;
+  if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
+    goto err;
+  if (EVP_PKEY_CTX_set_signature_md(pkctx, EVP_MD_CTX_md(mdctx)) <= 0)
+    goto err;
+  if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
+    goto err;
+  *sig_len = sltmp;
+  rv = 1;
+ err:
+  EVP_PKEY_CTX_free(pkctx);
+  return rv;
 }
 
 SignBase::Error Sign::SignFinal(const char* key_pem,

test/fixtures/0-dns/create-cert.js

@@ -8,7 +8,7 @@ const BN = asn1.bignum;
 const id_at_commonName = [ 2, 5, 4, 3 ];
 const rsaEncryption = [1, 2, 840, 113549, 1, 1, 1];
 const sha256WithRSAEncryption = [1, 2, 840, 113549, 1, 1, 11];
-const sigalg = 'RSA-SHA256';
+const digest = 'SHA256';
 
 const private_key = fs.readFileSync('./0-dns-key.pem');
 // public key file can be generated from the private key with
@@ -59,7 +59,7 @@ const tbs = {
 
 const tbs_der = rfc5280.TBSCertificate.encode(tbs, 'der');
 
-const sign = crypto.createSign(sigalg);
+const sign = crypto.createSign(digest);
 sign.update(tbs_der);
 const signature = sign.sign(private_key);
 

test/parallel/test-crypto-binary-default.js

@@ -424,28 +424,28 @@ assert.throws(function() {
 }, /^Error: Digest method not supported$/);
 
 // Test signing and verifying
-const s1 = crypto.createSign('RSA-SHA1')
+const s1 = crypto.createSign('SHA1')
                .update('Test123')
                .sign(keyPem, 'base64');
-const s1Verified = crypto.createVerify('RSA-SHA1')
+const s1Verified = crypto.createVerify('SHA1')
                        .update('Test')
                        .update('123')
                        .verify(certPem, s1, 'base64');
 assert.strictEqual(s1Verified, true, 'sign and verify (base 64)');
 
-const s2 = crypto.createSign('RSA-SHA256')
+const s2 = crypto.createSign('SHA256')
                .update('Test123')
                .sign(keyPem); // binary
-const s2Verified = crypto.createVerify('RSA-SHA256')
+const s2Verified = crypto.createVerify('SHA256')
                        .update('Test')
                        .update('123')
                        .verify(certPem, s2); // binary
 assert.strictEqual(s2Verified, true, 'sign and verify (binary)');
 
-const s3 = crypto.createSign('RSA-SHA1')
+const s3 = crypto.createSign('SHA1')
                .update('Test123')
                .sign(keyPem, 'buffer');
-const s3Verified = crypto.createVerify('RSA-SHA1')
+const s3Verified = crypto.createVerify('SHA1')
                        .update('Test')
                        .update('123')
                        .verify(certPem, s3);
@@ -610,8 +610,8 @@ const d = crypto.createDiffieHellman(p, 'hex');
 assert.strictEqual(d.verifyError, DH_NOT_SUITABLE_GENERATOR);
 
 // Test RSA key signing/verification
-const rsaSign = crypto.createSign('RSA-SHA1');
-const rsaVerify = crypto.createVerify('RSA-SHA1');
+const rsaSign = crypto.createSign('SHA1');
+const rsaVerify = crypto.createVerify('SHA1');
 assert.ok(rsaSign instanceof crypto.Sign);
 assert.ok(rsaVerify instanceof crypto.Verify);
 
@@ -646,13 +646,13 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
       '8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
       '0ddfb299bedeb1ad';
 
-  const sign = crypto.createSign('RSA-SHA256');
+  const sign = crypto.createSign('SHA256');
   sign.update(input);
 
   const output = sign.sign(privateKey, 'hex');
   assert.strictEqual(output, signature);
 
-  const verify = crypto.createVerify('RSA-SHA256');
+  const verify = crypto.createVerify('SHA256');
   verify.update(input);
 
   assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
@@ -670,11 +670,11 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
 
   // DSA signatures vary across runs so there is no static string to verify
   // against
-  const sign = crypto.createSign('DSS1');
+  const sign = crypto.createSign('SHA1');
   sign.update(input);
   const signature = sign.sign(privateKey, 'hex');
 
-  const verify = crypto.createVerify('DSS1');
+  const verify = crypto.createVerify('SHA1');
   verify.update(input);
 
   assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);

test/parallel/test-crypto-rsa-dsa.js

@@ -131,8 +131,8 @@ test_rsa('RSA_PKCS1_PADDING');
 test_rsa('RSA_PKCS1_OAEP_PADDING');
 
 // Test RSA key signing/verification
-let rsaSign = crypto.createSign('RSA-SHA1');
-let rsaVerify = crypto.createVerify('RSA-SHA1');
+let rsaSign = crypto.createSign('SHA1');
+let rsaVerify = crypto.createVerify('SHA1');
 assert.ok(rsaSign);
 assert.ok(rsaVerify);
 
@@ -151,19 +151,19 @@ rsaVerify.update(rsaPubPem);
 assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
 
 // Test RSA key signing/verification with encrypted key
-rsaSign = crypto.createSign('RSA-SHA1');
+rsaSign = crypto.createSign('SHA1');
 rsaSign.update(rsaPubPem);
 assert.doesNotThrow(() => {
   const signOptions = { key: rsaKeyPemEncrypted, passphrase: 'password' };
   rsaSignature = rsaSign.sign(signOptions, 'hex');
 });
 assert.strictEqual(rsaSignature, expectedSignature);
 
-rsaVerify = crypto.createVerify('RSA-SHA1');
+rsaVerify = crypto.createVerify('SHA1');
 rsaVerify.update(rsaPubPem);
 assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
 
-rsaSign = crypto.createSign('RSA-SHA1');
+rsaSign = crypto.createSign('SHA1');
 rsaSign.update(rsaPubPem);
 assert.throws(() => {
   const signOptions = { key: rsaKeyPemEncrypted, passphrase: 'wrong' };
@@ -186,16 +186,28 @@ assert.throws(() => {
       '8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
       '0ddfb299bedeb1ad';
 
-  const sign = crypto.createSign('RSA-SHA256');
+  const sign = crypto.createSign('SHA256');
   sign.update(input);
 
   const output = sign.sign(privateKey, 'hex');
   assert.strictEqual(signature, output);
 
-  const verify = crypto.createVerify('RSA-SHA256');
+  const verify = crypto.createVerify('SHA256');
   verify.update(input);
 
   assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
+
+  // Test the legacy signature algorithm name.
+  const sign2 = crypto.createSign('RSA-SHA256');
+  sign2.update(input);
+
+  const output2 = sign2.sign(privateKey, 'hex');
+  assert.strictEqual(signature, output2);
+
+  const verify2 = crypto.createVerify('SHA256');
+  verify2.update(input);
+
+  assert.strictEqual(verify2.verify(publicKey, signature, 'hex'), true);
 }
 
 
@@ -207,14 +219,24 @@ assert.throws(() => {
 
   // DSA signatures vary across runs so there is no static string to verify
   // against
-  const sign = crypto.createSign('DSS1');
+  const sign = crypto.createSign('SHA1');
   sign.update(input);
   const signature = sign.sign(dsaKeyPem, 'hex');
 
-  const verify = crypto.createVerify('DSS1');
+  const verify = crypto.createVerify('SHA1');
   verify.update(input);
 
   assert.strictEqual(verify.verify(dsaPubPem, signature, 'hex'), true);
+
+  // Test the legacy 'DSS1' name.
+  const sign2 = crypto.createSign('DSS1');
+  sign2.update(input);
+  const signature2 = sign2.sign(dsaKeyPem, 'hex');
+
+  const verify2 = crypto.createVerify('DSS1');
+  verify2.update(input);
+
+  assert.strictEqual(verify2.verify(dsaPubPem, signature2, 'hex'), true);
 }
 
 
@@ -224,7 +246,7 @@ assert.throws(() => {
 const input = 'I AM THE WALRUS';
 
 {
-  const sign = crypto.createSign('DSS1');
+  const sign = crypto.createSign('SHA1');
   sign.update(input);
   assert.throws(() => {
     sign.sign({ key: dsaKeyPemEncrypted, passphrase: 'wrong' }, 'hex');
@@ -234,7 +256,7 @@ const input = 'I AM THE WALRUS';
 {
   // DSA signatures vary across runs so there is no static string to verify
   // against
-  const sign = crypto.createSign('DSS1');
+  const sign = crypto.createSign('SHA1');
   sign.update(input);
 
   let signature;
@@ -243,7 +265,7 @@ const input = 'I AM THE WALRUS';
     signature = sign.sign(signOptions, 'hex');
   });
 
-  const verify = crypto.createVerify('DSS1');
+  const verify = crypto.createVerify('SHA1');
   verify.update(input);
 
   assert.strictEqual(verify.verify(dsaPubPem, signature, 'hex'), true);