Repository: denoland/deno
Pull request: fix(ext/node): add key length validation in `DiffieHellman.prototype.computeSecret`

This PR fixes mainly 2 things

- Added key length validation in `DiffieHellman.prototype.computeSecret`.
- Changed the final block handling of Decipheriv of aes-{128,192,256}-ecb. Now empty final block throws.

These changes enable `parallel/test-crypto-dh.js` test case.

towards #29637 

Changed files:
filename: ext/node/ops/crypto/cipher.rs
patch: @@ -11,6 +11,7 @@ use aes::cipher::KeyIvInit;
 use aes::cipher::KeySizeUser;
 use aes::cipher::StreamCipher;
 use deno_core::Resource;
+use deno_error::JsErrorClass;
 use digest::generic_array::GenericArray;
 use digest::KeyInit;
 
@@ -433,7 +434,7 @@ impl Cipher {
 
 #[derive(Debug, thiserror::Error, deno_error::JsError)]
 #[property("library" = "Provider routines")]
-#[property("reason" = self.get_message().to_string())]
+#[property("reason" = self.reason())]
 #[property("code" = self.code())]
 pub enum DecipherError {
   #[class(type)]
@@ -446,7 +447,7 @@ pub enum DecipherError {
   #[error("Invalid authentication tag length: {0}")]
   InvalidAuthTag(usize),
   #[class(range)]
-  #[error("Wrong final block length")]
+  #[error("error:1C80006B:Provider routines::wrong final block length")]
   InvalidFinalBlockLength,
   #[class(type)]
   #[error("Invalid initialization vector")]
@@ -480,9 +481,21 @@ impl DecipherError {
       Self::InvalidAuthTag(_) => {
         deno_error::PropertyValue::String("ERR_CRYPTO_INVALID_AUTH_TAG".into())
       }
+      Self::InvalidFinalBlockLength => deno_error::PropertyValue::String(
+        "ERR_OSSL_WRONG_FINAL_BLOCK_LENGTH".into(),
+      ),
       _ => deno_error::PropertyValue::String("ERR_CRYPTO_DECIPHER".into()),
     }
   }
+
+  fn reason(&self) -> deno_error::PropertyValue {
+    match self {
+      Self::InvalidFinalBlockLength => {
+        deno_error::PropertyValue::String("wrong final block length".into())
+      }
+      _ => deno_error::PropertyValue::String(self.get_message()),
+    }
+  }
 }
 
 macro_rules! assert_block_len {
@@ -677,7 +690,16 @@ impl Decipher {
   ) -> Result<(), DecipherError> {
     use Decipher::*;
 
-    if input.is_empty() && !matches!(self, Aes128Gcm(..) | Aes256Gcm(..)) {
+    if input.is_empty()
+      && !matches!(
+        self,
+        Aes128Ecb(..)
+          | Aes192Ecb(..)
+          | Aes256Ecb(..)
+          | Aes128Gcm(..)
+          | Aes256Gcm(..)
+      )
+    {
       return Ok(());
     }
 

filename: ext/node/polyfills/internal/crypto/diffiehellman.ts
patch: @@ -42,6 +42,7 @@ import type {
   ECDHKeyFormat,
 } from "ext:deno_node/internal/crypto/types.ts";
 import {
+  getArrayBufferOrView,
   getKeyObjectHandle,
   kConsumePrivate,
   kConsumePublic,
@@ -194,11 +195,12 @@ export class DiffieHellmanImpl {
     inputEncoding?: BinaryToTextEncoding,
     outputEncoding?: BinaryToTextEncoding,
   ): Buffer | string {
-    let buf;
-    if (inputEncoding != undefined && inputEncoding != "buffer") {
-      buf = Buffer.from(otherPublicKey.buffer, inputEncoding);
-    } else {
-      buf = Buffer.from(otherPublicKey.buffer);
+    const buf = getArrayBufferOrView(otherPublicKey, "key", inputEncoding);
+    if (buf.length === 0) {
+      throw new NodeError(
+        "ERR_CRYPTO_INVALID_KEYLEN",
+        "Unspecified validation error",
+      );
     }
 
     const sharedSecret = op_node_dh_compute_secret(

filename: tests/node_compat/config.jsonc
patch: @@ -44,7 +44,6 @@
       "test-console-instance.js",
       "test-console-tty-colors.js",
       "test-crypto-dh-errors.js",
-      "test-crypto-dh.js",
       "test-crypto-hash.js",
       "test-crypto-hkdf.js",
       "test-crypto-hmac.js",

filename: tests/node_compat/test/parallel/test-crypto-dh.js
patch: @@ -2,145 +2,70 @@
 // deno-lint-ignore-file
 
 // Copyright Joyent and Node contributors. All rights reserved. MIT license.
-// Taken from Node 18.12.1
-// This file is automatically generated by "node/_tools/setup.ts". Do not modify this file manually
+// Taken from Node 23.9.0
+// This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually.
 
 'use strict';
 const common = require('../common');
-if (!common.hasCrypto)
+if (!common.hasCrypto) {
   common.skip('missing crypto');
+}
 
 const assert = require('assert');
 const crypto = require('crypto');
+const {
+  hasOpenSSL3,
+  hasOpenSSL,
+} = require('../common/crypto');
 
-const size = common.hasFipsCrypto || common.hasOpenSSL3 ? 1024 : 256;
-const dh1 = crypto.createDiffieHellman(size);
-const p1 = dh1.getPrime('buffer');
-const dh2 = crypto.createDiffieHellman(p1, 'buffer');
-const key1 = dh1.generateKeys();
-const key2 = dh2.generateKeys('hex');
-const secret1 = dh1.computeSecret(key2, 'hex', 'base64');
-const secret2 = dh2.computeSecret(key1, 'latin1', 'buffer');
-
-// Test Diffie-Hellman with two parties sharing a secret,
-// using various encodings as we go along
-assert.strictEqual(secret2.toString('base64'), secret1);
-assert.strictEqual(dh1.verifyError, 0);
-assert.strictEqual(dh2.verifyError, 0);
-
-// https://github.com/nodejs/node/issues/32738
-// XXX(bnoordhuis) validateInt32() throwing ERR_OUT_OF_RANGE and RangeError
-// instead of ERR_INVALID_ARG_TYPE and TypeError is questionable, IMO.
-assert.throws(() => crypto.createDiffieHellman(13.37), {
-  code: 'ERR_OUT_OF_RANGE',
-  name: 'RangeError',
-  message: 'The value of "sizeOrKey" is out of range. ' +
-           'It must be an integer. Received 13.37',
-});
-
-assert.throws(() => crypto.createDiffieHellman('abcdef', 13.37), {
-  code: 'ERR_OUT_OF_RANGE',
-  name: 'RangeError',
-  message: 'The value of "generator" is out of range. ' +
-           'It must be an integer. Received 13.37',
-});
-
-for (const bits of [-1, 0, 1]) {
-  if (common.hasOpenSSL3) {
-    assert.throws(() => crypto.createDiffieHellman(bits), {
-      code: 'ERR_OSSL_DH_MODULUS_TOO_SMALL',
-      name: 'Error',
-      message: /modulus too small/,
-    });
-  } else {
-    assert.throws(() => crypto.createDiffieHellman(bits), {
-      code: 'ERR_OSSL_BN_BITS_TOO_SMALL',
-      name: 'Error',
-      message: /bits too small/,
-    });
-  }
-}
-
-// Through a fluke of history, g=0 defaults to DH_GENERATOR (2).
 {
-  const g = 0;
-  crypto.createDiffieHellman('abcdef', g);
-  crypto.createDiffieHellman('abcdef', 'hex', g);
-}
-
-for (const g of [-1, 1]) {
-  const ex = {
-    code: 'ERR_OSSL_DH_BAD_GENERATOR',
-    name: 'Error',
-    message: /bad generator/,
-  };
-  assert.throws(() => crypto.createDiffieHellman('abcdef', g), ex);
-  assert.throws(() => crypto.createDiffieHellman('abcdef', 'hex', g), ex);
-}
+  const size = crypto.getFips() || hasOpenSSL3 ? 1024 : 256;
+  const dh1 = crypto.createDiffieHellman(size);
+  const p1 = dh1.getPrime('buffer');
+  const dh2 = crypto.createDiffieHellman(p1, 'buffer');
+  const key1 = dh1.generateKeys();
+  const key2 = dh2.generateKeys('hex');
+  const secret1 = dh1.computeSecret(key2, 'hex', 'base64');
+  const secret2 = dh2.computeSecret(key1, 'latin1', 'buffer');
+
+  // Test Diffie-Hellman with two parties sharing a secret,
+  // using various encodings as we go along
+  assert.strictEqual(secret2.toString('base64'), secret1);
+  assert.strictEqual(dh1.verifyError, 0);
+  assert.strictEqual(dh2.verifyError, 0);
+
+  // Create "another dh1" using generated keys from dh1,
+  // and compute secret again
+  const dh3 = crypto.createDiffieHellman(p1, 'buffer');
+  const privkey1 = dh1.getPrivateKey();
+  dh3.setPublicKey(key1);
+  dh3.setPrivateKey(privkey1);
+
+  assert.deepStrictEqual(dh1.getPrime(), dh3.getPrime());
+  assert.deepStrictEqual(dh1.getGenerator(), dh3.getGenerator());
+  assert.deepStrictEqual(dh1.getPublicKey(), dh3.getPublicKey());
+  assert.deepStrictEqual(dh1.getPrivateKey(), dh3.getPrivateKey());
+  assert.strictEqual(dh3.verifyError, 0);
+
+  const secret3 = dh3.computeSecret(key2, 'hex', 'base64');
+
+  assert.strictEqual(secret1, secret3);
+
+  // computeSecret works without a public key set at all.
+  const dh4 = crypto.createDiffieHellman(p1, 'buffer');
+  dh4.setPrivateKey(privkey1);
+
+  assert.deepStrictEqual(dh1.getPrime(), dh4.getPrime());
+  assert.deepStrictEqual(dh1.getGenerator(), dh4.getGenerator());
+  assert.deepStrictEqual(dh1.getPrivateKey(), dh4.getPrivateKey());
+  assert.strictEqual(dh4.verifyError, 0);
+
+  const secret4 = dh4.computeSecret(key2, 'hex', 'base64');
+
+  assert.strictEqual(secret1, secret4);
 
-crypto.createDiffieHellman('abcdef', Buffer.from([2]));  // OK
-
-for (const g of [Buffer.from([]),
-                 Buffer.from([0]),
-                 Buffer.from([1])]) {
-  const ex = {
-    code: 'ERR_OSSL_DH_BAD_GENERATOR',
-    name: 'Error',
-    message: /bad generator/,
-  };
-  assert.throws(() => crypto.createDiffieHellman('abcdef', g), ex);
-  assert.throws(() => crypto.createDiffieHellman('abcdef', 'hex', g), ex);
-}
-
-[
-  [0x1, 0x2],
-  () => { },
-  /abc/,
-  {},
-].forEach((input) => {
-  assert.throws(
-    () => crypto.createDiffieHellman(input),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      name: 'TypeError',
-    }
-  );
-});
-
-// Create "another dh1" using generated keys from dh1,
-// and compute secret again
-const dh3 = crypto.createDiffieHellman(p1, 'buffer');
-const privkey1 = dh1.getPrivateKey();
-dh3.setPublicKey(key1);
-dh3.setPrivateKey(privkey1);
-
-assert.deepStrictEqual(dh1.getPrime(), dh3.getPrime());
-assert.deepStrictEqual(dh1.getGenerator(), dh3.getGenerator());
-assert.deepStrictEqual(dh1.getPublicKey(), dh3.getPublicKey());
-assert.deepStrictEqual(dh1.getPrivateKey(), dh3.getPrivateKey());
-assert.strictEqual(dh3.verifyError, 0);
-
-const secret3 = dh3.computeSecret(key2, 'hex', 'base64');
-
-assert.strictEqual(secret1, secret3);
-
-// computeSecret works without a public key set at all.
-const dh4 = crypto.createDiffieHellman(p1, 'buffer');
-dh4.setPrivateKey(privkey1);
-
-assert.deepStrictEqual(dh1.getPrime(), dh4.getPrime());
-assert.deepStrictEqual(dh1.getGenerator(), dh4.getGenerator());
-assert.deepStrictEqual(dh1.getPrivateKey(), dh4.getPrivateKey());
-assert.strictEqual(dh4.verifyError, 0);
-
-const secret4 = dh4.computeSecret(key2, 'hex', 'base64');
-
-assert.strictEqual(secret1, secret4);
-
-
-if (false) {
   let wrongBlockLength;
-  if (common.hasOpenSSL3) {
+  if (hasOpenSSL3) {
     wrongBlockLength = {
       message: 'error:1C80006B:Provider routines::wrong final block length',
       code: 'ERR_OSSL_WRONG_FINAL_BLOCK_LENGTH',
@@ -172,43 +97,25 @@ if (false) {
     }, wrongBlockLength);
   }
 
-  assert.throws(() => {
-    dh3.computeSecret('');
-  }, { message: common.hasOpenSSL3 ?
-    'error:02800080:Diffie-Hellman routines::invalid secret' :
-    'Supplied key is too small' });
-
-  // Invalid test: curve argument is undefined
-  assert.throws(
-    () => crypto.createECDH(),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      name: 'TypeError',
-      message: 'The "curve" argument must be of type string. ' +
-              'Received undefined'
-    });
-}
-
-assert.throws(
-  function() {
-    crypto.getDiffieHellman('unknown-group');
-  },
-  {
-    name: 'Error',
-    code: 'ERR_CRYPTO_UNKNOWN_DH_GROUP',
-    message: 'Unknown DH group'
-  },
-  'crypto.getDiffieHellman(\'unknown-group\') ' +
-  'failed to throw the expected error.'
-);
-
-assert.throws(
-  () => crypto.createDiffieHellman('', true),
   {
-    code: 'ERR_INVALID_ARG_TYPE'
+    // Error message was changed in OpenSSL 3.0.x from 3.0.12, and 3.1.x from 3.1.4.
+    const hasOpenSSL3WithNewErrorMessage = (hasOpenSSL(3, 0, 12) && !hasOpenSSL(3, 1, 0)) ||
+                                           (hasOpenSSL(3, 1, 4));
+    assert.throws(() => {
+      dh3.computeSecret('');
+    }, { message: hasOpenSSL3 && !hasOpenSSL3WithNewErrorMessage ?
+      'Unspecified validation error' :
+      'Supplied key is too small' });
   }
-);
-[true, Symbol(), {}, () => {}, []].forEach((generator) => assert.throws(
-  () => crypto.createDiffieHellman('', 'base64', generator),
-  { code: 'ERR_INVALID_ARG_TYPE' }
-));
+}
+
+// Through a fluke of history, g=0 defaults to DH_GENERATOR (2).
+{
+  const g = 0;
+  crypto.createDiffieHellman('abcdef', g);
+  crypto.createDiffieHellman('abcdef', 'hex', g);
+}
+
+{
+  crypto.createDiffieHellman('abcdef', Buffer.from([2]));  // OK
+}

filename: tests/unit_node/crypto/crypto_cipher_test.ts
patch: @@ -471,7 +471,7 @@ Deno.test({
         decipher.final();
       },
       RangeError,
-      "Wrong final block length",
+      "wrong final block length",
     );
   },
 });