crypto: disable PKCS#1 padding for privateDecrypt

Refs: https://hackerone.com/bugs?subject=nodejs&report_id=2269177 Disable RSA_PKCS1_PADDING for crypto.privateDecrypt() in order to protect against the Marvin attack. Includes a security revert flag that can be used to restore support. Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs-private/node-private#525 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=2269177 Reviewed-By: Rafael Gonzaga <[email protected]> CVE-ID: CVE-2023-46809
nodejs · Feb 14, 2024 · 54cd268 · 54cd268
1 parent b43171c
commit 54cd268
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 12 deletions.
diff --git a/src/crypto/crypto_cipher.cc b/src/crypto/crypto_cipher.cc
@@ -1043,6 +1043,32 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
   uint32_t padding;
   if (!args[offset + 1]->Uint32Value(env->context()).To(&padding)) return;
 
+  if (EVP_PKEY_cipher == EVP_PKEY_decrypt &&
+      operation == PublicKeyCipher::kPrivate && padding == RSA_PKCS1_PADDING) {
+    EVPKeyCtxPointer ctx(EVP_PKEY_CTX_new(pkey.get(), nullptr));
+    CHECK(ctx);
+
+    if (EVP_PKEY_decrypt_init(ctx.get()) <= 0) {
+      return ThrowCryptoError(env, ERR_get_error());
+    }
+
+    int rsa_pkcs1_implicit_rejection =
+        EVP_PKEY_CTX_ctrl_str(ctx.get(), "rsa_pkcs1_implicit_rejection", "1");
+    // From the doc -2 means that the option is not supported.
+    // The default for the option is enabled and if it has been
+    // specifically disabled we want to respect that so we will
+    // not throw an error if the option is supported regardless
+    // of how it is set. The call to set the value
+    // will not affect what is used since a different context is
+    // used in the call if the option is supported
+    if (rsa_pkcs1_implicit_rejection <= 0) {
+      return THROW_ERR_INVALID_ARG_VALUE(
+          env,
+          "RSA_PKCS1_PADDING is no longer supported for private decryption,"
+          " this can be reverted with --security-revert=CVE-2024-PEND");
+    }
+  }
+
   const EVP_MD* digest = nullptr;
   if (args[offset + 2]->IsString()) {
     const Utf8Value oaep_str(env->isolate(), args[offset + 2]);

diff --git a/test/parallel/test-crypto-rsa-dsa.js b/test/parallel/test-crypto-rsa-dsa.js
@@ -221,19 +221,37 @@ function test_rsa(padding, encryptOaepHash, decryptOaepHash) {
     oaepHash: encryptOaepHash
   }, bufferToEncrypt);
 
-  let decryptedBuffer = crypto.privateDecrypt({
-    key: rsaKeyPem,
-    padding: padding,
-    oaepHash: decryptOaepHash
-  }, encryptedBuffer);
-  assert.deepStrictEqual(decryptedBuffer, input);
 
-  decryptedBuffer = crypto.privateDecrypt({
-    key: rsaPkcs8KeyPem,
-    padding: padding,
-    oaepHash: decryptOaepHash
-  }, encryptedBuffer);
-  assert.deepStrictEqual(decryptedBuffer, input);
+  if (padding === constants.RSA_PKCS1_PADDING) {
+    assert.throws(() => {
+      crypto.privateDecrypt({
+        key: rsaKeyPem,
+        padding: padding,
+        oaepHash: decryptOaepHash
+      }, encryptedBuffer);
+    }, { code: 'ERR_INVALID_ARG_VALUE' });
+    assert.throws(() => {
+      crypto.privateDecrypt({
+        key: rsaPkcs8KeyPem,
+        padding: padding,
+        oaepHash: decryptOaepHash
+      }, encryptedBuffer);
+    }, { code: 'ERR_INVALID_ARG_VALUE' });
+  } else {
+    let decryptedBuffer = crypto.privateDecrypt({
+      key: rsaKeyPem,
+      padding: padding,
+      oaepHash: decryptOaepHash
+    }, encryptedBuffer);
+    assert.deepStrictEqual(decryptedBuffer, input);
+
+    decryptedBuffer = crypto.privateDecrypt({
+      key: rsaPkcs8KeyPem,
+      padding: padding,
+      oaepHash: decryptOaepHash
+    }, encryptedBuffer);
+    assert.deepStrictEqual(decryptedBuffer, input);
+  }
 }
 
 test_rsa('RSA_NO_PADDING');