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: https://github.com/nodejs/node/pull/36825
Refs: https://github.com/openssl/openssl/pull/13374
Refs: https://github.com/openssl/openssl/pull/13374
Refs: https://github.com/openssl/openssl/issues/2165)
Refs: https://www.openssl.org/blog/blog/2017/02/21/threads
Reviewed-By: James M Snell <jasnell@gmail.com>

src/crypto/crypto_dsa.cc

@@ -133,11 +133,12 @@ Maybe<bool> GetDsaKeyDetail(
   const BIGNUM* p;  // Modulus length
   const BIGNUM* q;  // Divisor length
 
-  ManagedEVPPKey pkey = key->GetAsymmetricKey();
+  ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
-  int type = EVP_PKEY_id(pkey.get());
+  Mutex::ScopedLock lock(*m_pkey.mutex());
+  int type = EVP_PKEY_id(m_pkey.get());
   CHECK(type == EVP_PKEY_DSA);
 
-  DSA* dsa = EVP_PKEY_get0_DSA(pkey.get());
+  DSA* dsa = EVP_PKEY_get0_DSA(m_pkey.get());
   CHECK_NOT_NULL(dsa);
 
   DSA_get0_pqg(dsa, &p, &q, nullptr);

src/crypto/crypto_ec.cc

@@ -601,9 +601,11 @@ WebCryptoKeyExportStatus EC_Raw_Export(
     KeyObjectData* key_data,
     const ECKeyExportConfig& params,
     ByteSource* out) {
-  CHECK(key_data->GetAsymmetricKey());
+  ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
+  CHECK(m_pkey);
+  Mutex::ScopedLock lock(*m_pkey.mutex());
 
-  EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(key_data->GetAsymmetricKey().get());
+  EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(m_pkey.get());
 
   unsigned char* data;
   size_t len = 0;
@@ -688,10 +690,11 @@ Maybe<bool> ExportJWKEcKey(
     Environment* env,
     std::shared_ptr<KeyObjectData> key,
     Local<Object> target) {
-  ManagedEVPPKey pkey = key->GetAsymmetricKey();
+  ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
-  CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC);
+  Mutex::ScopedLock lock(*m_pkey.mutex());
+  CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_EC);
 
-  EC_KEY* ec = EVP_PKEY_get0_EC_KEY(pkey.get());
+  EC_KEY* ec = EVP_PKEY_get0_EC_KEY(m_pkey.get());
   CHECK_NOT_NULL(ec);
 
   const EC_POINT* pub = EC_KEY_get0_public_key(ec);
@@ -893,10 +896,11 @@ Maybe<bool> GetEcKeyDetail(
     Environment* env,
     std::shared_ptr<KeyObjectData> key,
     Local<Object> target) {
-  ManagedEVPPKey pkey = key->GetAsymmetricKey();
+  ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
-  CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC);
+  Mutex::ScopedLock lock(*m_pkey.mutex());
+  CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_EC);
 
-  EC_KEY* ec = EVP_PKEY_get0_EC_KEY(pkey.get());
+  EC_KEY* ec = EVP_PKEY_get0_EC_KEY(m_pkey.get());
   CHECK_NOT_NULL(ec);
 
   const EC_GROUP* group = EC_KEY_get0_group(ec);

src/crypto/crypto_keys.cc

@@ -552,7 +552,8 @@ Maybe<bool> GetAsymmetricKeyDetail(
 }
 }  // namespace
 
-ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)) {}
+ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)),
+    mutex_(std::make_shared<Mutex>()) {}
 
 ManagedEVPPKey::ManagedEVPPKey(const ManagedEVPPKey& that) {
   *this = that;
@@ -564,6 +565,8 @@ ManagedEVPPKey& ManagedEVPPKey::operator=(const ManagedEVPPKey& that) {
   if (pkey_)
     EVP_PKEY_up_ref(pkey_.get());
 
+  mutex_ = that.mutex_;
+
   return *this;
 }
 
@@ -575,6 +578,10 @@ EVP_PKEY* ManagedEVPPKey::get() const {
   return pkey_.get();
 }
 
+Mutex* ManagedEVPPKey::mutex() const {
+  return mutex_.get();
+}
+
 void ManagedEVPPKey::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackFieldWithSize("pkey",
                               !pkey_ ? 0 : kSizeOf_EVP_PKEY +
@@ -1326,8 +1333,10 @@ WebCryptoKeyExportStatus PKEY_SPKI_Export(
     KeyObjectData* key_data,
     ByteSource* out) {
   CHECK_EQ(key_data->GetKeyType(), kKeyTypePublic);
+  ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
+  Mutex::ScopedLock lock(*m_pkey.mutex());
   BIOPointer bio(BIO_new(BIO_s_mem()));
-  if (!i2d_PUBKEY_bio(bio.get(), key_data->GetAsymmetricKey().get()))
+  if (!i2d_PUBKEY_bio(bio.get(), m_pkey.get()))
     return WebCryptoKeyExportStatus::FAILED;
 
   *out = ByteSource::FromBIO(bio);
@@ -1338,8 +1347,11 @@ WebCryptoKeyExportStatus PKEY_PKCS8_Export(
     KeyObjectData* key_data,
     ByteSource* out) {
   CHECK_EQ(key_data->GetKeyType(), kKeyTypePrivate);
+  ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
+  Mutex::ScopedLock lock(*m_pkey.mutex());
+
   BIOPointer bio(BIO_new(BIO_s_mem()));
-  PKCS8Pointer p8inf(EVP_PKEY2PKCS8(key_data->GetAsymmetricKey().get()));
+  PKCS8Pointer p8inf(EVP_PKEY2PKCS8(m_pkey.get()));
   if (!i2d_PKCS8_PRIV_KEY_INFO_bio(bio.get(), p8inf.get()))
     return WebCryptoKeyExportStatus::FAILED;
 

src/crypto/crypto_keys.h

@@ -81,6 +81,7 @@ class ManagedEVPPKey : public MemoryRetainer {
 
   operator bool() const;
   EVP_PKEY* get() const;
+  Mutex* mutex() const;
 
   void MemoryInfo(MemoryTracker* tracker) const override;
   SET_MEMORY_INFO_NAME(ManagedEVPPKey)
@@ -127,6 +128,7 @@ class ManagedEVPPKey : public MemoryRetainer {
   size_t size_of_public_key() const;
 
   EVPKeyPointer pkey_;
+  std::shared_ptr<Mutex> mutex_;
 };
 
 // Objects of this class can safely be shared among threads.

src/crypto/crypto_rsa.cc

@@ -191,9 +191,10 @@ WebCryptoCipherStatus RSA_Cipher(
     const ByteSource& in,
     ByteSource* out) {
   CHECK_NE(key_data->GetKeyType(), kKeyTypeSecret);
+  ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey();
+  Mutex::ScopedLock lock(*m_pkey.mutex());
 
-  EVPKeyCtxPointer ctx(
+  EVPKeyCtxPointer ctx(EVP_PKEY_CTX_new(m_pkey.get(), nullptr));
-      EVP_PKEY_CTX_new(key_data->GetAsymmetricKey().get(), nullptr));
 
   if (!ctx || init(ctx.get()) <= 0)
     return WebCryptoCipherStatus::FAILED;
@@ -363,17 +364,18 @@ Maybe<bool> ExportJWKRsaKey(
     Environment* env,
     std::shared_ptr<KeyObjectData> key,
     Local<Object> target) {
-  ManagedEVPPKey pkey = key->GetAsymmetricKey();
+  ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
-  int type = EVP_PKEY_id(pkey.get());
+  Mutex::ScopedLock lock(*m_pkey.mutex());
+  int type = EVP_PKEY_id(m_pkey.get());
   CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS);
 
   // TODO(tniessen): Remove the "else" branch once we drop support for OpenSSL
   // versions older than 1.1.1e via FIPS / dynamic linking.
   RSA* rsa;
   if (OpenSSL_version_num() >= 0x1010105fL) {
-    rsa = EVP_PKEY_get0_RSA(pkey.get());
+    rsa = EVP_PKEY_get0_RSA(m_pkey.get());
   } else {
-    rsa = static_cast<RSA*>(EVP_PKEY_get0(pkey.get()));
+    rsa = static_cast<RSA*>(EVP_PKEY_get0(m_pkey.get()));
   }
   CHECK_NOT_NULL(rsa);
 
@@ -511,17 +513,18 @@ Maybe<bool> GetRsaKeyDetail(
   const BIGNUM* e;  // Public Exponent
   const BIGNUM* n;  // Modulus
 
-  ManagedEVPPKey pkey = key->GetAsymmetricKey();
+  ManagedEVPPKey m_pkey = key->GetAsymmetricKey();
-  int type = EVP_PKEY_id(pkey.get());
+  Mutex::ScopedLock lock(*m_pkey.mutex());
+  int type = EVP_PKEY_id(m_pkey.get());
   CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS);
 
   // TODO(tniessen): Remove the "else" branch once we drop support for OpenSSL
   // versions older than 1.1.1e via FIPS / dynamic linking.
   RSA* rsa;
   if (OpenSSL_version_num() >= 0x1010105fL) {
-    rsa = EVP_PKEY_get0_RSA(pkey.get());
+    rsa = EVP_PKEY_get0_RSA(m_pkey.get());
   } else {
-    rsa = static_cast<RSA*>(EVP_PKEY_get0(pkey.get()));
+    rsa = static_cast<RSA*>(EVP_PKEY_get0(m_pkey.get()));
   }
   CHECK_NOT_NULL(rsa);
 

src/crypto/crypto_sig.cc

@@ -96,8 +96,8 @@ AllocatedBuffer Node_SignFinal(Environment* env,
   return AllocatedBuffer();
 }
 
-int GetDefaultSignPadding(const ManagedEVPPKey& key) {
+int GetDefaultSignPadding(const ManagedEVPPKey& m_pkey) {
-  return EVP_PKEY_id(key.get()) == EVP_PKEY_RSA_PSS ? RSA_PKCS1_PSS_PADDING :
+  return EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_RSA_PSS ? RSA_PKCS1_PSS_PADDING :
                                                          RSA_PKCS1_PADDING;
 }
 
@@ -752,11 +752,11 @@ Maybe<bool> SignTraits::AdditionalConfig(
     }
     // If this is an EC key (assuming ECDSA) we need to convert the
     // the signature from WebCrypto format into DER format...
-    if (EVP_PKEY_id(params->key->GetAsymmetricKey().get()) == EVP_PKEY_EC) {
+    ManagedEVPPKey m_pkey = params->key->GetAsymmetricKey();
+    Mutex::ScopedLock lock(*m_pkey.mutex());
+    if (EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_EC) {
       params->signature =
-          ConvertFromWebCryptoSignature(
+          ConvertFromWebCryptoSignature(m_pkey, signature.ToByteSource());
-              params->key->GetAsymmetricKey(),
-              signature.ToByteSource());
     } else {
       params->signature = mode == kCryptoJobAsync
           ? signature.ToCopy()
@@ -774,6 +774,8 @@ bool SignTraits::DeriveBits(
   EVPMDPointer context(EVP_MD_CTX_new());
   EVP_PKEY_CTX* ctx = nullptr;
 
+  ManagedEVPPKey m_pkey = params.key->GetAsymmetricKey();
+  Mutex::ScopedLock lock(*m_pkey.mutex());
   switch (params.mode) {
     case SignConfiguration::kSign:
       CHECK_EQ(params.key->GetKeyType(), kKeyTypePrivate);
@@ -782,7 +784,7 @@ bool SignTraits::DeriveBits(
               &ctx,
               params.digest,
               nullptr,
-              params.key->GetAsymmetricKey().get())) {
+              m_pkey.get())) {
         return false;
       }
       break;
@@ -793,7 +795,7 @@ bool SignTraits::DeriveBits(
               &ctx,
               params.digest,
               nullptr,
-              params.key->GetAsymmetricKey().get())) {
+              m_pkey.get())) {
         return false;
       }
       break;
@@ -801,13 +803,13 @@ bool SignTraits::DeriveBits(
 
   int padding = params.flags & SignConfiguration::kHasPadding
       ? params.padding
-      : GetDefaultSignPadding(params.key->GetAsymmetricKey());
+      : GetDefaultSignPadding(m_pkey);
 
   Maybe<int> salt_length = params.flags & SignConfiguration::kHasSaltLength
       ? Just<int>(params.salt_length) : Nothing<int>();
 
   if (!ApplyRSAOptions(
-          params.key->GetAsymmetricKey(),
+          m_pkey,
           ctx,
           padding,
           salt_length)) {