Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 28a7874

Browse filesBrowse files
panvaaduh95
panva
authored and
aduh95
committed
crypto: remove Argon2 KDF derivation from its job setup
Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #62863 Backport-PR-URL: #63173 Fixes: #62861 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 2ca42c8 commit 28a7874
Copy full SHA for 28a7874

4 files changed

+140-16Lines changed: 140 additions & 16 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/crypto/crypto_argon2.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_argon2.cc
-14Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -104,20 +104,6 @@ Maybe<void> Argon2Traits::AdditionalConfig(
104104
config->type =
105105
static_cast<ncrypto::Argon2Type>(args[offset + 8].As<Uint32>()->Value());
106106

107-
if (!ncrypto::argon2(config->pass,
108-
config->salt,
109-
config->lanes,
110-
config->keylen,
111-
config->memcost,
112-
config->iter,
113-
config->version,
114-
config->secret,
115-
config->ad,
116-
config->type)) {
117-
THROW_ERR_CRYPTO_INVALID_ARGON2_PARAMS(env);
118-
return Nothing<void>();
119-
}
120-
121107
return JustVoid();
122108
}
123109

Collapse file

‎src/node_errors.h‎

Copy file name to clipboardExpand all lines: src/node_errors.h
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
5353
V(ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED, Error) \
5454
V(ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, Error) \
5555
V(ERR_CRYPTO_INITIALIZATION_FAILED, Error) \
56-
V(ERR_CRYPTO_INVALID_ARGON2_PARAMS, TypeError) \
5756
V(ERR_CRYPTO_INVALID_AUTH_TAG, TypeError) \
5857
V(ERR_CRYPTO_INVALID_COUNTER, TypeError) \
5958
V(ERR_CRYPTO_INVALID_CURVE, TypeError) \
@@ -195,7 +194,6 @@ ERRORS_WITH_CODE(V)
195194
V(ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, \
196195
"The selected key encoding is incompatible with the key type") \
197196
V(ERR_CRYPTO_INITIALIZATION_FAILED, "Initialization failed") \
198-
V(ERR_CRYPTO_INVALID_ARGON2_PARAMS, "Invalid Argon2 params") \
199197
V(ERR_CRYPTO_INVALID_AUTH_TAG, "Invalid authentication tag") \
200198
V(ERR_CRYPTO_INVALID_COUNTER, "Invalid counter") \
201199
V(ERR_CRYPTO_INVALID_CURVE, "Invalid EC curve name") \
Collapse file

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

Copy file name to clipboard
+59Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// Flags: --expose-internals --no-warnings
2+
'use strict';
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const { hasOpenSSL } = require('../common/crypto');
8+
9+
if (!hasOpenSSL(3, 2))
10+
common.skip('requires OpenSSL >= 3.2');
11+
12+
// Exercises the native Argon2 job directly via internalBinding, bypassing
13+
// the JS validators, to ensure that if invalid parameters ever reach the
14+
// native layer they produce a clean error from the KDF rather than crashing,
15+
// in both sync and async modes.
16+
17+
const assert = require('node:assert');
18+
const { internalBinding } = require('internal/test/binding');
19+
const {
20+
Argon2Job,
21+
kCryptoJobAsync,
22+
kCryptoJobSync,
23+
kTypeArgon2id,
24+
} = internalBinding('crypto');
25+
26+
const pass = Buffer.from('password');
27+
const salt = Buffer.alloc(16, 0x02);
28+
const empty = Buffer.alloc(0);
29+
30+
// Parameters that OpenSSL's Argon2 KDF rejects.
31+
const badParams = [
32+
{ lanes: 0, keylen: 32, memcost: 16, iter: 1 }, // lanes < 1
33+
{ lanes: 1, keylen: 32, memcost: 0, iter: 1 }, // memcost == 0
34+
{ lanes: 1, keylen: 32, memcost: 16, iter: 0 }, // iter == 0
35+
];
36+
37+
for (const { lanes, keylen, memcost, iter } of badParams) {
38+
{
39+
const job = new Argon2Job(
40+
kCryptoJobSync, pass, salt, lanes, keylen, memcost, iter,
41+
empty, empty, kTypeArgon2id);
42+
const { 0: err, 1: result } = job.run();
43+
assert.ok(err);
44+
assert.match(err.message, /Deriving bits failed/);
45+
assert.strictEqual(result, undefined);
46+
}
47+
48+
{
49+
const job = new Argon2Job(
50+
kCryptoJobAsync, pass, salt, lanes, keylen, memcost, iter,
51+
empty, empty, kTypeArgon2id);
52+
job.ondone = common.mustCall((err, result) => {
53+
assert.ok(err);
54+
assert.match(err.message, /Deriving bits failed/);
55+
assert.strictEqual(result, undefined);
56+
});
57+
job.run();
58+
}
59+
}
Collapse file

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

