src: add mutex to ManagedEVPPKey class · nodejs/node@42cc33c · GitHub
Skip to content

Commit 42cc33c

Browse 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

6 files changed

Lines changed: 59 additions & 35 deletions

File tree

‎src/crypto/crypto_dsa.cc‎

Lines changed: 4 additions & 3 deletions

‎src/crypto/crypto_ec.cc‎

Lines 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);

‎src/crypto/crypto_keys.cc‎

Lines 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

‎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: 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

‎src/crypto/crypto_sig.cc‎

Lines changed: 13 additions & 11 deletions

0 commit comments

Comments
 (0)