From b5816cddbc7fd9ae1f676916cab5e80996b5ed3c Mon Sep 17 00:00:00 2001
From: Filip Skokan <panva.ip@gmail.com>
Date: Fri, 20 Aug 2021 18:57:31 +0200
Subject: [PATCH] crypto: fix rsa-pss one-shot sign/verify error handling

fixes #39822

PR-URL: https://github.com/nodejs/node/pull/39830
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/crypto/crypto_sig.cc                 |   3 +
 test/parallel/test-crypto-sign-verify.js | 111 +++++++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc
index 7b113a8dcb06b0..7846df17ffbe8b 100644
--- a/src/crypto/crypto_sig.cc
+++ b/src/crypto/crypto_sig.cc
@@ -686,6 +686,7 @@ bool SignTraits::DeriveBits(
               nullptr,
               params.key.get())) {
         crypto::CheckThrow(env, SignBase::Error::kSignInit);
+        return false;
       }
       break;
     case SignConfiguration::kVerify:
@@ -696,6 +697,7 @@ bool SignTraits::DeriveBits(
               nullptr,
               params.key.get())) {
         crypto::CheckThrow(env, SignBase::Error::kSignInit);
+        return false;
       }
       break;
   }
@@ -713,6 +715,7 @@ bool SignTraits::DeriveBits(
           padding,
           salt_length)) {
     crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey);
+    return false;
   }
 
   switch (params.mode) {
diff --git a/test/parallel/test-crypto-sign-verify.js b/test/parallel/test-crypto-sign-verify.js
index 444135538ccff8..6893f0c0e6d49a 100644
--- a/test/parallel/test-crypto-sign-verify.js
+++ b/test/parallel/test-crypto-sign-verify.js
@@ -631,3 +631,114 @@ assert.throws(
     assert(stdout.includes('Verified OK'));
   }));
 }
+
+{
+  // Test RSA-PSS.
+  {
+    // This key pair does not restrict the message digest algorithm or salt
+    // length.
+    const publicPem = fixtures.readKey('rsa_pss_public_2048.pem');
+    const privatePem = fixtures.readKey('rsa_pss_private_2048.pem');
+
+    const publicKey = crypto.createPublicKey(publicPem);
+    const privateKey = crypto.createPrivateKey(privatePem);
+
+    for (const key of [privatePem, privateKey]) {
+      // Any algorithm should work.
+      for (const algo of ['sha1', 'sha256']) {
+        // Any salt length should work.
+        for (const saltLength of [undefined, 8, 10, 12, 16, 18, 20]) {
+          const signature = crypto.sign(algo, 'foo', { key, saltLength });
+
+          for (const pkey of [key, publicKey, publicPem]) {
+            const okay = crypto.verify(
+              algo,
+              'foo',
+              { key: pkey, saltLength },
+              signature
+            );
+
+            assert.ok(okay);
+          }
+        }
+      }
+    }
+  }
+
+  {
+    // This key pair enforces sha256 as the message digest and the MGF1
+    // message digest and a salt length of at least 16 bytes.
+    const publicPem =
+      fixtures.readKey('rsa_pss_public_2048_sha256_sha256_16.pem');
+    const privatePem =
+      fixtures.readKey('rsa_pss_private_2048_sha256_sha256_16.pem');
+
+    const publicKey = crypto.createPublicKey(publicPem);
+    const privateKey = crypto.createPrivateKey(privatePem);
+
+    for (const key of [privatePem, privateKey]) {
+      // Signing with anything other than sha256 should fail.
+      assert.throws(() => {
+        crypto.sign('sha1', 'foo', key);
+      }, /digest not allowed/);
+
+      // Signing with salt lengths less than 16 bytes should fail.
+      for (const saltLength of [8, 10, 12]) {
+        assert.throws(() => {
+          crypto.sign('sha256', 'foo', { key, saltLength });
+        }, /pss saltlen too small/);
+      }
+
+      // Signing with sha256 and appropriate salt lengths should work.
+      for (const saltLength of [undefined, 16, 18, 20]) {
+        const signature = crypto.sign('sha256', 'foo', { key, saltLength });
+
+        for (const pkey of [key, publicKey, publicPem]) {
+          const okay = crypto.verify(
+            'sha256',
+            'foo',
+            { key: pkey, saltLength },
+            signature
+          );
+
+          assert.ok(okay);
+        }
+      }
+    }
+  }
+
+  {
+    // This key enforces sha512 as the message digest and sha256 as the MGF1
+    // message digest.
+    const publicPem =
+      fixtures.readKey('rsa_pss_public_2048_sha512_sha256_20.pem');
+    const privatePem =
+      fixtures.readKey('rsa_pss_private_2048_sha512_sha256_20.pem');
+
+    const publicKey = crypto.createPublicKey(publicPem);
+    const privateKey = crypto.createPrivateKey(privatePem);
+
+    // Node.js usually uses the same hash function for the message and for MGF1.
+    // However, when a different MGF1 message digest algorithm has been
+    // specified as part of the key, it should automatically switch to that.
+    // This behavior is required by sections 3.1 and 3.3 of RFC4055.
+    for (const key of [privatePem, privateKey]) {
+      // sha256 matches the MGF1 hash function and should be used internally,
+      // but it should not be permitted as the main message digest algorithm.
+      for (const algo of ['sha1', 'sha256']) {
+        assert.throws(() => {
+          crypto.sign(algo, 'foo', key);
+        }, /digest not allowed/);
+      }
+
+      // sha512 should produce a valid signature.
+      const signature = crypto.sign('sha512', 'foo', key);
+
+      for (const pkey of [key, publicKey, publicPem]) {
+        const okay = crypto.verify('sha512', 'foo', pkey, signature);
+
+        assert.ok(okay);
+      }
+    }
+  }
+}