Copy file name to clipboard
+81Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Flags: --expose-internals --no-warnings
2+
'use strict';
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const { hasOpenSSL } = require('../common/crypto');
8+
9+
if (!hasOpenSSL(3, 2))
10+
common.skip('requires OpenSSL >= 3.2');
11+
12+
// Regression test for https://github.com/nodejs/node/issues/62861.
13+
// `AdditionalConfig` used to invoke the full Argon2 KDF synchronously inside
14+
// `new Argon2Job(...)` for the purpose of getting a parameter validation
15+
// error.
16+
//
17+
// The fix removes the redundant KDF call from the constructor. This test
18+
// asserts that the constructor returns in a small fraction of the time a
19+
// full sync job takes. Pre-fix the constructor ran the KDF, and job.run()
20+
// then ran it a second time in DeriveBits; post-fix the constructor does
21+
// no KDF work at all.
22+
23+
const assert = require('node:assert');
24+
const { internalBinding } = require('internal/test/binding');
25+
const {
26+
Argon2Job,
27+
kCryptoJobAsync,
28+
kCryptoJobSync,
29+
kTypeArgon2id,
30+
} = internalBinding('crypto');
31+
32+
const pass = Buffer.from('password');
33+
const salt = Buffer.alloc(16, 0x02);
34+
const empty = Buffer.alloc(0);
35+
36+
// Tuned so a single-threaded Argon2id derivation is expensive enough that
37+
// scheduler/GC noise is negligible compared to it.
38+
const kdfArgs = [
39+
pass,
40+
salt,
41+
1, // lanes
42+
32, // keylen
43+
65536, // memcost
44+
20, // iter
45+
empty, // secret
46+
empty, // associatedData
47+
kTypeArgon2id,
48+
];
49+
50+
// For each mode, measure the constructor and the derivation separately and
51+
// assert that the constructor is a small fraction of the derivation. Pre-fix
52+
// the constructor ran a full KDF, so ctorNs was comparable to runNs. Post-fix
53+
// the constructor does no KDF work and must be orders of magnitude faster.
54+
//
55+
// For async mode the derivation happens on the thread pool, so runNs is
56+
// measured from the start of run() until ondone fires.
57+
58+
// Sync: run() derives on the main thread and returns when done.
59+
{
60+
const ctorStart = process.hrtime.bigint();
61+
const job = new Argon2Job(kCryptoJobSync, ...kdfArgs);
62+
const ctorNs = process.hrtime.bigint() - ctorStart;
63+
const runStart = process.hrtime.bigint();
64+
const { 0: err } = job.run();
65+
const runNs = process.hrtime.bigint() - runStart;
66+
assert.strictEqual(err, undefined);
67+
assert.ok(ctorNs * 10n < runNs);
68+
}
69+
70+
// Async: run() dispatches to the thread pool; measure until ondone fires.
71+
{
72+
const ctorStart = process.hrtime.bigint();
73+
const job = new Argon2Job(kCryptoJobAsync, ...kdfArgs);
74+
const ctorNs = process.hrtime.bigint() - ctorStart;
75+
const runStart = process.hrtime.bigint();
76+
job.ondone = common.mustSucceed(() => {
77+
const runNs = process.hrtime.bigint() - runStart;
78+
assert.ok(ctorNs * 10n < runNs);
79+
});
80+
job.run();
81+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.