crypto: fix rsa-pss one-shot sign/verify error handling

panva · targos · commit 1dd91582dada · 2021-09-06T09:19:20.000+02:00
fixes #39822 PR-URL: #39830 Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git 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
@@ -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);
+      }
+    }
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -686,6 +686,7 @@ bool SignTraits::DeriveBits(`
`686`	`686`	`nullptr,`
`687`	`687`	`params.key.get())) {`
`688`	`688`	`crypto::CheckThrow(env, SignBase::Error::kSignInit);`
	`689`	`+ return false;`
`689`	`690`	`}`
`690`	`691`	`break;`
`691`	`692`	`case SignConfiguration::kVerify:`
`@@ -696,6 +697,7 @@ bool SignTraits::DeriveBits(`
`696`	`697`	`nullptr,`
`697`	`698`	`params.key.get())) {`
`698`	`699`	`crypto::CheckThrow(env, SignBase::Error::kSignInit);`
	`700`	`+ return false;`
`699`	`701`	`}`
`700`	`702`	`break;`
`701`	`703`	`}`
`@@ -713,6 +715,7 @@ bool SignTraits::DeriveBits(`
`713`	`715`	`padding,`
`714`	`716`	`salt_length)) {`
`715`	`717`	`crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey);`
	`718`	`+ return false;`
`716`	`719`	`}`
`717`	`720`
`718`	`721`	`switch (params.mode) {`