Skip to content

Commit 15b1e98

Browse files
panvapanva
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. 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: #36825 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
1 parent 8dbc046 commit 15b1e98

5 files changed

Lines changed: 14 additions & 40 deletions

File tree

‎src/crypto/crypto_ec.cc‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,6 @@ Maybe<void> EcKeyGenTraits::AdditionalConfig(
479479
bool ExportJWKEcKey(Environment* env,
480480
const KeyObjectData& key,
481481
Local<Object> target) {
482-
Mutex::ScopedLock lock(key.mutex());
483482
const auto& m_pkey = key.GetAsymmetricKey();
484483
DCHECK(m_pkey.isA(KeyAlgorithm::EC));
485484

@@ -624,7 +623,6 @@ KeyObjectData ImportJWKEcKey(Environment* env, Local<Object> jwk) {
624623
bool GetEcKeyDetail(Environment* env,
625624
const KeyObjectData& key,
626625
Local<Object> target) {
627-
Mutex::ScopedLock lock(key.mutex());
628626
const auto& m_pkey = key.GetAsymmetricKey();
629627
DCHECK(m_pkey.isA(KeyAlgorithm::EC));
630628

‎src/crypto/crypto_keys.cc‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,6 @@ KeyObjectData ImportJWKSecretKey(Environment* env, Local<Object> jwk) {
187187
static bool ExportJWKRawKey(Environment* env,
188188
const KeyObjectData& key,
189189
Local<Object> target) {
190-
Mutex::ScopedLock lock(key.mutex());
191190
auto result =
192191
key.GetAsymmetricKey().exportRawJwk(key.GetKeyType() == kKeyTypePrivate);
193192
if (!result) {
@@ -416,7 +415,6 @@ bool KeyObjectData::ToEncodedPublicKey(
416415
return ExportJWKInner(
417416
env, addRefWithType(KeyType::kKeyTypePublic), *out, false);
418417
} else if (config.format == EVPKeyPointer::PKFormatType::RAW_PUBLIC) {
419-
Mutex::ScopedLock lock(mutex());
420418
const auto& pkey = GetAsymmetricKey();
421419
const auto* algorithm = pkey.getAlgorithm();
422420
if (algorithm == &KeyAlgorithm::EC) {
@@ -471,7 +469,6 @@ bool KeyObjectData::ToEncodedPrivateKey(
471469
return ExportJWKInner(
472470
env, addRefWithType(KeyType::kKeyTypePrivate), *out, false);
473471
} else if (config.format == EVPKeyPointer::PKFormatType::RAW_PRIVATE) {
474-
Mutex::ScopedLock lock(mutex());
475472
const auto& pkey = GetAsymmetricKey();
476473
const auto* algorithm = pkey.getAlgorithm();
477474
if (algorithm == &KeyAlgorithm::EC) {
@@ -497,7 +494,6 @@ bool KeyObjectData::ToEncodedPrivateKey(
497494
return Buffer::Copy(env, raw_data.get<const char>(), raw_data.size())
498495
.ToLocal(out);
499496
} else if (config.format == EVPKeyPointer::PKFormatType::RAW_SEED) {
500-
Mutex::ScopedLock lock(mutex());
501497
const auto& pkey = GetAsymmetricKey();
502498
auto raw_data = pkey.rawSeed();
503499
if (!raw_data) {
@@ -1056,9 +1052,7 @@ KeyObjectData::KeyObjectData(ByteSource symmetric_key)
10561052
data_(std::make_shared<Data>(std::move(symmetric_key))) {}
10571053

10581054
KeyObjectData::KeyObjectData(KeyType type, EVPKeyPointer&& pkey)
1059-
: key_type_(type),
1060-
mutex_(std::make_shared<Mutex>()),
1061-
data_(std::make_shared<Data>(std::move(pkey))) {}
1055+
: key_type_(type), data_(std::make_shared<Data>(std::move(pkey))) {}
10621056

10631057
void KeyObjectData::Data::MemoryInfo(MemoryTracker* tracker) const {
10641058
if (asymmetric_key) {
@@ -1075,11 +1069,6 @@ void KeyObjectData::MemoryInfo(MemoryTracker* tracker) const {
10751069
tracker->TrackField("data", data_);
10761070
}
10771071

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

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

14801468
const bool is_raw_supported = pkey.supportsRawPublic();
@@ -1502,7 +1490,6 @@ void KeyObjectHandle::RawPrivateKey(
15021490
const KeyObjectData& data = key->Data();
15031491
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
15041492

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

15081495
const bool is_raw_supported = pkey.supportsRawPrivate();
@@ -1530,7 +1517,6 @@ void KeyObjectHandle::ExportECPublicRaw(
15301517
const KeyObjectData& data = key->Data();
15311518
CHECK_NE(data.GetKeyType(), kKeyTypeSecret);
15321519

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

1572-
Mutex::ScopedLock lock(data.mutex());
15731558
const auto& m_pkey = data.GetAsymmetricKey();
15741559
if (!m_pkey.isA(KeyAlgorithm::EC)) {
15751560
return THROW_ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS(env);
@@ -1592,7 +1577,6 @@ void KeyObjectHandle::ExportECPrivatePkcs8(
15921577
ASSIGN_OR_RETURN_UNWRAP(&key, args.This());
15931578
const KeyObjectData& data = key->Data();
15941579
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
1595-
Mutex::ScopedLock lock(data.mutex());
15961580
auto encoded = ncrypto::Ec::ExportPrivatePkcs8(data.GetAsymmetricKey());
15971581
if (!encoded) {
15981582
return THROW_ERR_CRYPTO_OPERATION_FAILED(env,
@@ -1611,7 +1595,6 @@ void KeyObjectHandle::RawSeed(const v8::FunctionCallbackInfo<v8::Value>& args) {
16111595
const KeyObjectData& data = key->Data();
16121596
CHECK_EQ(data.GetKeyType(), kKeyTypePrivate);
16131597

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

16171600
auto raw_data = pkey.rawSeed();

‎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
@@ -298,7 +298,6 @@ WebCryptoCipherStatus RSACipherTraits::DoCipher(Environment* env,
298298
bool ExportJWKRsaKey(Environment* env,
299299
const KeyObjectData& key,
300300
Local<Object> target) {
301-
Mutex::ScopedLock lock(key.mutex());
302301
const auto& m_pkey = key.GetAsymmetricKey();
303302

304303
const ncrypto::Rsa rsa = m_pkey;
@@ -526,7 +525,6 @@ KeyObjectData ImportJWKRsaKey(Environment* env, Local<Object> jwk) {
526525
bool GetRsaKeyDetail(Environment* env,
527526
const KeyObjectData& key,
528527
Local<Object> target) {
529-
Mutex::ScopedLock lock(key.mutex());
530528
const auto& m_pkey = key.GetAsymmetricKey();
531529

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

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,6 @@ async function exercise({ keys, peer, expected }) {
103103
}), Buffer.from(expected.compressedPublic));
104104
assert.deepStrictEqual(
105105
publicKey.export(spki), Buffer.from(expected.public));
106-
assert.deepStrictEqual(
107-
privateKey.export(pkcs8), Buffer.from(expected.originalPrivate));
108106

109107
if (keys.ecdsaPrivate === undefined) {
110108
assert.deepStrictEqual(diffieHellman({
@@ -136,11 +134,6 @@ async function exercise({ keys, peer, expected }) {
136134
}),
137135
);
138136
await Promise.all(pending);
139-
140-
// PKCS8 export adds the EC public point on an independent copy. It must
141-
// not change the original key's encoding flags, including during signing.
142-
assert.deepStrictEqual(
143-
privateKey.export(pkcs8), Buffer.from(expected.originalPrivate));
144137
}
145138
}
146139

@@ -190,6 +183,13 @@ if (workerData?.sharedKeyTest) {
190183
Atomics.store(barrier, 0, 1);
191184
Atomics.notify(barrier, 0);
192185
await Promise.all([exercise(data), ...exited]);
186+
187+
// Ordinary PKCS8 encoding temporarily changes EC encoding flags in some
188+
// OpenSSL versions, so compare only after all shared-key users finish.
189+
// WebCrypto export must add the public point on a copy and leave the
190+
// original key's encoding flags unchanged.
191+
assert.deepStrictEqual(
192+
privateKey.export(pkcs8), Buffer.from(expected.originalPrivate));
193193
}
194194

195195
(async () => {

0 commit comments

Comments
 (0)