Skip to content

Commit 5da4dee

Browse files
danbevdanbev
committed
src: add mutex to ManagedEVPPKey class
This commit introduces a mutex field on the ManagedEVPPKey class intended to be used when multiple threads require access to an OpenSSL EVP_PKEY object. The motivation for this came from the work being done to upgrade Node.js to OpenSSL 3.0. OpenSSL objects, like EVP_PKEY, are not thread safe (see refs for details). In versions prior to OpenSSL 3.0 this was not noticeable and did not cause any issues (like incorrect logic or crashes), but with OpenSSL 3.0 this does cause issues if access to an EVP_PKEY instance is required from multiple threads without locking. In OpenSSL 3.0 when the evp_pkey_downgrade function is called, which downgrades an EVP_PKEY instance to a legacy version, it will clear all the fields of EVP_PKEY struct except the lock (nodejs#13374). But this also means that keymgmt and keydata will also be cleared, which other parts of the code base depends on, and those calls will either fail to export the key (returning null) or crash due to a segment fault. This same code works with OpenSSL 1.1.1 without locking and I think this is because there is no downgrade being done in OpenSSL 1.1.1. But even so, as far as I can tell there are no guarantees that these object are thread safe in 1.1.1 either and should be protected with a lock. Refs: openssl/openssl#13374 openssl/openssl#2165) https://www.openssl.org/blog/blog/2017/02/21/threads
1 parent 67c8e2b commit 5da4dee

File tree

6 files changed

+70
-42
lines changed

6 files changed

+70
-42
lines changed

‎src/crypto/crypto_dsa.cc‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,11 @@ Maybe<bool> ExportJWKDsaKey(
131131
Environment* env,
132132
std::shared_ptr<KeyObjectData> key,
133133
Local<Object> target) {
134-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
135-
CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_DSA);
134+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
135+
Mutex::ScopedLock lock(*m_pkey.mutex());
136+
CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_DSA);
136137

137-
DSA* dsa = EVP_PKEY_get0_DSA(pkey.get());
138+
DSA* dsa = EVP_PKEY_get0_DSA(m_pkey.get());
138139
CHECK_NOT_NULL(dsa);
139140

140141
const BIGNUM* y;
@@ -235,11 +236,12 @@ Maybe<bool> GetDsaKeyDetail(
235236
const BIGNUM* p; // Modulus length
236237
const BIGNUM* q; // Divisor length
237238

238-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
239-
int type = EVP_PKEY_id(pkey.get());
239+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
240+
Mutex::ScopedLock lock(*m_pkey.mutex());
241+
int type = EVP_PKEY_id(m_pkey.get());
240242
CHECK(type == EVP_PKEY_DSA);
241243

242-
DSA* dsa = EVP_PKEY_get0_DSA(pkey.get());
244+
DSA* dsa = EVP_PKEY_get0_DSA(m_pkey.get());
243245
CHECK_NOT_NULL(dsa);
244246

245247
DSA_get0_pqg(dsa, &p, &q, nullptr);

‎src/crypto/crypto_ecdh.cc‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -425,17 +425,20 @@ Maybe<bool> ECDHBitsTraits::AdditionalConfig(
425425
return Nothing<bool>();
426426
}
427427

428+
ManagedEVPPKey m_pkey = private_key->Data()->GetAsymmetricKey();
429+
Mutex::ScopedLock private_key_lock(*m_pkey.mutex());
430+
428431
params->private_key = ECKeyPointer(
429-
EC_KEY_dup(
430-
EVP_PKEY_get1_EC_KEY(private_key->Data()->GetAsymmetricKey().get())));
432+
EC_KEY_dup(EVP_PKEY_get1_EC_KEY(m_pkey.get())));
431433
if (!params->private_key) {
432434
THROW_ERR_CRYPTO_INVALID_KEYTYPE(env);
433435
return Nothing<bool>();
434436
}
435437

