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 42cc33c

Browse filesBrowse files
danbevdanielleadams
danbev
authored and
danielleadams
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 (#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. PR-URL: #36825 Refs: openssl/openssl#13374 Refs: openssl/openssl#13374 Refs: openssl/openssl#2165) Refs: https://www.openssl.org/blog/blog/2017/02/21/threads Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent e28ea89 commit 42cc33c
Copy full SHA for 42cc33c

File tree

Expand file treeCollapse file tree

6 files changed

+59
-35
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+59
-35
lines changed
Open diff view settings
Collapse file

‎src/crypto/crypto_dsa.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_dsa.cc
+4-3Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,11 +133,12 @@ Maybe<bool> GetDsaKeyDetail(
133133
const BIGNUM* p; // Modulus length
134134
const BIGNUM* q; // Divisor length
135135

136-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
137-
int type = EVP_PKEY_id(pkey.get());
136+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
137+
Mutex::ScopedLock lock(*m_pkey.mutex());
138+
int type = EVP_PKEY_id(m_pkey.get());
138139
CHECK(type == EVP_PKEY_DSA);
139140

140-
DSA* dsa = EVP_PKEY_get0_DSA(pkey.get());
141+
DSA* dsa = EVP_PKEY_get0_DSA(m_pkey.get());
141142
CHECK_NOT_NULL(dsa);
142143

143144
DSA_get0_pqg(dsa, &p, &q, nullptr);
Collapse file

‎src/crypto/crypto_ec.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_ec.cc
+12-8Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -601,9 +601,11 @@ WebCryptoKeyExportStatus EC_Raw_Export(
601601
KeyObjectData* key_data,
602602
const ECKeyExportConfig& params,
603603
ByteSource* out) {
604-
CHECK(key_data->GetAsymmetricKey());
604+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
605+
CHECK(m_pkey);
606+
Mutex::ScopedLock lock(*m_pkey.mutex());
605607

606-
EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(key_data->GetAsymmetricKey().get());
608+
EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(m_pkey.get());
607609

608610
unsigned char* data;
609611
size_t len = 0;
@@ -688,10 +690,11 @@ Maybe<bool> ExportJWKEcKey(
688690
Environment* env,
689691
std::shared_ptr<KeyObjectData> key,
690692
Local<Object> target) {
691-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
692-
CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC);
693+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
694+
Mutex::ScopedLock lock(*m_pkey.mutex());
695+
CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_EC);
693696

694-
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(pkey.get());
697+
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(m_pkey.get());
695698
CHECK_NOT_NULL(ec);
696699

697700
const EC_POINT* pub = EC_KEY_get0_public_key(ec);
@@ -893,10 +896,11 @@ Maybe<bool> GetEcKeyDetail(
893896
Environment* env,
894897
std::shared_ptr<KeyObjectData> key,
895898
Local<Object> target) {
896-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
897-
CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC);
899+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
900+
Mutex::ScopedLock lock(*m_pkey.mutex());
901+
CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_EC);
898902

899-
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(pkey.get());
903+
EC_KEY* ec = EVP_PKEY_get0_EC_KEY(m_pkey.get());
900904
CHECK_NOT_NULL(ec);
901905

902906
const EC_GROUP* group = EC_KEY_get0_group(ec);
Collapse file

‎src/crypto/crypto_keys.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_keys.cc
+15-3Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,8 @@ Maybe<bool> GetAsymmetricKeyDetail(
552552
}
553553
} // namespace
554554

555-
ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)) {}
555+
ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)),
556+
mutex_(std::make_shared<Mutex>()) {}
556557

