crypto: remove Argon2 KDF derivation from its job setup

Fixes: #62861

Signed-off-by: Filip Skokan <[email protected]>
PR-URL: #62863
Fixes: #62861
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>

Author: panva

src/crypto/crypto_argon2.cc

@@ -104,20 +104,6 @@ Maybe<void> Argon2Traits::AdditionalConfig(
   config->type =
       static_cast<ncrypto::Argon2Type>(args[offset + 8].As<Uint32>()->Value());
 
-  if (!ncrypto::argon2(config->pass,
-                       config->salt,
-                       config->lanes,
-                       config->keylen,
-                       config->memcost,
-                       config->iter,
-                       config->version,
-                       config->secret,
-                       config->ad,
-                       config->type)) {
-    THROW_ERR_CRYPTO_INVALID_ARGON2_PARAMS(env);
-    return Nothing<void>();
-  }
-
   return JustVoid();
 }
 

src/node_errors.h

@@ -53,7 +53,6 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
   V(ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED, Error)                             \
   V(ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, Error)                                \
   V(ERR_CRYPTO_INITIALIZATION_FAILED, Error)                                   \
-  V(ERR_CRYPTO_INVALID_ARGON2_PARAMS, TypeError)                               \
   V(ERR_CRYPTO_INVALID_AUTH_TAG, TypeError)                                    \
   V(ERR_CRYPTO_INVALID_COUNTER, TypeError)                                     \
   V(ERR_CRYPTO_INVALID_CURVE, TypeError)                                       \
@@ -198,7 +197,6 @@ ERRORS_WITH_CODE(V)
   V(ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS,                                       \
     "The selected key encoding is incompatible with the key type")             \
   V(ERR_CRYPTO_INITIALIZATION_FAILED, "Initialization failed")                 \
-  V(ERR_CRYPTO_INVALID_ARGON2_PARAMS, "Invalid Argon2 params")                 \
   V(ERR_CRYPTO_INVALID_AUTH_TAG, "Invalid authentication tag")                 \
   V(ERR_CRYPTO_INVALID_COUNTER, "Invalid counter")                             \
   V(ERR_CRYPTO_INVALID_CURVE, "Invalid EC curve name")                         \

test/parallel/test-crypto-argon2-job.js

@@ -0,0 +1,59 @@
+// Flags: --expose-internals --no-warnings
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const { hasOpenSSL } = require('../common/crypto');
+
+if (!hasOpenSSL(3, 2))
+  common.skip('requires OpenSSL >= 3.2');
+
+// Exercises the native Argon2 job directly via internalBinding, bypassing
+// the JS validators, to ensure that if invalid parameters ever reach the
+// native layer they produce a clean error from the KDF rather than crashing,
+// in both sync and async modes.
+
+const assert = require('node:assert');
+const { internalBinding } = require('internal/test/binding');
+const {
+  Argon2Job,
+  kCryptoJobAsync,
+  kCryptoJobSync,
+  kTypeArgon2id,
+} = internalBinding('crypto');
+
+const pass = Buffer.from('password');
+const salt = Buffer.alloc(16, 0x02);
+const empty = Buffer.alloc(0);
+
+// Parameters that OpenSSL's Argon2 KDF rejects.
+const badParams = [
+  { lanes: 0, keylen: 32, memcost: 16, iter: 1 },  // lanes < 1
+  { lanes: 1, keylen: 32, memcost: 0, iter: 1 },   // memcost == 0
+  { lanes: 1, keylen: 32, memcost: 16, iter: 0 },  // iter == 0
+];
+
+for (const { lanes, keylen, memcost, iter } of badParams) {
+  {
+    const job = new Argon2Job(
+      kCryptoJobSync, pass, salt, lanes, keylen, memcost, iter,
+      empty, empty, kTypeArgon2id);
+    const { 0: err, 1: result } = job.run();
+    assert.ok(err);
+    assert.match(err.message, /Argon2 derivation failed/);
+    assert.strictEqual(result, undefined);
+  }
+
+  {
+    const job = new Argon2Job(
+      kCryptoJobAsync, pass, salt, lanes, keylen, memcost, iter,
+      empty, empty, kTypeArgon2id);
+    job.ondone = common.mustCall((err, result) => {
+      assert.ok(err);
+      assert.match(err.message, /Argon2 derivation failed/);
+      assert.strictEqual(result, undefined);
+    });
+    job.run();
+  }
+}

test/pummel/test-crypto-argon2-nonblocking-constructor.js

@@ -0,0 +1,81 @@
+// Flags: --expose-internals --no-warnings
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const { hasOpenSSL } = require('../common/crypto');
+
+if (!hasOpenSSL(3, 2))
+  common.skip('requires OpenSSL >= 3.2');
+
+// Regression test for https://github.com/nodejs/node/issues/62861.
+// `AdditionalConfig` used to invoke the full Argon2 KDF synchronously inside
+// `new Argon2Job(...)` for the purpose of getting a parameter validation
+// error.
+//
+// The fix removes the redundant KDF call from the constructor. This test
+// asserts that the constructor returns in a small fraction of the time a
+// full sync job takes. Pre-fix the constructor ran the KDF, and job.run()
+// then ran it a second time in DeriveBits; post-fix the constructor does
+// no KDF work at all.
+
+const assert = require('node:assert');
+const { internalBinding } = require('internal/test/binding');
+const {
+  Argon2Job,
+  kCryptoJobAsync,
+  kCryptoJobSync,
+  kTypeArgon2id,
+} = internalBinding('crypto');
+
+const pass = Buffer.from('password');
+const salt = Buffer.alloc(16, 0x02);
+const empty = Buffer.alloc(0);
+
+// Tuned so a single-threaded Argon2id derivation is expensive enough that
+// scheduler/GC noise is negligible compared to it.
+const kdfArgs = [
+  pass,
+  salt,
+  1, // lanes
+  32, // keylen
+  65536, // memcost
+  20, // iter
+  empty, // secret
+  empty, // associatedData
+  kTypeArgon2id,
+];
+
+// For each mode, measure the constructor and the derivation separately and
+// assert that the constructor is a small fraction of the derivation. Pre-fix
+// the constructor ran a full KDF, so ctorNs was comparable to runNs. Post-fix
+// the constructor does no KDF work and must be orders of magnitude faster.
+//
+// For async mode the derivation happens on the thread pool, so runNs is
+// measured from the start of run() until ondone fires.
+
+// Sync: run() derives on the main thread and returns when done.
+{
+  const ctorStart = process.hrtime.bigint();
+  const job = new Argon2Job(kCryptoJobSync, ...kdfArgs);
+  const ctorNs = process.hrtime.bigint() - ctorStart;
+  const runStart = process.hrtime.bigint();
+  const { 0: err } = job.run();
+  const runNs = process.hrtime.bigint() - runStart;
+  assert.strictEqual(err, undefined);
+  assert.ok(ctorNs * 10n < runNs);
+}
+
+// Async: run() dispatches to the thread pool; measure until ondone fires.
+{
+  const ctorStart = process.hrtime.bigint();
+  const job = new Argon2Job(kCryptoJobAsync, ...kdfArgs);
+  const ctorNs = process.hrtime.bigint() - ctorStart;
+  const runStart = process.hrtime.bigint();
+  job.ondone = common.mustSucceed(() => {
+    const runNs = process.hrtime.bigint() - runStart;
+    assert.ok(ctorNs * 10n < runNs);
+  });
+  job.run();
+}