438+
ManagedEVPPKey m_ppkey = public_key->Data()->GetAsymmetricKey();
439+
Mutex::ScopedLock public_key_lock(*m_ppkey.mutex());
436440
params->public_key = ECKeyPointer(
437-
EC_KEY_dup(
438-
EVP_PKEY_get1_EC_KEY(public_key->Data()->GetAsymmetricKey().get())));
441+
EC_KEY_dup(EVP_PKEY_get1_EC_KEY(m_ppkey.get())));
439442
if (!params->public_key) {
440443
THROW_ERR_CRYPTO_INVALID_KEYTYPE(env);
441444
return Nothing<bool>();
@@ -535,9 +538,11 @@ WebCryptoKeyExportStatus EC_Raw_Export(
535538
KeyObjectData* key_data,
536539
const ECKeyExportConfig& params,
537540
ByteSource* out) {
538-
CHECK(key_data->GetAsymmetricKey());
541+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
542+
CHECK(m_pkey);
543+
Mutex::ScopedLock lock(*m_pkey.mutex());
539544

540-
EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(key_data->GetAsymmetricKey().get());
545+
EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(m_pkey.get());
541546
CHECK_NOT_NULL(ec_key);
542547

543548
const EC_GROUP* group = EC_KEY_get0_group(ec_key);
@@ -598,10 +603,11 @@ Maybe<bool> ExportJWKEcKey(
598603
Environment* env,
599604
std::shared_ptr<KeyObjectData> key,
600605
Local<Object> target) {
601-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
602-
CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC);
606+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
607+
Mutex::ScopedLock lock(*m_pkey.mutex());
608+
CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_EC);
603609

604-
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(pkey.get());
610+
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(m_pkey.get());
605611
CHECK_NOT_NULL(ec);
606612

607613
const EC_POINT* pub = EC_KEY_get0_public_key(ec);
@@ -719,10 +725,11 @@ Maybe<bool> GetEcKeyDetail(
719725
Environment* env,
720726
std::shared_ptr<KeyObjectData> key,
721727
Local<Object> target) {
722-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
723-
CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC);
728+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
729+
Mutex::ScopedLock lock(*m_pkey.mutex());
730+
CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_EC);
724731

725-
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(pkey.get());
732+
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(m_pkey.get());
726733
CHECK_NOT_NULL(ec);
727734

728735
const EC_GROUP* group = EC_KEY_get0_group(ec);

‎src/crypto/crypto_keys.cc‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,8 @@ Maybe<bool> GetAsymmetricKeyDetail(
548548
}
549549
} // namespace
550550

551-
ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)) {}
551+
ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)),
552+
mutex_(std::make_shared<Mutex>()) {}
552553

