From cd65dcf8eb78fec2e6def6907f1b76c0bc1ee7d1 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Wed, 30 Sep 2026 13:50:19 +0200 Subject: [PATCH 1/6] crypto: skip secret key mutex allocation Secret key bytes are immutable after KeyObjectData construction. AES, ChaCha20-Poly1305, MAC, and KDF jobs read those bytes without acquiring KeyObjectData::mutex(), and secret-key export and equality do the same. Do not allocate a shared mutex that none of these operations uses. Asymmetric key mutex initialization remains eager so copies continue to share the same lock while asymmetric acquisitions remain. Signed-off-by: Filip Skokan Assisted-by: Codex --- src/crypto/crypto_keys.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index 580f9ac009d6..da56f4a4e264 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -1053,7 +1053,6 @@ KeyObjectData::KeyObjectData(std::nullptr_t) KeyObjectData::KeyObjectData(ByteSource symmetric_key) : key_type_(KeyType::kKeyTypeSecret), - mutex_(std::make_shared()), data_(std::make_shared(std::move(symmetric_key))) {} KeyObjectData::KeyObjectData(KeyType type, EVPKeyPointer&& pkey) From 71df230a8dcf5e121575746079f336abe057a601 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Wed, 30 Sep 2026 14:05:15 +0200 Subject: [PATCH 2/6] crypto: remove RSA-OAEP key locking RSA_Cipher delegates to ncrypto::Rsa with a new EVP_PKEY_CTX per operation. Padding, digests, labels, and output buffers belong to that context. OpenSSL and BoringSSL synchronize RSA blinding and Montgomery caches internally. The lock was introduced for early OpenSSL 3 key downgrading that cleared shared provider fields. Current OpenSSL publishes a separate legacy cache under its own lock. Cover cold shared CryptoKeys and worker clones with concurrent encrypt/decrypt, synchronous KeyObject aliases, and checked plaintexts. Refs: https://github.com/nodejs/node/pull/36825 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan Assisted-by: Codex --- src/crypto/crypto_rsa.cc | 1 - .../parallel/test-webcrypto-rsa-shared-key.js | 109 ++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-webcrypto-rsa-shared-key.js diff --git a/src/crypto/crypto_rsa.cc b/src/crypto/crypto_rsa.cc index e3c594bfbc4e..0a4dbd80ab8d 100644 --- a/src/crypto/crypto_rsa.cc +++ b/src/crypto/crypto_rsa.cc @@ -210,7 +210,6 @@ WebCryptoCipherStatus RSA_Cipher(Environment* env, const ByteSource& in, ByteSource* out) { CHECK_NE(key_data.GetKeyType(), kKeyTypeSecret); - Mutex::ScopedLock lock(key_data.mutex()); const auto& m_pkey = key_data.GetAsymmetricKey(); const ncrypto::Rsa::CipherParams nparams{ .padding = params.padding, diff --git a/test/parallel/test-webcrypto-rsa-shared-key.js b/test/parallel/test-webcrypto-rsa-shared-key.js new file mode 100644 index 000000000000..1813b517ae92 --- /dev/null +++ b/test/parallel/test-webcrypto-rsa-shared-key.js @@ -0,0 +1,109 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { + createPrivateKey, + createPublicKey, + privateDecrypt, + publicEncrypt, +} = require('crypto'); +const { once } = require('events'); +const { Worker, parentPort, workerData } = require('worker_threads'); +const fixtures = require('../common/fixtures'); +const { subtle } = globalThis.crypto; + +const algorithm = { name: 'RSA-OAEP', hash: 'SHA-256' }; +const label = Buffer.from('shared RSA-OAEP key'); +const plaintext = Buffer.from('concurrent key reuse'); + +async function encrypt({ publicKey, publicCryptoKey }) { + const operations = Array.from({ length: 4 }, () => + subtle.encrypt({ name: 'RSA-OAEP', label }, publicCryptoKey, plaintext)); + + // Exercise the synchronous API while the shared CryptoKey jobs are queued. + operations.push(publicEncrypt({ + key: publicKey, + oaepHash: 'sha256', + oaepLabel: label, + }, plaintext)); + + const ciphertexts = await Promise.all(operations); + for (const ciphertext of ciphertexts) + assert.strictEqual(ciphertext.byteLength, 256); + return ciphertexts; +} + +async function decrypt({ privateKey, privateCryptoKey }, ciphertexts) { + const operations = ciphertexts.map((ciphertext) => + subtle.decrypt({ name: 'RSA-OAEP', label }, privateCryptoKey, ciphertext)); + + for (const ciphertext of ciphertexts) { + assert.deepStrictEqual(privateDecrypt({ + key: privateKey, + oaepHash: 'sha256', + oaepLabel: label, + }, Buffer.from(ciphertext)), plaintext); + } + + for (const result of await Promise.all(operations)) + assert.deepStrictEqual(Buffer.from(result), plaintext); +} + +function waitForStart(barrier, phase) { + parentPort.postMessage(phase); + Atomics.wait(barrier, phase, 0); +} + +async function runWorker() { + const barrier = new Int32Array(workerData.barrier); + waitForStart(barrier, 0); + const ciphertexts = await encrypt(workerData.keys); + waitForStart(barrier, 1); + await decrypt(workerData.keys, ciphertexts); +} + +async function runMain() { + const publicKey = createPublicKey(fixtures.readKey('rsa_public_2048.pem')); + const privateKey = createPrivateKey(fixtures.readKey('rsa_private_2048.pem')); + const keys = { + publicKey, + privateKey, + publicCryptoKey: publicKey.toCryptoKey(algorithm, false, ['encrypt']), + privateCryptoKey: privateKey.toCryptoKey(algorithm, false, ['decrypt']), + }; + const barrier = new Int32Array(new SharedArrayBuffer(8)); + const workers = Array.from({ length: 3 }, () => new Worker(__filename, { + workerData: { test: 'rsa-shared-key', keys, barrier: barrier.buffer }, + })); + const exits = workers.map((worker) => once(worker, 'exit')); + + // Clones retain the same backing keys. Release every worker together so + // their first encryption and decryption also exercise cold key state. + const ready = await Promise.all( + workers.map((worker) => once(worker, 'message'))); + for (const [phase] of ready) + assert.strictEqual(phase, 0); + const decryptReady = workers.map((worker) => once(worker, 'message')); + Atomics.store(barrier, 0, 1); + Atomics.notify(barrier, 0); + const ciphertexts = await encrypt(keys); + + for (const [phase] of await Promise.all(decryptReady)) + assert.strictEqual(phase, 1); + Atomics.store(barrier, 1, 1); + Atomics.notify(barrier, 1); + await decrypt(keys, ciphertexts); + + for (const [code] of await Promise.all(exits)) + assert.strictEqual(code, 0); +} + +if (workerData?.test === 'rsa-shared-key') { + runWorker().then(common.mustCall()); +} else { + runMain().then(common.mustCall()); +} From 32b22fdec75d3b5e739809ef3043087a5d07b839 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Wed, 30 Sep 2026 14:05:15 +0200 Subject: [PATCH 3/6] crypto: remove KEM key locking ncrypto::KEM creates a new EVP_PKEY_CTX per operation. Parameters, entropy, scratch buffers, and outputs are private to each call. ML-KEM expansion finishes during import, and operations read an immutable key. EC and X25519 retain read-only recipient keys. RSA synchronizes mutable caches internally. Remove the outer locks serializing jobs that share a KeyObject or CryptoKey. Cover cold keys and worker clones across RSA, EC, X25519, and ML-KEM, checking every resulting shared secret. Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan Assisted-by: Codex --- src/crypto/crypto_kem.cc | 2 - test/parallel/test-crypto-kem-shared-key.js | 140 ++++++++++++++++++++ 2 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-crypto-kem-shared-key.js diff --git a/src/crypto/crypto_kem.cc b/src/crypto/crypto_kem.cc index 726912b57f29..7d3f309157df 100644 --- a/src/crypto/crypto_kem.cc +++ b/src/crypto/crypto_kem.cc @@ -117,7 +117,6 @@ KEMEncapsulateJob::KEMEncapsulateJob(Environment* env, void KEMEncapsulateJob::DoThreadPoolWork() { ncrypto::ClearErrorOnReturn clear_error_on_return; AdditionalParams* params = CryptoJob::params(); - Mutex::ScopedLock lock(params->key.mutex()); auto result = ncrypto::KEM::Encapsulate(params->key.GetAsymmetricKey()); if (result) { out_.emplace(); @@ -223,7 +222,6 @@ bool KEMDecapsulateTraits::DeriveBits(Environment* env, ByteSource* out, CryptoJobMode mode, CryptoErrorStore* errors) { - Mutex::ScopedLock lock(params.key.mutex()); const auto& private_key = params.key.GetAsymmetricKey(); return DoKEMDecapsulate( diff --git a/test/parallel/test-crypto-kem-shared-key.js b/test/parallel/test-crypto-kem-shared-key.js new file mode 100644 index 000000000000..da28d5b41347 --- /dev/null +++ b/test/parallel/test-crypto-kem-shared-key.js @@ -0,0 +1,140 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { + createPrivateKey, + createPublicKey, + encapsulate, + decapsulate, +} = require('crypto'); +const { once } = require('events'); +const { promisify } = require('util'); +const { Worker, parentPort, workerData } = require('worker_threads'); +const { hasOpenSSL, hasFIPS, isBoringSSL } = require('../common/crypto'); +const fixtures = require('../common/fixtures'); +const { subtle } = globalThis.crypto; + +if (!hasOpenSSL(3) && !isBoringSSL) + common.skip('requires OpenSSL >= 3 or BoringSSL'); + +const encapsulateAsync = promisify(encapsulate); +const decapsulateAsync = promisify(decapsulate); + +async function encapsulateKeys(keys) { + const operations = Array.from({ length: 4 }, () => + encapsulateAsync(keys.publicKey)); + operations.push(encapsulate(keys.publicKey)); + if (keys.publicCryptoKey) { + operations.push(subtle.encapsulateBits('ML-KEM-768', keys.publicCryptoKey)); + } + + const results = await Promise.all(operations); + for (const { sharedKey, ciphertext } of results) { + assert.strictEqual(sharedKey.byteLength, keys.sharedSecretLength); + assert.strictEqual(ciphertext.byteLength, keys.ciphertextLength); + } + return results; +} + +async function decapsulateKeys(keys, results) { + const operations = results.map(common.mustCall(async ({ sharedKey, ciphertext }) => { + const result = await decapsulateAsync(keys.privateKey, ciphertext); + assert.deepStrictEqual(result, Buffer.from(sharedKey)); + }, results.length)); + + for (const { sharedKey, ciphertext } of results) { + assert.deepStrictEqual( + decapsulate(keys.privateKey, ciphertext), Buffer.from(sharedKey)); + if (keys.privateCryptoKey) { + operations.push(subtle.decapsulateBits( + 'ML-KEM-768', keys.privateCryptoKey, ciphertext, + ).then((result) => { + assert.deepStrictEqual(Buffer.from(result), Buffer.from(sharedKey)); + })); + } + } + + await Promise.all(operations); +} + +function waitForStart(barrier, phase) { + parentPort.postMessage(phase); + Atomics.wait(barrier, phase, 0); +} + +async function runWorker() { + const barrier = new Int32Array(workerData.barrier); + waitForStart(barrier, 0); + const results = await encapsulateKeys(workerData.keys); + waitForStart(barrier, 1); + await decapsulateKeys(workerData.keys, results); +} + +async function testKeys(keys) { + const barrier = new Int32Array(new SharedArrayBuffer(8)); + const workers = Array.from({ length: 3 }, () => new Worker(__filename, { + workerData: { test: 'kem-shared-key', keys, barrier: barrier.buffer }, + })); + const exits = workers.map((worker) => once(worker, 'exit')); + + // Synchronize each phase before the backing keys have been used for it. + const ready = await Promise.all( + workers.map((worker) => once(worker, 'message'))); + for (const [phase] of ready) + assert.strictEqual(phase, 0); + const decapsulateReady = workers.map((worker) => once(worker, 'message')); + Atomics.store(barrier, 0, 1); + Atomics.notify(barrier, 0); + const results = await encapsulateKeys(keys); + + for (const [phase] of await Promise.all(decapsulateReady)) + assert.strictEqual(phase, 1); + Atomics.store(barrier, 1, 1); + Atomics.notify(barrier, 1); + await decapsulateKeys(keys, results); + + for (const [code] of await Promise.all(exits)) + assert.strictEqual(code, 0); +} + +async function runMain() { + const types = []; + if (hasOpenSSL(3)) { + types.push(['rsa', 'rsa', 256, 256]); + } + if (hasOpenSSL(3, 2) && !hasFIPS(3)) { + types.push(['ec', 'ec_p256', 32, 65], ['x25519', 'x25519', 32, 32]); + } + if (hasOpenSSL(3, 5) || isBoringSSL) { + types.push(['ml-kem-768', 'ml_kem_768', 32, 1088]); + } + + for (const [type, prefix, sharedSecretLength, ciphertextLength] of types) { + const suffix = type === 'rsa' ? '_2048' : ''; + const privateSuffix = type === 'ml-kem-768' ? '_seed_only' : suffix; + const publicKey = createPublicKey(fixtures.readKey( + `${prefix}_public${suffix}.pem`)); + const privateKey = createPrivateKey(fixtures.readKey( + `${prefix}_private${privateSuffix}.pem`)); + const keys = { + publicKey, privateKey, sharedSecretLength, ciphertextLength, + }; + if (type === 'ml-kem-768') { + keys.publicCryptoKey = publicKey.toCryptoKey( + 'ML-KEM-768', false, ['encapsulateBits']); + keys.privateCryptoKey = privateKey.toCryptoKey( + 'ML-KEM-768', false, ['decapsulateBits']); + } + await testKeys(keys); + } +} + +if (workerData?.test === 'kem-shared-key') { + runWorker().then(common.mustCall()); +} else { + runMain().then(common.mustCall()); +} From 3f462893b2b43675617cd271b1e414cd9fd3a0e0 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Wed, 30 Sep 2026 14:05:16 +0200 Subject: [PATCH 4/6] crypto: remove verify setup key locking Verify configuration only copies signature bytes and reads DSA/EC order width for P1363 conversion. Conversion constructs independent DER and BIGNUM values. Signing and verification already run without the mutex using separate operation contexts. Cover cold shared public keys and worker clones with checked P1363 verification while signatures, derivation, exports, metadata queries, and equality checks overlap. Compare source PKCS8 encoding after workers finish. OpenSSL's ordinary EC PKCS8 encoder temporarily changes source flags and races even on the unmodified base. Keep WebCrypto PKCS8 exports concurrent through their independent clones. Refs: https://github.com/nodejs/node/pull/36825 Refs: https://github.com/nodejs/node/pull/37816 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan Assisted-by: Codex --- src/crypto/crypto_sig.cc | 1 - .../test-crypto-key-reuse-concurrent.js | 242 ++++++++++++++++++ 2 files changed, 242 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-crypto-key-reuse-concurrent.js diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc index 1e003e9e51b9..400dce70ad22 100644 --- a/src/crypto/crypto_sig.cc +++ b/src/crypto/crypto_sig.cc @@ -712,7 +712,6 @@ Maybe SignTraits::AdditionalConfig( } // If this is an EC key (assuming ECDSA) we need to convert the // the signature from WebCrypto format into DER format... - Mutex::ScopedLock lock(params->key.mutex()); const auto& akey = params->key.GetAsymmetricKey(); if (UseP1363Encoding(akey, params->dsa_encoding)) { params->signature = ConvertSignatureToDER(akey, signature.ToByteSource()); diff --git a/test/parallel/test-crypto-key-reuse-concurrent.js b/test/parallel/test-crypto-key-reuse-concurrent.js new file mode 100644 index 000000000000..ae72fe7f84b2 --- /dev/null +++ b/test/parallel/test-crypto-key-reuse-concurrent.js @@ -0,0 +1,242 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { + createPrivateKey, + createPublicKey, + diffieHellman, + generateKeyPairSync, + KeyObject, + sign, + verify, +} = require('crypto'); +const { once } = require('events'); +const { promisify } = require('util'); +const { Worker, parentPort, workerData } = require('worker_threads'); +const fixtures = require('../common/fixtures'); +const { subtle } = globalThis.crypto; +const signAsync = promisify(sign); +const verifyAsync = promisify(verify); +const ecdsa = { name: 'ECDSA', hash: 'SHA-256' }; +const pkcs8 = { type: 'pkcs8', format: 'der' }; +const spki = { type: 'spki', format: 'der' }; +const iterations = 16; + +async function exercise({ keys, peer, expected }) { + // Check generated signatures and exports with independent imports that do + // not warm the shared key's provider caches before its first operation. + const referencePrivate = createPrivateKey({ + key: expected.originalPrivate, ...pkcs8, + }); + const referencePublic = createPublicKey({ key: expected.public, ...spki }); + + for (let i = 0; i < iterations; i++) { + const data = Buffer.from(`shared key operation ${i}`); + const referenceSignature = Buffer.from(expected.signatures[i]); + const pending = []; + for (let j = 0; j < 4; j++) { + pending.push( + verifyAsync('sha256', data, { + key: keys.publicKey, + dsaEncoding: 'ieee-p1363', + }, referenceSignature).then((valid) => { + assert.strictEqual(valid, true); + }), + signAsync('sha256', data, keys.privateKey).then((signature) => { + assert.strictEqual( + verify('sha256', data, referencePublic, signature), true); + }), + ); + if (keys.ecdsaPrivate === undefined) + continue; + pending.push( + subtle.verify(ecdsa, keys.ecdsaPublic, referenceSignature, data) + .then((valid) => { + assert.strictEqual(valid, true); + }), + subtle.sign(ecdsa, keys.ecdsaPrivate, data).then((signature) => { + assert.strictEqual(verify('sha256', data, { + key: referencePublic, + dsaEncoding: 'ieee-p1363', + }, Buffer.from(signature)), true); + }), + subtle.deriveBits({ name: 'ECDH', public: peer }, keys.ecdhPrivate, 256) + .then((bits) => { + assert.deepStrictEqual( + Buffer.from(bits), Buffer.from(expected.bits)); + }), + ); + } + + // Export, compare, and query metadata while jobs use this same native key + // in the threadpool and other Workers. Fresh wrappers also exercise the + // native metadata getters on every iteration rather than their JS caches. + const privateKey = keys.ecdsaPrivate === undefined ? + structuredClone(keys.privateKey) : KeyObject.from(keys.ecdsaPrivate); + const publicKey = keys.ecdsaPublic === undefined ? + structuredClone(keys.publicKey) : KeyObject.from(keys.ecdsaPublic); + assert.strictEqual(privateKey.equals(referencePrivate), true); + assert.strictEqual(publicKey.equals(referencePublic), true); + assert.strictEqual(privateKey.equals(keys.privateKey), true); + assert.strictEqual(publicKey.equals(keys.publicKey), true); + assert.deepStrictEqual(privateKey.asymmetricKeyDetails, { + namedCurve: 'prime256v1', + }); + assert.deepStrictEqual(publicKey.asymmetricKeyDetails, { + namedCurve: 'prime256v1', + }); + assert.deepStrictEqual( + privateKey.export({ format: 'jwk' }), expected.privateJwk); + assert.deepStrictEqual( + publicKey.export({ format: 'jwk' }), expected.publicJwk); + assert.deepStrictEqual( + privateKey.export({ format: 'raw-private' }), + Buffer.from(expected.rawPrivate)); + assert.deepStrictEqual( + publicKey.export({ format: 'raw-public' }), Buffer.from(expected.rawPublic)); + assert.deepStrictEqual(publicKey.export({ + format: 'raw-public', type: 'compressed', + }), Buffer.from(expected.compressedPublic)); + assert.deepStrictEqual( + publicKey.export(spki), Buffer.from(expected.public)); + + if (keys.ecdsaPrivate === undefined) { + assert.deepStrictEqual(diffieHellman({ + privateKey, publicKey: peer, + }), Buffer.from(expected.bits)); + await Promise.all(pending); + continue; + } + + for (const key of [keys.ecdsaPrivate, keys.ecdhPrivate]) { + pending.push(subtle.exportKey('pkcs8', key).then((encoded) => { + assert.deepStrictEqual( + Buffer.from(encoded), Buffer.from(expected.private)); + })); + } + pending.push( + subtle.exportKey('raw', keys.ecdsaPublic).then((encoded) => { + assert.deepStrictEqual( + Buffer.from(encoded), Buffer.from(expected.rawPublic)); + }), + subtle.exportKey('spki', keys.ecdsaPublic).then((encoded) => { + assert.deepStrictEqual( + Buffer.from(encoded), Buffer.from(expected.public)); + }), + subtle.exportKey('jwk', keys.ecdsaPrivate).then((jwk) => { + assert.deepStrictEqual(jwk, { + ...expected.privateJwk, key_ops: ['sign'], ext: true, + }); + }), + ); + await Promise.all(pending); + } +} + +if (workerData?.sharedKeyTest) { + parentPort.postMessage('ready'); + Atomics.wait(workerData.barrier, 0, 0); + exercise(workerData).then(common.mustCall()); +} else { + function der(tag, ...parts) { + const body = Buffer.concat(parts); + assert(body.length < 128); + return Buffer.concat([Buffer.from([tag, body.length]), body]); + } + + async function run(input, peer, expected, webcrypto = true) { + // The KeyObject-only case performs no operations before the barrier, + // exercising concurrent first use of the shared key. CryptoKey conversion + // validates the key before its first concurrent sign/derive/export calls. + const privateKey = createPrivateKey({ key: input, ...pkcs8 }); + const publicKey = createPublicKey(privateKey); + const keys = { privateKey, publicKey }; + if (webcrypto) { + keys.ecdsaPrivate = privateKey.toCryptoKey({ + name: 'ECDSA', namedCurve: 'P-256', + }, true, ['sign']); + keys.ecdsaPublic = publicKey.toCryptoKey({ + name: 'ECDSA', namedCurve: 'P-256', + }, true, ['verify']); + keys.ecdhPrivate = privateKey.toCryptoKey({ + name: 'ECDH', namedCurve: 'P-256', + }, true, ['deriveBits']); + } + const barrier = new Int32Array(new SharedArrayBuffer(4)); + const data = { sharedKeyTest: true, keys, peer, expected, barrier }; + const ready = []; + const exited = []; + for (let i = 0; i < 3; i++) { + const worker = new Worker(__filename, { workerData: data }); + ready.push(once(worker, 'message').then(([message]) => { + assert.strictEqual(message, 'ready'); + })); + exited.push(once(worker, 'exit').then(common.mustCall(([code]) => { + assert.strictEqual(code, 0); + }))); + } + await Promise.all(ready); + Atomics.store(barrier, 0, 1); + Atomics.notify(barrier, 0); + await Promise.all([exercise(data), ...exited]); + + // Ordinary PKCS8 encoding temporarily changes EC encoding flags in some + // OpenSSL versions, so compare only after all shared-key users finish. + // WebCrypto export must add the public point on a copy and leave the + // original key's encoding flags unchanged. + assert.deepStrictEqual( + privateKey.export(pkcs8), Buffer.from(expected.originalPrivate)); + } + + (async () => { + const reference = createPrivateKey(fixtures.readKey('ec_p256_private.pem')); + const publicKey = createPublicKey(reference); + const { publicKey: peer } = generateKeyPairSync('ec', { + namedCurve: 'prime256v1', + }); + const privateJwk = reference.export({ format: 'jwk' }); + const expected = { + private: reference.export(pkcs8), + public: publicKey.export(spki), + privateJwk, + publicJwk: publicKey.export({ format: 'jwk' }), + rawPrivate: reference.export({ format: 'raw-private' }), + rawPublic: publicKey.export({ format: 'raw-public' }), + compressedPublic: publicKey.export({ + format: 'raw-public', type: 'compressed', + }), + bits: diffieHellman({ privateKey: reference, publicKey: peer }), + signatures: Array.from({ length: iterations }, (_, i) => { + return sign('sha256', Buffer.from(`shared key operation ${i}`), { + key: reference, + dsaEncoding: 'ieee-p1363', + }); + }), + }; + const cryptoPeer = peer.toCryptoKey({ + name: 'ECDH', namedCurve: 'P-256', + }, true, []); + await run(expected.private, peer, { + ...expected, originalPrivate: expected.private, + }, false); + await run(expected.private, cryptoPeer, { + ...expected, originalPrivate: expected.private, + }); + + const algorithmIdentifier = der(0x30, Buffer.from( + '06072a8648ce3d020106082a8648ce3d030107', 'hex')); + const ecPrivateKey = der( + 0x30, Buffer.from('020101', 'hex'), + der(0x04, Buffer.from(privateJwk.d, 'base64url'))); + const privateOnly = der( + 0x30, Buffer.from('020100', 'hex'), algorithmIdentifier, + der(0x04, ecPrivateKey)); + const originalPrivate = createPrivateKey({ key: privateOnly, ...pkcs8 }) + .export(pkcs8); + await run(privateOnly, cryptoPeer, { ...expected, originalPrivate }); + })().then(common.mustCall()); +} From 286570b5daea20c3bcd4b49c978029ee26327ae8 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Wed, 30 Sep 2026 14:05:16 +0200 Subject: [PATCH 5/6] crypto: remove key export and metadata locks Raw and JWK getters return independently owned bytes or BIGNUMs. RSA metadata snapshots public parameters and PSS restrictions. EC fallback paths reconstruct or duplicate the source key. EC PKCS8 export changes encoding flags only on its own clone. Early OpenSSL 3 key downgrading mutated shared provider fields. Current OpenSSL publishes separate legacy and provider conversion caches under internal read/write locks. Ordinary DER/PEM export, equality, signing, and key derivation already access the same keys without these locks. Remove export and metadata acquisitions, including their coverage of V8 allocation and encoding, and remove the now-unused shared mutex storage. Retain shared immutable key ownership. The concurrent reuse test checks that EC PKCS8 export leaves the original flags intact. Refs: https://github.com/nodejs/node/pull/36825 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan Assisted-by: Codex --- src/crypto/crypto_ec.cc | 2 -- src/crypto/crypto_keys.cc | 19 +------------------ src/crypto/crypto_keys.h | 17 ++++++----------- src/crypto/crypto_rsa.cc | 2 -- 4 files changed, 7 insertions(+), 33 deletions(-) diff --git a/src/crypto/crypto_ec.cc b/src/crypto/crypto_ec.cc index 1126d72ad5f2..f51d311846cd 100644 --- a/src/crypto/crypto_ec.cc +++ b/src/crypto/crypto_ec.cc @@ -479,7 +479,6 @@ Maybe EcKeyGenTraits::AdditionalConfig( bool ExportJWKEcKey(Environment* env, const KeyObjectData& key, Local target) { - Mutex::ScopedLock lock(key.mutex()); const auto& m_pkey = key.GetAsymmetricKey(); DCHECK(m_pkey.isA(KeyAlgorithm::EC)); @@ -624,7 +623,6 @@ KeyObjectData ImportJWKEcKey(Environment* env, Local jwk) { bool GetEcKeyDetail(Environment* env, const KeyObjectData& key, Local target) { - Mutex::ScopedLock lock(key.mutex()); const auto& m_pkey = key.GetAsymmetricKey(); DCHECK(m_pkey.isA(KeyAlgorithm::EC)); diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index da56f4a4e264..64bbd6da072a 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -187,7 +187,6 @@ KeyObjectData ImportJWKSecretKey(Environment* env, Local jwk) { static bool ExportJWKRawKey(Environment* env, const KeyObjectData& key, Local target) { - Mutex::ScopedLock lock(key.mutex()); auto result = key.GetAsymmetricKey().exportRawJwk(key.GetKeyType() == kKeyTypePrivate); if (!result) { @@ -416,7 +415,6 @@ bool KeyObjectData::ToEncodedPublicKey( return ExportJWKInner( env, addRefWithType(KeyType::kKeyTypePublic), *out, false); } else if (config.format == EVPKeyPointer::PKFormatType::RAW_PUBLIC) { - Mutex::ScopedLock lock(mutex()); const auto& pkey = GetAsymmetricKey(); const auto* algorithm = pkey.getAlgorithm(); if (algorithm == &KeyAlgorithm::EC) { @@ -471,7 +469,6 @@ bool KeyObjectData::ToEncodedPrivateKey( return ExportJWKInner( env, addRefWithType(KeyType::kKeyTypePrivate), *out, false); } else if (config.format == EVPKeyPointer::PKFormatType::RAW_PRIVATE) { - Mutex::ScopedLock lock(mutex()); const auto& pkey = GetAsymmetricKey(); const auto* algorithm = pkey.getAlgorithm(); if (algorithm == &KeyAlgorithm::EC) { @@ -497,7 +494,6 @@ bool KeyObjectData::ToEncodedPrivateKey( return Buffer::Copy(env, raw_data.get(), raw_data.size()) .ToLocal(out); } else if (config.format == EVPKeyPointer::PKFormatType::RAW_SEED) { - Mutex::ScopedLock lock(mutex()); const auto& pkey = GetAsymmetricKey(); auto raw_data = pkey.rawSeed(); if (!raw_data) { @@ -1056,9 +1052,7 @@ KeyObjectData::KeyObjectData(ByteSource symmetric_key) data_(std::make_shared(std::move(symmetric_key))) {} KeyObjectData::KeyObjectData(KeyType type, EVPKeyPointer&& pkey) - : key_type_(type), - mutex_(std::make_shared()), - data_(std::make_shared(std::move(pkey))) {} + : key_type_(type), data_(std::make_shared(std::move(pkey))) {} void KeyObjectData::Data::MemoryInfo(MemoryTracker* tracker) const { if (asymmetric_key) { @@ -1075,11 +1069,6 @@ void KeyObjectData::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("data", data_); } -Mutex& KeyObjectData::mutex() const { - if (!mutex_) mutex_ = std::make_shared(); - return *mutex_.get(); -} - KeyObjectData KeyObjectData::CreateSecret(ByteSource key) { return KeyObjectData(std::move(key)); } @@ -1474,7 +1463,6 @@ void KeyObjectHandle::RawPublicKey( const KeyObjectData& data = key->Data(); CHECK_NE(data.GetKeyType(), kKeyTypeSecret); - Mutex::ScopedLock lock(data.mutex()); const auto& pkey = data.GetAsymmetricKey(); const bool is_raw_supported = pkey.supportsRawPublic(); @@ -1502,7 +1490,6 @@ void KeyObjectHandle::RawPrivateKey( const KeyObjectData& data = key->Data(); CHECK_EQ(data.GetKeyType(), kKeyTypePrivate); - Mutex::ScopedLock lock(data.mutex()); const auto& pkey = data.GetAsymmetricKey(); const bool is_raw_supported = pkey.supportsRawPrivate(); @@ -1530,7 +1517,6 @@ void KeyObjectHandle::ExportECPublicRaw( const KeyObjectData& data = key->Data(); CHECK_NE(data.GetKeyType(), kKeyTypeSecret); - Mutex::ScopedLock lock(data.mutex()); const auto& m_pkey = data.GetAsymmetricKey(); if (!m_pkey.isA(KeyAlgorithm::EC)) { return THROW_ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS(env); @@ -1569,7 +1555,6 @@ void KeyObjectHandle::ExportECPrivateRaw( const KeyObjectData& data = key->Data(); CHECK_EQ(data.GetKeyType(), kKeyTypePrivate); - Mutex::ScopedLock lock(data.mutex()); const auto& m_pkey = data.GetAsymmetricKey(); if (!m_pkey.isA(KeyAlgorithm::EC)) { return THROW_ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS(env); @@ -1592,7 +1577,6 @@ void KeyObjectHandle::ExportECPrivatePkcs8( ASSIGN_OR_RETURN_UNWRAP(&key, args.This()); const KeyObjectData& data = key->Data(); CHECK_EQ(data.GetKeyType(), kKeyTypePrivate); - Mutex::ScopedLock lock(data.mutex()); auto encoded = ncrypto::Ec::ExportPrivatePkcs8(data.GetAsymmetricKey()); if (!encoded) { return THROW_ERR_CRYPTO_OPERATION_FAILED(env, @@ -1611,7 +1595,6 @@ void KeyObjectHandle::RawSeed(const v8::FunctionCallbackInfo& args) { const KeyObjectData& data = key->Data(); CHECK_EQ(data.GetKeyType(), kKeyTypePrivate); - Mutex::ScopedLock lock(data.mutex()); const auto& pkey = data.GetAsymmetricKey(); auto raw_data = pkey.rawSeed(); diff --git a/src/crypto/crypto_keys.h b/src/crypto/crypto_keys.h index cb7209d0835d..5a14651deaa5 100644 --- a/src/crypto/crypto_keys.h +++ b/src/crypto/crypto_keys.h @@ -54,8 +54,8 @@ class KeyObjectData final : public MemoryRetainer { KeyType GetKeyType() const; - // These functions allow unprotected access to the raw key material and should - // only be used to implement cryptographic operations requiring the key. + // The key material is immutable and can be used concurrently by operations + // with separate contexts. const ncrypto::EVPKeyPointer& GetAsymmetricKey() const; const char* GetSymmetricKey() const; size_t GetSymmetricKeySize() const; @@ -64,8 +64,6 @@ class KeyObjectData final : public MemoryRetainer { SET_MEMORY_INFO_NAME(KeyObjectData) SET_SELF_SIZE(KeyObjectData) - Mutex& mutex() const; - static v8::Maybe GetPublicKeyEncodingFromJs(const v8::FunctionCallbackInfo& args, unsigned int* offset, @@ -97,11 +95,11 @@ class KeyObjectData final : public MemoryRetainer { v8::Local* out); inline KeyObjectData addRef() const { - return KeyObjectData(key_type_, mutex_, data_); + return KeyObjectData(key_type_, data_); } inline KeyObjectData addRefWithType(KeyType type) const { - return KeyObjectData(type, mutex_, data_); + return KeyObjectData(type, data_); } private: @@ -115,7 +113,6 @@ class KeyObjectData final : public MemoryRetainer { const char* default_msg); KeyType key_type_; - mutable std::shared_ptr mutex_; struct Data final : public MemoryRetainer { const ByteSource symmetric_key; @@ -131,10 +128,8 @@ class KeyObjectData final : public MemoryRetainer { }; std::shared_ptr data_; - KeyObjectData(KeyType type, - std::shared_ptr mutex, - std::shared_ptr data) - : key_type_(type), mutex_(std::move(mutex)), data_(std::move(data)) {} + KeyObjectData(KeyType type, std::shared_ptr data) + : key_type_(type), data_(std::move(data)) {} }; class KeyObjectHandle : public BaseObject { diff --git a/src/crypto/crypto_rsa.cc b/src/crypto/crypto_rsa.cc index 0a4dbd80ab8d..25d8808e6ead 100644 --- a/src/crypto/crypto_rsa.cc +++ b/src/crypto/crypto_rsa.cc @@ -298,7 +298,6 @@ WebCryptoCipherStatus RSACipherTraits::DoCipher(Environment* env, bool ExportJWKRsaKey(Environment* env, const KeyObjectData& key, Local target) { - Mutex::ScopedLock lock(key.mutex()); const auto& m_pkey = key.GetAsymmetricKey(); const ncrypto::Rsa rsa = m_pkey; @@ -526,7 +525,6 @@ KeyObjectData ImportJWKRsaKey(Environment* env, Local jwk) { bool GetRsaKeyDetail(Environment* env, const KeyObjectData& key, Local target) { - Mutex::ScopedLock lock(key.mutex()); const auto& m_pkey = key.GetAsymmetricKey(); const auto rsa = ncrypto::Rsa::PublicOnly(m_pkey); From 61f898263edee589130c41d3681195b638cc4ac2 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Thu, 1 Oct 2026 08:43:05 +0200 Subject: [PATCH 6/6] crypto: isolate provider EC PKCS8 encoding OpenSSL's provider ec_pki_priv_to_der() temporarily changes EC_KEY encoding flags on the input key. Concurrent PKCS8 exports can emit inconsistent DER or affect a concurrent SEC1 export. The old Node key mutex never covered this path. Encode provider EC and SM2 keys through EVP_PKEY_dup(), preserving encoding flags, point conversion form, parameters, and provider. Legacy OpenSSL and BoringSSL already encode using local flags. Restore concurrent KeyObject PKCS8 DER assertions and cover PEM and SEC1 exports, including ECPrivateKey inputs without a public point. Signed-off-by: Filip Skokan Assisted-by: Codex --- deps/ncrypto/ncrypto.cc | 19 +++++++++++++++++-- .../test-crypto-key-reuse-concurrent.js | 16 ++++++++++++---- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc index 44b74e13fd2b..1dbb6f434182 100644 --- a/deps/ncrypto/ncrypto.cc +++ b/deps/ncrypto/ncrypto.cc @@ -4401,11 +4401,26 @@ Result EVPKeyPointer::writePrivateKey( break; } case PKEncodingType::PKCS8: { + EVP_PKEY* export_key = get(); +#if NCRYPTO_USE_OPENSSL3_PROVIDER + // OpenSSL's provider EC PKCS8 encoders temporarily change encoding flags. + // Use an independent key so concurrent exports do not change the source. + EVPKeyPointer key_copy; + if ((isA(KeyAlgorithm::EC) || isA(KeyAlgorithm::SM2)) && + EVP_PKEY_get0_provider(get()) != nullptr) { + key_copy.reset(EVP_PKEY_dup(get())); + if (!key_copy) { + return Result(false, + mark_pop_error_on_return.peekError()); + } + export_key = key_copy.get(); + } +#endif switch (config.format) { case PKFormatType::PEM: { // Encode PKCS#8 as PEM. err = PEM_write_bio_PKCS8PrivateKey(bio.get(), - get(), + export_key, config.cipher, passphrase.data, passphrase.len, @@ -4415,7 +4430,7 @@ Result EVPKeyPointer::writePrivateKey( } case PKFormatType::DER: { err = i2d_PKCS8PrivateKey_bio(bio.get(), - get(), + export_key, config.cipher, passphrase.data, passphrase.len, diff --git a/test/parallel/test-crypto-key-reuse-concurrent.js b/test/parallel/test-crypto-key-reuse-concurrent.js index ae72fe7f84b2..6c16fb88f0d4 100644 --- a/test/parallel/test-crypto-key-reuse-concurrent.js +++ b/test/parallel/test-crypto-key-reuse-concurrent.js @@ -23,6 +23,8 @@ const signAsync = promisify(sign); const verifyAsync = promisify(verify); const ecdsa = { name: 'ECDSA', hash: 'SHA-256' }; const pkcs8 = { type: 'pkcs8', format: 'der' }; +const pkcs8Pem = { type: 'pkcs8', format: 'pem' }; +const sec1 = { type: 'sec1', format: 'der' }; const spki = { type: 'spki', format: 'der' }; const iterations = 16; @@ -33,6 +35,8 @@ async function exercise({ keys, peer, expected }) { key: expected.originalPrivate, ...pkcs8, }); const referencePublic = createPublicKey({ key: expected.public, ...spki }); + const referencePem = referencePrivate.export(pkcs8Pem); + const referenceSec1 = referencePrivate.export(sec1); for (let i = 0; i < iterations; i++) { const data = Buffer.from(`shared key operation ${i}`); @@ -103,6 +107,10 @@ async function exercise({ keys, peer, expected }) { }), Buffer.from(expected.compressedPublic)); assert.deepStrictEqual( publicKey.export(spki), Buffer.from(expected.public)); + assert.deepStrictEqual( + privateKey.export(pkcs8), Buffer.from(expected.originalPrivate)); + assert.strictEqual(privateKey.export(pkcs8Pem), referencePem); + assert.deepStrictEqual(privateKey.export(sec1), referenceSec1); if (keys.ecdsaPrivate === undefined) { assert.deepStrictEqual(diffieHellman({ @@ -184,12 +192,12 @@ if (workerData?.sharedKeyTest) { Atomics.notify(barrier, 0); await Promise.all([exercise(data), ...exited]); - // Ordinary PKCS8 encoding temporarily changes EC encoding flags in some - // OpenSSL versions, so compare only after all shared-key users finish. - // WebCrypto export must add the public point on a copy and leave the - // original key's encoding flags unchanged. + // PKCS8 export must leave the source encoding flags unchanged, including + // whether SEC1 includes parameters and whether the public point is omitted. assert.deepStrictEqual( privateKey.export(pkcs8), Buffer.from(expected.originalPrivate)); + assert.deepStrictEqual(privateKey.export(sec1), + createPrivateKey({ key: input, ...pkcs8 }).export(sec1)); } (async () => {