crypto: fix Node_SignFinal

PR #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 #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: https://github.com/nodejs/node/pull/15024
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

@@ -842,28 +842,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');
 
@@ -872,27 +875,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(private_key[, output_format])
@@ -965,7 +963,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();
@@ -980,7 +978,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

@@ -3993,7 +3993,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) {
@@ -4102,33 +4103,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

@@ -404,28 +404,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);
@@ -569,8 +569,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);
 
@@ -606,13 +606,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);
@@ -631,11 +631,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

@@ -132,8 +132,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);
 
@@ -152,7 +152,7 @@ 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' };
@@ -160,11 +160,11 @@ assert.doesNotThrow(() => {
 });
 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' };
@@ -188,16 +188,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);
 }
 
 
@@ -209,14 +221,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);
 }
 
 
@@ -226,7 +248,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');
@@ -236,7 +258,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;
@@ -245,7 +267,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);

test/parallel/test-crypto-sign-verify.js

@@ -21,15 +21,15 @@ const modSize = 1024;
 
 // Test signing and verifying
 {
-  const s1 = crypto.createSign('RSA-SHA1')
+  const s1 = crypto.createSign('SHA1')
                    .update('Test123')
                    .sign(keyPem, 'base64');
-  let s1stream = crypto.createSign('RSA-SHA1');
+  let s1stream = crypto.createSign('SHA1');
   s1stream.end('Test123');
   s1stream = s1stream.sign(keyPem, 'base64');
   assert.strictEqual(s1, s1stream, 'Stream produces same output');
 
-  const verified = crypto.createVerify('RSA-SHA1')
+  const verified = crypto.createVerify('SHA1')
                          .update('Test')
                          .update('123')
                          .verify(certPem, s1, 'base64');
@@ -37,21 +37,21 @@ const modSize = 1024;
 }
 
 {
-  const s2 = crypto.createSign('RSA-SHA256')
+  const s2 = crypto.createSign('SHA256')
                    .update('Test123')
                    .sign(keyPem, 'latin1');
-  let s2stream = crypto.createSign('RSA-SHA256');
+  let s2stream = crypto.createSign('SHA256');
   s2stream.end('Test123');
   s2stream = s2stream.sign(keyPem, 'latin1');
   assert.strictEqual(s2, s2stream, 'Stream produces same output');
 
-  let verified = crypto.createVerify('RSA-SHA256')
+  let verified = crypto.createVerify('SHA256')
                        .update('Test')
                        .update('123')
                        .verify(certPem, s2, 'latin1');
   assert.strictEqual(verified, true, 'sign and verify (latin1)');
 
-  const verStream = crypto.createVerify('RSA-SHA256');
+  const verStream = crypto.createVerify('SHA256');
   verStream.write('Tes');
   verStream.write('t12');
   verStream.end('3');
@@ -60,16 +60,16 @@ const modSize = 1024;
 }
 
 {
-  const s3 = crypto.createSign('RSA-SHA1')
+  const s3 = crypto.createSign('SHA1')
                    .update('Test123')
                    .sign(keyPem, 'buffer');
-  let verified = crypto.createVerify('RSA-SHA1')
+  let verified = crypto.createVerify('SHA1')
                        .update('Test')
                        .update('123')
                        .verify(certPem, s3);
   assert.strictEqual(verified, true, 'sign and verify (buffer)');
 
-  const verStream = crypto.createVerify('RSA-SHA1');
+  const verStream = crypto.createVerify('SHA1');
   verStream.write('Tes');
   verStream.write('t12');
   verStream.end('3');
@@ -170,8 +170,8 @@ const modSize = 1024;
     });
   }
 
-  testPSS('RSA-SHA1', 20);
-  testPSS('RSA-SHA256', 32);
+  testPSS('SHA1', 20);
+  testPSS('SHA256', 32);
 }
 
 // Test vectors for RSA_PKCS1_PSS_PADDING provided by the RSA Laboratories:
@@ -179,7 +179,7 @@ const modSize = 1024;
 {
   // We only test verification as we cannot specify explicit salts when signing
   function testVerify(cert, vector) {
-    const verified = crypto.createVerify('RSA-SHA1')
+    const verified = crypto.createVerify('SHA1')
                           .update(Buffer.from(vector.message, 'hex'))
                           .verify({
                             key: cert,
@@ -206,7 +206,7 @@ const modSize = 1024;
   [null, undefined, NaN, 'boom', {}, [], true, false]
     .forEach((invalidValue) => {
       assert.throws(() => {
-        crypto.createSign('RSA-SHA256')
+        crypto.createSign('SHA256')
           .update('Test123')
           .sign({
             key: keyPem,
@@ -215,7 +215,7 @@ const modSize = 1024;
       }, /^TypeError: padding must be an integer$/);
 
       assert.throws(() => {
-        crypto.createSign('RSA-SHA256')
+        crypto.createSign('SHA256')
           .update('Test123')
           .sign({
             key: keyPem,
@@ -226,7 +226,7 @@ const modSize = 1024;
     });
 
   assert.throws(() => {
-    crypto.createSign('RSA-SHA1')
+    crypto.createSign('SHA1')
       .update('Test123')
       .sign({
         key: keyPem,
@@ -238,7 +238,7 @@ const modSize = 1024;
 // Test throws exception when key options is null
 {
   assert.throws(() => {
-    crypto.createSign('RSA-SHA1').update('Test123').sign(null, 'base64');
+    crypto.createSign('SHA1').update('Test123').sign(null, 'base64');
   }, /^Error: No key provided to sign$/);
 }
 
@@ -254,7 +254,7 @@ const modSize = 1024;
   const privkey = fs.readFileSync(privfile);
 
   const msg = 'Test123';
-  const s5 = crypto.createSign('RSA-SHA256')
+  const s5 = crypto.createSign('SHA256')
     .update(msg)
     .sign({
       key: privkey,

test/parallel/test-crypto-verify-failure.js

@@ -28,7 +28,7 @@ const server = tls.Server(options, (socket) => {
 });
 
 function verify() {
-  crypto.createVerify('RSA-SHA1')
+  crypto.createVerify('SHA1')
     .update('Test')
     .verify(certPem, 'asdfasdfas', 'base64');
 }

test/parallel/test-crypto.js

@@ -125,11 +125,11 @@ assert.throws(function() {
 }, /^TypeError: Bad input string$/);
 
 assert.throws(function() {
-  crypto.createSign('RSA-SHA1').update('0', 'hex');
+  crypto.createSign('SHA1').update('0', 'hex');
 }, /^TypeError: Bad input string$/);
 
 assert.throws(function() {
-  crypto.createVerify('RSA-SHA1').update('0', 'hex');
+  crypto.createVerify('SHA1').update('0', 'hex');
 }, /^TypeError: Bad input string$/);
 
 assert.throws(function() {
@@ -142,7 +142,7 @@ assert.throws(function() {
     '-----END RSA PRIVATE KEY-----',
     ''
   ].join('\n');
-  crypto.createSign('RSA-SHA256').update('test').sign(priv);
+  crypto.createSign('SHA256').update('test').sign(priv);
 }, /digest too big for rsa key$/);
 
 assert.throws(function() {

test/parallel/test-dsa-fips-invalid-key.js

@@ -11,7 +11,7 @@ const input = 'hello';
 
 const dsapri = fs.readFileSync(
   `${common.fixturesDir}/keys/dsa_private_1025.pem`);
-const sign = crypto.createSign('DSS1');
+const sign = crypto.createSign('SHA1');
 sign.update(input);
 
 assert.throws(function() {