553554
ManagedEVPPKey::ManagedEVPPKey(const ManagedEVPPKey& that) {
554555
*this = that;
@@ -560,6 +561,8 @@ ManagedEVPPKey& ManagedEVPPKey::operator=(const ManagedEVPPKey& that) {
560561
if (pkey_)
561562
EVP_PKEY_up_ref(pkey_.get());
562563

564+
mutex_ = that.mutex_;
565+
563566
return *this;
564567
}
565568

@@ -571,6 +574,10 @@ EVP_PKEY* ManagedEVPPKey::get() const {
571574
return pkey_.get();
572575
}
573576

577+
Mutex* ManagedEVPPKey::mutex() const {
578+
return mutex_.get();
579+
}
580+
574581
void ManagedEVPPKey::MemoryInfo(MemoryTracker* tracker) const {
575582
tracker->TrackFieldWithSize("pkey",
576583
!pkey_ ? 0 : kSizeOf_EVP_PKEY +
@@ -1279,8 +1286,10 @@ WebCryptoKeyExportStatus PKEY_SPKI_Export(
12791286
KeyObjectData* key_data,
12801287
ByteSource* out) {
12811288
CHECK_EQ(key_data->GetKeyType(), kKeyTypePublic);
1289+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
1290+
Mutex::ScopedLock lock(*m_pkey.mutex());
12821291
BIOPointer bio(BIO_new(BIO_s_mem()));
1283-
if (!i2d_PUBKEY_bio(bio.get(), key_data->GetAsymmetricKey().get()))
1292+
if (!i2d_PUBKEY_bio(bio.get(), m_pkey.get()))
12841293
return WebCryptoKeyExportStatus::FAILED;
12851294

12861295
*out = ByteSource::FromBIO(bio);
@@ -1291,8 +1300,11 @@ WebCryptoKeyExportStatus PKEY_PKCS8_Export(
12911300
KeyObjectData* key_data,
12921301
ByteSource* out) {
12931302
CHECK_EQ(key_data->GetKeyType(), kKeyTypePrivate);
1303+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
1304+
Mutex::ScopedLock lock(*m_pkey.mutex());
1305+
12941306
BIOPointer bio(BIO_new(BIO_s_mem()));
1295-
PKCS8Pointer p8inf(EVP_PKEY2PKCS8(key_data->GetAsymmetricKey().get()));
1307+
PKCS8Pointer p8inf(EVP_PKEY2PKCS8(m_pkey.get()));
12961308
if (!i2d_PKCS8_PRIV_KEY_INFO_bio(bio.get(), p8inf.get()))
12971309
return WebCryptoKeyExportStatus::FAILED;
12981310

‎src/crypto/crypto_keys.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ class ManagedEVPPKey : public MemoryRetainer {
8181

8282
operator bool() const;
8383
EVP_PKEY* get() const;
84+
Mutex* mutex() const;
8485

8586
void MemoryInfo(MemoryTracker* tracker) const override;
8687
SET_MEMORY_INFO_NAME(ManagedEVPPKey)
@@ -127,6 +128,7 @@ class ManagedEVPPKey : public MemoryRetainer {
127128
size_t size_of_public_key() const;
128129

129130
EVPKeyPointer pkey_;
131+
std::shared_ptr<Mutex> mutex_;
130132
};
131133

132134
// Objects of this class can safely be shared among threads.

‎src/crypto/crypto_rsa.cc‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,10 @@ WebCryptoCipherStatus RSA_Cipher(
191191
const ByteSource& in,
192192
ByteSource* out) {
193193
CHECK_NE(key_data->GetKeyType(), kKeyTypeSecret);
194+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
195+
Mutex::ScopedLock lock(*m_pkey.mutex());
194196

195-
EVPKeyCtxPointer ctx(
196-
EVP_PKEY_CTX_new(key_data->GetAsymmetricKey().get(), nullptr));
197+
EVPKeyCtxPointer ctx(EVP_PKEY_CTX_new(m_pkey.get(), nullptr));
197198

198199
if (!ctx || init(ctx.get()) <= 0)
199200
return WebCryptoCipherStatus::FAILED;
@@ -363,11 +364,12 @@ Maybe<bool> ExportJWKRsaKey(
363364
Environment* env,
364365
std::shared_ptr<KeyObjectData> key,
365366
Local<Object> target) {
366-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
367-
int type = EVP_PKEY_id(pkey.get());
367+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
368+
Mutex::ScopedLock lock(*m_pkey.mutex());
369+
int type = EVP_PKEY_id(m_pkey.get());
368370
CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS);
369371

370-
RSA* rsa = EVP_PKEY_get0_RSA(pkey.get());
372+
RSA* rsa = EVP_PKEY_get0_RSA(m_pkey.get());
371373
CHECK_NOT_NULL(rsa);
372374

373375
const BIGNUM* n;
@@ -504,11 +506,12 @@ Maybe<bool> GetRsaKeyDetail(
504506
const BIGNUM* e; // Public Exponent
505507
const BIGNUM* n; // Modulus
506508

507-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
508-
int type = EVP_PKEY_id(pkey.get());
509+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
510+
Mutex::ScopedLock lock(*m_pkey.mutex());
511+
int type = EVP_PKEY_id(m_pkey.get());
509512
CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS);
510513

511-
RSA* rsa = EVP_PKEY_get0_RSA(pkey.get());
514+
RSA* rsa = EVP_PKEY_get0_RSA(m_pkey.get());
512515
CHECK_NOT_NULL(rsa);
513516

514517
RSA_get0_key(rsa, &n, &e, nullptr);

‎src/crypto/crypto_sig.cc‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ AllocatedBuffer Node_SignFinal(Environment* env,
9696
return AllocatedBuffer();
9797
}
9898

99-
int GetDefaultSignPadding(const ManagedEVPPKey& key) {
100-
return EVP_PKEY_id(key.get()) == EVP_PKEY_RSA_PSS ? RSA_PKCS1_PSS_PADDING :
101-
RSA_PKCS1_PADDING;
99+
int GetDefaultSignPadding(const ManagedEVPPKey& m_pkey) {
100+
return EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_RSA_PSS ? RSA_PKCS1_PSS_PADDING :
101+
RSA_PKCS1_PADDING;
102102
}
103103

104104
unsigned int GetBytesOfRS(const ManagedEVPPKey& pkey) {
@@ -742,11 +742,11 @@ Maybe<bool> SignTraits::AdditionalConfig(
742742
}
743743
// If this is an EC key (assuming ECDSA) we need to convert the
744744
// the signature from WebCrypto format into DER format...
745-
if (EVP_PKEY_id(params->key->GetAsymmetricKey().get()) == EVP_PKEY_EC) {
745+
ManagedEVPPKey m_pkey = params->key->GetAsymmetricKey();
746+
Mutex::ScopedLock lock(*m_pkey.mutex());
747+
if (EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_EC) {
746748
params->signature =
747-
ConvertFromWebCryptoSignature(
748-
params->key->GetAsymmetricKey(),
749-
signature.ToByteSource());
749+
ConvertFromWebCryptoSignature(m_pkey, signature.ToByteSource());
750750
} else {
751751
params->signature = mode == kCryptoJobAsync
752752
? signature.ToCopy()
@@ -764,6 +764,8 @@ bool SignTraits::DeriveBits(
764764
EVPMDPointer context(EVP_MD_CTX_new());
765765
EVP_PKEY_CTX* ctx = nullptr;
766766

767+
ManagedEVPPKey m_pkey = params.key->GetAsymmetricKey();
768+
Mutex::ScopedLock lock(*m_pkey.mutex());
767769
switch (params.mode) {
768770
case SignConfiguration::kSign:
769771
CHECK_EQ(params.key->GetKeyType(), kKeyTypePrivate);
@@ -772,7 +774,7 @@ bool SignTraits::DeriveBits(
772774
&ctx,
773775
params.digest,
774776
nullptr,
775-
params.key->GetAsymmetricKey().get())) {
777+
m_pkey.get())) {
776778
return false;
777779
}
778780
break;
@@ -783,21 +785,21 @@ bool SignTraits::DeriveBits(
783785
&ctx,
784786
params.digest,
785787
nullptr,
786-
params.key->GetAsymmetricKey().get())) {
788+
m_pkey.get())) {
787789
return false;
788790
}
789791
break;
790792
}
791793

792794
int padding = params.flags & SignConfiguration::kHasPadding
793795
? params.padding
794-
: GetDefaultSignPadding(params.key->GetAsymmetricKey());
796+
: GetDefaultSignPadding(m_pkey);
795797

796798
Maybe<int> salt_length = params.flags & SignConfiguration::kHasSaltLength
797799
? Just<int>(params.salt_length) : Nothing<int>();
798800

799801
if (!ApplyRSAOptions(
800-
params.key->GetAsymmetricKey(),
802+
m_pkey,
801803
ctx,
802804
padding,
803805
salt_length)) {
@@ -822,8 +824,8 @@ bool SignTraits::DeriveBits(
822824

823825
// If this is an EC key (assuming ECDSA) we have to
824826
// convert the signature in to the proper format.
825-
if (EVP_PKEY_id(params.key->GetAsymmetricKey().get()) == EVP_PKEY_EC) {
826-
*out = ConvertToWebCryptoSignature(params.key->GetAsymmetricKey(), buf);
827+
if (EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_EC) {
828+
*out = ConvertToWebCryptoSignature(m_pkey, buf);
827829
} else {
828830
buf.Resize(len);
829831
*out = std::move(buf);

0 commit comments

Comments
 (0)