Skip to content

Commit 4afe009

Browse files
committed
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: #36825 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex PR-URL: #66413 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 138ed26 commit 4afe009

5 files changed

Lines changed: 103 additions & 41 deletions

File tree

‎src/crypto/crypto_ec.cc‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,6 @@ WebCryptoKeyExportStatus EC_Raw_Export(const KeyObjectData& key_data,
485485
ByteSource* out) {
486486
const auto& m_pkey = key_data.GetAsymmetricKey();
487487
CHECK(m_pkey);
488-
Mutex::ScopedLock lock(key_data.mutex());
489488

490489
if (!m_pkey.isA(KeyAlgorithm::EC)) {
491490
switch (key_data.GetKeyType()) {
@@ -573,7 +572,6 @@ WebCryptoKeyExportStatus ECKeyExportTraits::DoExport(
573572
// the header but it is a somewhat silly hoop to jump through because
574573
// the header is for all practical purposes a static 26 byte sequence
575574
// where only the second byte changes.
576-
Mutex::ScopedLock lock(key_data.mutex());
577575
ECKeyPointer ec_key(m_pkey);
578576
if (!ec_key) return WebCryptoKeyExportStatus::FAILED;
579577
const auto group = ec_key.getGroup();
@@ -617,7 +615,6 @@ WebCryptoKeyExportStatus ECKeyExportTraits::DoExport(
617615
bool ExportJWKEcKey(Environment* env,
618616
const KeyObjectData& key,
619617
Local<Object> target) {
620-
Mutex::ScopedLock lock(key.mutex());
621618
const auto& m_pkey = key.GetAsymmetricKey();
622619
DCHECK(m_pkey.isA(KeyAlgorithm::EC));
623620

@@ -762,7 +759,6 @@ KeyObjectData ImportJWKEcKey(Environment* env, Local<Object> jwk) {
762759
bool GetEcKeyDetail(Environment* env,
763760
const KeyObjectData& key,
764761
Local<Object> target) {
765-
Mutex::ScopedLock lock(key.mutex());
766762
const auto& m_pkey = key.GetAsymmetricKey();
767763
DCHECK(m_pkey.isA(KeyAlgorithm::EC));
768764

‎src/crypto/crypto_keys.cc‎

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,6 @@ KeyObjectData ImportJWKSecretKey(Environment* env, Local<Object> jwk) {
188188
static bool ExportJWKRawKey(Environment* env,
189189
const KeyObjectData& key,
190190
Local<Object> target) {
191-
Mutex::ScopedLock lock(key.mutex());
192191
auto result =
193192
key.GetAsymmetricKey().exportRawJwk(key.GetKeyType() == kKeyTypePrivate);
194193
if (!result) {
@@ -417,7 +416,6 @@ bool KeyObjectData::ToEncodedPublicKey(
417416
return ExportJWKInner(
418417
env, addRefWithType(KeyType::kKeyTypePublic), *out, false);
419418
} else if (config.format == EVPKeyPointer::PKFormatType::RAW_PUBLIC) {
420-
Mutex::ScopedLock lock(mutex());
421419
const auto& pkey = GetAsymmetricKey();
422420
const auto* algorithm = pkey.getAlgorithm();
423421
if (algorithm == &KeyAlgorithm::EC) {
@@ -472,7 +470,6 @@ bool KeyObjectData::ToEncodedPrivateKey(
472470
return ExportJWKInner(
473471
env, addRefWithType(KeyType::kKeyTypePrivate), *out, false);
474472
} else if (config.format == EVPKeyPointer::PKFormatType::RAW_PRIVATE) {
475-
Mutex::ScopedLock lock(mutex());
476473
const auto& pkey = GetAsymmetricKey();
477474
const auto* algorithm = pkey.getAlgorithm();
478475
if (algorithm == &KeyAlgorithm::EC) {
@@ -498,7 +495,6 @@ bool KeyObjectData::ToEncodedPrivateKey(
498495
return Buffer::Copy(env, raw_data.get<const char>(), raw_data.size())
499496
.ToLocal(out);
500497
} else if (config.format == EVPKeyPointer::PKFormatType::RAW_SEED) {
501-
Mutex::ScopedLock lock(mutex());
502498
const auto& pkey = GetAsymmetricKey();
503499
auto raw_data = pkey.rawSeed();
504500
if (!raw_data) {
@@ -1057,9 +1053,7 @@ KeyObjectData::KeyObjectData(ByteSource symmetric_key)
10571053
data_(std::make_shared<Data>(std::move(symmetric_key))) {}
10581054

10591055
KeyObjectData::KeyObjectData(KeyType type, EVPKeyPointer&& pkey)
1060-
: key_type_(type),
1061-
mutex_(std::make_shared<Mutex>()),
1062-
data_(std::make_shared<Data>(std::move(pkey))) {}
1056+
: key_type_(type), data_(std::make_shared<Data>(std::move(pkey))) {}
10631057

10641058
void KeyObjectData::Data::MemoryInfo(MemoryTracker* tracker) const {
10651059
if (asymmetric_key) {
@@ -1076,11 +1070,6 @@ void KeyObjectData::MemoryInfo(MemoryTracker* tracker) const {
10761070
tracker->TrackField("data", data_);
10771071
}
10781072

1079-
Mutex& KeyObjectData::mutex() const {
1080-
if (!mutex_) mutex_ = std::make_shared<Mutex>();
1081-
return *mutex_.get();
1082-
}
1083-
10841073
KeyObjectData KeyObjectData::CreateSecret(ByteSource key) {
10851074
return KeyObjectData(std::move(key));
10861075
}
@@ -1475,7 +1464,6 @@ void KeyObjectHandle::RawPublicKey(
14751464
const KeyObjectData& data = key->Data();
14761465
CHECK_NE(data.GetKeyType(), kKeyTypeSecret);
14771466

1478-
Mutex::ScopedLock lock(data.mutex());
14791467
const auto& pkey = data.GetAsymmetricKey();
14801468

14811469
const bool is_raw_supported = pkey.supportsRawPublic();
@@ -1503,7 +1491,6 @@ void KeyObjectHandle::RawPrivateKey(
15031491
const KeyObjectData& data = key->Data();
15041492
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
15051493

1506-
Mutex::ScopedLock lock(data.mutex());
15071494
const auto& pkey = data.GetAsymmetricKey();
15081495

15091496
const bool is_raw_supported = pkey.supportsRawPrivate();
@@ -1531,7 +1518,6 @@ void KeyObjectHandle::ExportECPublicRaw(
15311518
const KeyObjectData& data = key->Data();
15321519
CHECK_NE(data.GetKeyType(), kKeyTypeSecret);
15331520

1534-
Mutex::ScopedLock lock(data.mutex());
15351521
const auto& m_pkey = data.GetAsymmetricKey();
15361522
if (!m_pkey.isA(KeyAlgorithm::EC)) {
15371523
return THROW_ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS(env);
@@ -1570,7 +1556,6 @@ void KeyObjectHandle::ExportECPrivateRaw(
15701556
const KeyObjectData& data = key->Data();
15711557
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
15721558

1573-
Mutex::ScopedLock lock(data.mutex());
15741559
const auto& m_pkey = data.GetAsymmetricKey();
15751560
if (!m_pkey.isA(KeyAlgorithm::EC)) {
15761561
return THROW_ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS(env);
@@ -1593,7 +1578,6 @@ void KeyObjectHandle::ExportECPrivatePkcs8(
15931578
ASSIGN_OR_RETURN_UNWRAP(&key, args.This());
15941579
const KeyObjectData& data = key->Data();
15951580
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
1596-
Mutex::ScopedLock lock(data.mutex());
15971581
auto encoded = ncrypto::Ec::ExportPrivatePkcs8(data.GetAsymmetricKey());
15981582
if (!encoded) {
15991583
return THROW_ERR_CRYPTO_OPERATION_FAILED(env,
@@ -1612,7 +1596,6 @@ void KeyObjectHandle::RawSeed(const v8::FunctionCallbackInfo<v8::Value>& args) {
16121596
const KeyObjectData& data = key->Data();
16131597
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
16141598

1615-
Mutex::ScopedLock lock(data.mutex());
16161599
const auto& pkey = data.GetAsymmetricKey();
16171600

16181601
auto raw_data = pkey.rawSeed();
@@ -1800,7 +1783,6 @@ std::unique_ptr<worker::TransferData> NativeKeyObject::CloneForMessaging()
18001783
WebCryptoKeyExportStatus PKEY_SPKI_Export(const KeyObjectData& key_data,
18011784
ByteSource* out) {
18021785
CHECK_EQ(key_data.GetKeyType(), kKeyTypePublic);
1803-
Mutex::ScopedLock lock(key_data.mutex());
18041786
auto bio = key_data.GetAsymmetricKey().derPublicKey();
18051787
if (!bio) return WebCryptoKeyExportStatus::FAILED;
18061788
*out = ByteSource::FromBIO(bio);
@@ -1810,12 +1792,23 @@ WebCryptoKeyExportStatus PKEY_SPKI_Export(const KeyObjectData& key_data,
18101792
WebCryptoKeyExportStatus PKEY_PKCS8_Export(const KeyObjectData& key_data,
18111793
ByteSource* out) {
18121794
CHECK_EQ(key_data.GetKeyType(), kKeyTypePrivate);
1813-
Mutex::ScopedLock lock(key_data.mutex());
18141795
const auto& m_pkey = key_data.GetAsymmetricKey();
1796+
EVP_PKEY* export_key = m_pkey.get();
1797+
#if NCRYPTO_USE_OPENSSL3_PROVIDER
1798+
// OpenSSL's provider EC PKCS8 encoders temporarily change encoding flags.
1799+
// Use an independent key so concurrent exports do not change the source.
1800+
EVPKeyPointer key_copy;
1801+
if ((m_pkey.isA(KeyAlgorithm::EC) || m_pkey.isA(KeyAlgorithm::SM2)) &&
1802+
EVP_PKEY_get0_provider(m_pkey.get()) != nullptr) {
1803+
key_copy.reset(EVP_PKEY_dup(m_pkey.get()));
1804+
if (!key_copy) return WebCryptoKeyExportStatus::FAILED;
1805+
export_key = key_copy.get();
1806+
}
1807+
#endif
18151808

18161809
auto bio = BIOPointer::NewMem();
18171810
CHECK(bio);
1818-
PKCS8Pointer p8inf(EVP_PKEY2PKCS8(m_pkey.get()));
1811+
PKCS8Pointer p8inf(EVP_PKEY2PKCS8(export_key));
18191812
if (!i2d_PKCS8_PRIV_KEY_INFO_bio(bio.get(), p8inf.get()))
18201813
return WebCryptoKeyExportStatus::FAILED;
18211814

‎src/crypto/crypto_keys.h‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,8 @@ class KeyObjectData final : public MemoryRetainer {
5454

5555
KeyType GetKeyType() const;
5656

57-
// These functions allow unprotected access to the raw key material and should
58-
// only be used to implement cryptographic operations requiring the key.
57+
// The key material is immutable and can be used concurrently by operations
58+
// with separate contexts.
5959
const ncrypto::EVPKeyPointer& GetAsymmetricKey() const;
6060
const char* GetSymmetricKey() const;
6161
size_t GetSymmetricKeySize() const;
@@ -64,8 +64,6 @@ class KeyObjectData final : public MemoryRetainer {
6464
SET_MEMORY_INFO_NAME(KeyObjectData)
6565
SET_SELF_SIZE(KeyObjectData)
6666

67-
Mutex& mutex() const;
68-
6967
static v8::Maybe<ncrypto::EVPKeyPointer::PublicKeyEncodingConfig>
7068
GetPublicKeyEncodingFromJs(const v8::FunctionCallbackInfo<v8::Value>& args,
7169
unsigned int* offset,
@@ -97,11 +95,11 @@ class KeyObjectData final : public MemoryRetainer {
9795
v8::Local<v8::Value>* out);
9896

9997
inline KeyObjectData addRef() const {
100-
return KeyObjectData(key_type_, mutex_, data_);
98+
return KeyObjectData(key_type_, data_);
10199
}
102100

103101
inline KeyObjectData addRefWithType(KeyType type) const {
104-
return KeyObjectData(type, mutex_, data_);
102+
return KeyObjectData(type, data_);
105103
}
106104

107105
private:
@@ -115,7 +113,6 @@ class KeyObjectData final : public MemoryRetainer {
115113
const char* default_msg);
116114

117115
KeyType key_type_;
118-
mutable std::shared_ptr<Mutex> mutex_;
119116

120117
struct Data final : public MemoryRetainer {
121118
const ByteSource symmetric_key;
@@ -131,10 +128,8 @@ class KeyObjectData final : public MemoryRetainer {
131128
};
132129
std::shared_ptr<Data> data_;
133130

134-
KeyObjectData(KeyType type,
135-
std::shared_ptr<Mutex> mutex,
136-
std::shared_ptr<Data> data)
137-
: key_type_(type), mutex_(std::move(mutex)), data_(std::move(data)) {}
131+
KeyObjectData(KeyType type, std::shared_ptr<Data> data)
132+
: key_type_(type), data_(std::move(data)) {}
138133
};
139134

140135
class KeyObjectHandle : public BaseObject {

‎src/crypto/crypto_rsa.cc‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,6 @@ WebCryptoCipherStatus RSACipherTraits::DoCipher(Environment* env,
340340
bool ExportJWKRsaKey(Environment* env,
341341
const KeyObjectData& key,
342342
Local<Object> target) {
343-
Mutex::ScopedLock lock(key.mutex());
344343
const auto& m_pkey = key.GetAsymmetricKey();
345344

346345
const ncrypto::Rsa rsa = m_pkey;
@@ -568,7 +567,6 @@ KeyObjectData ImportJWKRsaKey(Environment* env, Local<Object> jwk) {
568567
bool GetRsaKeyDetail(Environment* env,
569568
const KeyObjectData& key,
570569
Local<Object> target) {
571-
Mutex::ScopedLock lock(key.mutex());
572570
const auto& m_pkey = key.GetAsymmetricKey();
573571

574572
const auto rsa = ncrypto::Rsa::PublicOnly(m_pkey);

‎test/parallel/test-crypto-key-reuse-concurrent.js‎

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// Flags: --expose-internals
12
'use strict';
23

34
const common = require('../common');
@@ -18,6 +19,17 @@ const { once } = require('events');
1819
const { promisify } = require('util');
1920
const { Worker, parentPort, workerData } = require('worker_threads');
2021
const fixtures = require('../common/fixtures');
22+
const { internalBinding } = require('internal/test/binding');
23+
const { getKeyObjectHandle } = require('internal/crypto/keys');
24+
const {
25+
ECKeyExportJob,
26+
RSAKeyExportJob,
27+
kCryptoJobWebCrypto,
28+
kKeyVariantRSA_SSA_PKCS1_v1_5,
29+
kWebCryptoKeyFormatRaw,
30+
kWebCryptoKeyFormatPKCS8,
31+
kWebCryptoKeyFormatSPKI,
32+
} = internalBinding('crypto');
2133
const { subtle } = globalThis.crypto;
2234
const signAsync = promisify(sign);
2335
const verifyAsync = promisify(verify);
@@ -28,6 +40,14 @@ const sec1 = { type: 'sec1', format: 'der' };
2840
const spki = { type: 'spki', format: 'der' };
2941
const iterations = 16;
3042

43+
function exportLegacyKey(Job, format, key, expected, ...params) {
44+
return new Job(
45+
kCryptoJobWebCrypto, format, getKeyObjectHandle(key), ...params)
46+
.run().then((encoded) => {
47+
assert.deepStrictEqual(Buffer.from(encoded), Buffer.from(expected));
48+
});
49+
}
50+
3151
async function exercise({ keys, peer, expected }) {
3252
// Check generated signatures and exports with independent imports that do
3353
// not warm the shared key's provider caches before its first operation.
@@ -37,6 +57,9 @@ async function exercise({ keys, peer, expected }) {
3757
const referencePublic = createPublicKey({ key: expected.public, ...spki });
3858
const referencePem = referencePrivate.export(pkcs8Pem);
3959
const referenceSec1 = referencePrivate.export(sec1);
60+
const referenceRsaPublic = createPublicKey({
61+
key: expected.rsa.public, ...spki,
62+
});
4063

4164
for (let i = 0; i < iterations; i++) {
4265
const data = Buffer.from(`shared key operation ${i}`);
@@ -76,6 +99,40 @@ async function exercise({ keys, peer, expected }) {
7699
);
77100
}
78101

102+
// Public Web Crypto exports use direct bindings. Exercise the older
103+
// export jobs explicitly while the same keys are used by other jobs.
104+
pending.push(
105+
exportLegacyKey(ECKeyExportJob, kWebCryptoKeyFormatRaw,
106+
keys.publicKey, expected.rawPublic),
107+
exportLegacyKey(ECKeyExportJob, kWebCryptoKeyFormatSPKI,
108+
keys.publicKey, expected.public),
109+
exportLegacyKey(ECKeyExportJob, kWebCryptoKeyFormatPKCS8,
110+
keys.privateKey, expected.originalPrivate),
111+
exportLegacyKey(RSAKeyExportJob, kWebCryptoKeyFormatSPKI,
112+
keys.rsa.publicKey, expected.rsa.public,
113+
kKeyVariantRSA_SSA_PKCS1_v1_5),
114+
exportLegacyKey(RSAKeyExportJob, kWebCryptoKeyFormatPKCS8,
115+
keys.rsa.privateKey, expected.rsa.private,
116+
kKeyVariantRSA_SSA_PKCS1_v1_5),
117+
assert.rejects(new ECKeyExportJob(
118+
kCryptoJobWebCrypto, kWebCryptoKeyFormatPKCS8,
119+
getKeyObjectHandle(keys.publicKey)).run(), (err) => {
120+
assert.strictEqual(err.name, 'OperationError');
121+
assert.strictEqual(
122+
err.message, 'The operation failed for an operation-specific reason');
123+
assert.strictEqual(err.cause?.message, 'Invalid key type');
124+
return true;
125+
}),
126+
signAsync('sha256', data, keys.rsa.privateKey).then((signature) => {
127+
assert.strictEqual(
128+
verify('sha256', data, referenceRsaPublic, signature), true);
129+
return verifyAsync('sha256', data, keys.rsa.publicKey, signature)
130+
.then((valid) => {
131+
assert.strictEqual(valid, true);
132+
});
133+
}),
134+
);
135+
79136
// Export, compare, and query metadata while jobs use this same native key
80137
// in the threadpool and other Workers. Fresh wrappers also exercise the
81138
// native metadata getters on every iteration rather than their JS caches.
@@ -106,7 +163,8 @@ async function exercise({ keys, peer, expected }) {
106163
format: 'raw-public', type: 'compressed',
107164
}), Buffer.from(expected.compressedPublic));
108165
assert.deepStrictEqual(
109-
publicKey.export(spki), Buffer.from(expected.public));
166+
publicKey.export(spki),
167+
Buffer.from(expected.originalPublic ?? expected.public));
110168
assert.deepStrictEqual(
111169
privateKey.export(pkcs8), Buffer.from(expected.originalPrivate));
112170
assert.strictEqual(privateKey.export(pkcs8Pem), referencePem);
@@ -161,8 +219,15 @@ if (workerData?.sharedKeyTest) {
161219
// exercising concurrent first use of the shared key. CryptoKey conversion
162220
// validates the key before its first concurrent sign/derive/export calls.
163221
const privateKey = createPrivateKey({ key: input, ...pkcs8 });
164-
const publicKey = createPublicKey(privateKey);
165-
const keys = { privateKey, publicKey };
222+
const publicKey = expected.originalPublic === undefined ?
223+
createPublicKey(privateKey) :
224+
createPublicKey({ key: expected.originalPublic, ...spki });
225+
const rsaPrivate = createPrivateKey({ key: expected.rsa.private, ...pkcs8 });
226+
const keys = {
227+
privateKey,
228+
publicKey,
229+
rsa: { privateKey: rsaPrivate, publicKey: createPublicKey(rsaPrivate) },
230+
};
166231
if (webcrypto) {
167232
keys.ecdsaPrivate = privateKey.toCryptoKey({
168233
name: 'ECDSA', namedCurve: 'P-256',
@@ -198,6 +263,8 @@ if (workerData?.sharedKeyTest) {
198263
privateKey.export(pkcs8), Buffer.from(expected.originalPrivate));
199264
assert.deepStrictEqual(privateKey.export(sec1),
200265
createPrivateKey({ key: input, ...pkcs8 }).export(sec1));
266+
assert.deepStrictEqual(publicKey.export(spki),
267+
Buffer.from(expected.originalPublic ?? expected.public));
201268
}
202269

203270
(async () => {
@@ -207,6 +274,7 @@ if (workerData?.sharedKeyTest) {
207274
namedCurve: 'prime256v1',
208275
});
209276
const privateJwk = reference.export({ format: 'jwk' });
277+
const rsaPrivate = createPrivateKey(fixtures.readKey('rsa_private_2048.pem'));
210278
const expected = {
211279
private: reference.export(pkcs8),
212280
public: publicKey.export(spki),
@@ -224,6 +292,10 @@ if (workerData?.sharedKeyTest) {
224292
dsaEncoding: 'ieee-p1363',
225293
});
226294
}),
295+
rsa: {
296+
private: rsaPrivate.export(pkcs8),
297+
public: createPublicKey(rsaPrivate).export(spki),
298+
},
227299
};
228300
const cryptoPeer = peer.toCryptoKey({
229301
name: 'ECDH', namedCurve: 'P-256',
@@ -237,6 +309,14 @@ if (workerData?.sharedKeyTest) {
237309

238310
const algorithmIdentifier = der(0x30, Buffer.from(
239311
'06072a8648ce3d020106082a8648ce3d030107', 'hex'));
312+
const compressedPublic = der(
313+
0x30, algorithmIdentifier,
314+
der(0x03, Buffer.from([0]), expected.compressedPublic));
315+
await run(expected.private, cryptoPeer, {
316+
...expected,
317+
originalPrivate: expected.private,
318+
originalPublic: compressedPublic,
319+
});
240320
const ecPrivateKey = der(
241321
0x30, Buffer.from('020101', 'hex'),
242322
der(0x04, Buffer.from(privateJwk.d, 'base64url')));

0 commit comments

Comments
 (0)