557558
ManagedEVPPKey::ManagedEVPPKey(const ManagedEVPPKey& that) {
558559
*this = that;
@@ -564,6 +565,8 @@ ManagedEVPPKey& ManagedEVPPKey::operator=(const ManagedEVPPKey& that) {
564565
if (pkey_)
565566
EVP_PKEY_up_ref(pkey_.get());
566567

568+
mutex_ = that.mutex_;
569+
567570
return *this;
568571
}
569572

@@ -575,6 +578,10 @@ EVP_PKEY* ManagedEVPPKey::get() const {
575578
return pkey_.get();
576579
}
577580

581+
Mutex* ManagedEVPPKey::mutex() const {
582+
return mutex_.get();
583+
}
584+
578585
void ManagedEVPPKey::MemoryInfo(MemoryTracker* tracker) const {
579586
tracker->TrackFieldWithSize("pkey",
580587
!pkey_ ? 0 : kSizeOf_EVP_PKEY +
@@ -1326,8 +1333,10 @@ WebCryptoKeyExportStatus PKEY_SPKI_Export(
13261333
KeyObjectData* key_data,
13271334
ByteSource* out) {
13281335
CHECK_EQ(key_data->GetKeyType(), kKeyTypePublic);
1336+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
1337+
Mutex::ScopedLock lock(*m_pkey.mutex());
13291338
BIOPointer bio(BIO_new(BIO_s_mem()));
1330-
if (!i2d_PUBKEY_bio(bio.get(), key_data->GetAsymmetricKey().get()))
1339+
if (!i2d_PUBKEY_bio(bio.get(), m_pkey.get()))
13311340
return WebCryptoKeyExportStatus::FAILED;
13321341

13331342
*out = ByteSource::FromBIO(bio);
@@ -1338,8 +1347,11 @@ WebCryptoKeyExportStatus PKEY_PKCS8_Export(
13381347
KeyObjectData* key_data,
13391348
ByteSource* out) {
13401349
CHECK_EQ(key_data->GetKeyType(), kKeyTypePrivate);
1350+
ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
1351+
Mutex::ScopedLock lock(*m_pkey.mutex());
1352+
13411353
BIOPointer bio(BIO_new(BIO_s_mem()));
1342-
PKCS8Pointer p8inf(EVP_PKEY2PKCS8(key_data->GetAsymmetricKey().get()));
1354+
PKCS8Pointer p8inf(EVP_PKEY2PKCS8(m_pkey.get()));
13431355
if (!i2d_PKCS8_PRIV_KEY_INFO_bio(bio.get(), p8inf.get()))
13441356
return WebCryptoKeyExportStatus::FAILED;
13451357

Collapse file

‎src/crypto/crypto_keys.h‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_keys.h
+2Lines 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.
Collapse file

‎src/crypto/crypto_rsa.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_rsa.cc
+13-10Lines changed: 13 additions & 10 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,17 +364,18 @@ 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

370372
// TODO(tniessen): Remove the "else" branch once we drop support for OpenSSL
371373
// versions older than 1.1.1e via FIPS / dynamic linking.
372374
RSA* rsa;
373375
if (OpenSSL_version_num() >= 0x1010105fL) {
374-
rsa = EVP_PKEY_get0_RSA(pkey.get());
376+
rsa = EVP_PKEY_get0_RSA(m_pkey.get());
375377
} else {
376-
rsa = static_cast<RSA*>(EVP_PKEY_get0(pkey.get()));
378+
rsa = static_cast<RSA*>(EVP_PKEY_get0(m_pkey.get()));
377379
}
378380
CHECK_NOT_NULL(rsa);
379381

@@ -511,17 +513,18 @@ Maybe<bool> GetRsaKeyDetail(
511513
const BIGNUM* e; // Public Exponent
512514
const BIGNUM* n; // Modulus
513515

514-
ManagedEVPPKey pkey = key->GetAsymmetricKey();
515-
int type = EVP_PKEY_id(pkey.get());
516+
ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
517+
Mutex::ScopedLock lock(*m_pkey.mutex());
518+
int type = EVP_PKEY_id(m_pkey.get());
516519
CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS);
517520

518521
// TODO(tniessen): Remove the "else" branch once we drop support for OpenSSL
519522
// versions older than 1.1.1e via FIPS / dynamic linking.
520523
RSA* rsa;
521524
if (OpenSSL_version_num() >= 0x1010105fL) {
522-
rsa = EVP_PKEY_get0_RSA(pkey.get());
525+
rsa = EVP_PKEY_get0_RSA(m_pkey.get());
523526
} else {
524-
rsa = static_cast<RSA*>(EVP_PKEY_get0(pkey.get()));
527+
rsa = static_cast<RSA*>(EVP_PKEY_get0(m_pkey.get()));
525528
}
526529
CHECK_NOT_NULL(rsa);
527530

Collapse file

‎src/crypto/crypto_sig.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_sig.cc
+13-11Lines changed: 13 additions & 11 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) {
@@ -752,11 +752,11 @@ Maybe<bool> SignTraits::AdditionalConfig(
752752
}
753753
// If this is an EC key (assuming ECDSA) we need to convert the
754754
// the signature from WebCrypto format into DER format...
755-
if (EVP_PKEY_id(params->key->GetAsymmetricKey().get()) == EVP_PKEY_EC) {
755+
ManagedEVPPKey m_pkey = params->key->GetAsymmetricKey();
756+
Mutex::ScopedLock lock(*m_pkey.mutex());
757+
if (EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_EC) {
756758
params->signature =
757-
ConvertFromWebCryptoSignature(
758-
params->key->GetAsymmetricKey(),
759-
signature.ToByteSource());
759+
ConvertFromWebCryptoSignature(m_pkey, signature.ToByteSource());
760760
} else {
761761
params->signature = mode == kCryptoJobAsync
762762
? signature.ToCopy()
@@ -774,6 +774,8 @@ bool SignTraits::DeriveBits(
774774
EVPMDPointer context(EVP_MD_CTX_new());
775775
EVP_PKEY_CTX* ctx = nullptr;
776776

777+
ManagedEVPPKey m_pkey = params.key->GetAsymmetricKey();
778+
Mutex::ScopedLock lock(*m_pkey.mutex());
777779
switch (params.mode) {
778780
case SignConfiguration::kSign:
779781
CHECK_EQ(params.key->GetKeyType(), kKeyTypePrivate);
@@ -782,7 +784,7 @@ bool SignTraits::DeriveBits(
782784
&ctx,
783785
params.digest,
784786
nullptr,
785-
params.key->GetAsymmetricKey().get())) {
787+
m_pkey.get())) {
786788
return false;
787789
}
788790
break;
@@ -793,21 +795,21 @@ bool SignTraits::DeriveBits(
793795
&ctx,
794796
params.digest,
795797
nullptr,
796-
params.key->GetAsymmetricKey().get())) {
798+
m_pkey.get())) {
797799
return false;
798800
}
799801
break;
800802
}
801803

802804
int padding = params.flags & SignConfiguration::kHasPadding
803805
? params.padding
804-
: GetDefaultSignPadding(params.key->GetAsymmetricKey());
806+
: GetDefaultSignPadding(m_pkey);
805807

806808
Maybe<int> salt_length = params.flags & SignConfiguration::kHasSaltLength
807809
? Just<int>(params.salt_length) : Nothing<int>();
808810

809811
if (!ApplyRSAOptions(
810-
params.key->GetAsymmetricKey(),
812+
m_pkey,
811813
ctx,
812814
padding,
813815
salt_length)) {

0 commit comments

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