From 5da4dee6e6415ccbba2a43bd77bfdf23b461bd56 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 7 Jan 2021 08:21:37 +0100 Subject: [PATCH] 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. Refs: https://github.com/openssl/openssl/pull/13374 https://github.com/openssl/openssl/issues/2165) https://www.openssl.org/blog/blog/2017/02/21/threads --- src/crypto/crypto_dsa.cc | 14 ++++++++------ src/crypto/crypto_ecdh.cc | 31 +++++++++++++++++++------------ src/crypto/crypto_keys.cc | 18 +++++++++++++++--- src/crypto/crypto_keys.h | 2 ++ src/crypto/crypto_rsa.cc | 19 +++++++++++-------- src/crypto/crypto_sig.cc | 28 +++++++++++++++------------- 6 files changed, 70 insertions(+), 42 deletions(-) diff --git a/src/crypto/crypto_dsa.cc b/src/crypto/crypto_dsa.cc index 8a80904d9c1afb..54c5ec6866644e 100644 --- a/src/crypto/crypto_dsa.cc +++ b/src/crypto/crypto_dsa.cc @@ -131,10 +131,11 @@ Maybe ExportJWKDsaKey( Environment* env, std::shared_ptr key, Local target) { - ManagedEVPPKey pkey = key->GetAsymmetricKey(); - CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_DSA); + ManagedEVPPKey m_pkey = key->GetAsymmetricKey(); + Mutex::ScopedLock lock(*m_pkey.mutex()); + CHECK_EQ(EVP_PKEY_id(m_pkey.get()), EVP_PKEY_DSA); - DSA* dsa = EVP_PKEY_get0_DSA(pkey.get()); + DSA* dsa = EVP_PKEY_get0_DSA(m_pkey.get()); CHECK_NOT_NULL(dsa); const BIGNUM* y; @@ -235,11 +236,12 @@ Maybe GetDsaKeyDetail( const BIGNUM* p; // Modulus length const BIGNUM* q; // Divisor length - ManagedEVPPKey pkey = key->GetAsymmetricKey(); - int type = EVP_PKEY_id(pkey.get()); + ManagedEVPPKey m_pkey = key->GetAsymmetricKey(); + 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); diff --git a/src/crypto/crypto_ecdh.cc b/src/crypto/crypto_ecdh.cc index efeb08b908e427..09d2b53d662196 100644 --- a/src/crypto/crypto_ecdh.cc +++ b/src/crypto/crypto_ecdh.cc @@ -425,17 +425,20 @@ Maybe ECDHBitsTraits::AdditionalConfig( return Nothing(); } + ManagedEVPPKey m_pkey = private_key->Data()->GetAsymmetricKey(); + Mutex::ScopedLock private_key_lock(*m_pkey.mutex()); + params->private_key = ECKeyPointer( - EC_KEY_dup( - EVP_PKEY_get1_EC_KEY(private_key->Data()->GetAsymmetricKey().get()))); + EC_KEY_dup(EVP_PKEY_get1_EC_KEY(m_pkey.get()))); if (!params->private_key) { THROW_ERR_CRYPTO_INVALID_KEYTYPE(env); return Nothing(); } + ManagedEVPPKey m_ppkey = public_key->Data()->GetAsymmetricKey(); + Mutex::ScopedLock public_key_lock(*m_ppkey.mutex()); params->public_key = ECKeyPointer( - EC_KEY_dup( - EVP_PKEY_get1_EC_KEY(public_key->Data()->GetAsymmetricKey().get()))); + EC_KEY_dup(EVP_PKEY_get1_EC_KEY(m_ppkey.get()))); if (!params->public_key) { THROW_ERR_CRYPTO_INVALID_KEYTYPE(env); return Nothing(); @@ -535,9 +538,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()); CHECK_NOT_NULL(ec_key); const EC_GROUP* group = EC_KEY_get0_group(ec_key); @@ -598,10 +603,11 @@ Maybe ExportJWKEcKey( Environment* env, std::shared_ptr key, Local target) { - ManagedEVPPKey pkey = key->GetAsymmetricKey(); - CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC); + ManagedEVPPKey m_pkey = key->GetAsymmetricKey(); + 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); @@ -719,10 +725,11 @@ Maybe GetEcKeyDetail( Environment* env, std::shared_ptr key, Local target) { - ManagedEVPPKey pkey = key->GetAsymmetricKey(); - CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC); + ManagedEVPPKey m_pkey = key->GetAsymmetricKey(); + 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); diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index f80a39ce5de575..4281f5895b2764 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -548,7 +548,8 @@ Maybe GetAsymmetricKeyDetail( } } // namespace -ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)) {} +ManagedEVPPKey::ManagedEVPPKey(EVPKeyPointer&& pkey) : pkey_(std::move(pkey)), + mutex_(std::make_shared()) {} ManagedEVPPKey::ManagedEVPPKey(const ManagedEVPPKey& that) { *this = that; @@ -560,6 +561,8 @@ ManagedEVPPKey& ManagedEVPPKey::operator=(const ManagedEVPPKey& that) { if (pkey_) EVP_PKEY_up_ref(pkey_.get()); + mutex_ = that.mutex_; + return *this; } @@ -571,6 +574,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 + @@ -1279,8 +1286,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); @@ -1291,8 +1300,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; diff --git a/src/crypto/crypto_keys.h b/src/crypto/crypto_keys.h index 8938a203eb1d08..3fa04beb63eb12 100644 --- a/src/crypto/crypto_keys.h +++ b/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_; }; // Objects of this class can safely be shared among threads. diff --git a/src/crypto/crypto_rsa.cc b/src/crypto/crypto_rsa.cc index d859551392a382..ccf52a21af5154 100644 --- a/src/crypto/crypto_rsa.cc +++ b/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( - EVP_PKEY_CTX_new(key_data->GetAsymmetricKey().get(), nullptr)); + EVPKeyCtxPointer ctx(EVP_PKEY_CTX_new(m_pkey.get(), nullptr)); if (!ctx || init(ctx.get()) <= 0) return WebCryptoCipherStatus::FAILED; @@ -363,11 +364,12 @@ Maybe ExportJWKRsaKey( Environment* env, std::shared_ptr key, Local target) { - ManagedEVPPKey pkey = key->GetAsymmetricKey(); - int type = EVP_PKEY_id(pkey.get()); + ManagedEVPPKey m_pkey = key->GetAsymmetricKey(); + Mutex::ScopedLock lock(*m_pkey.mutex()); + int type = EVP_PKEY_id(m_pkey.get()); CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS); - RSA* rsa = EVP_PKEY_get0_RSA(pkey.get()); + RSA* rsa = EVP_PKEY_get0_RSA(m_pkey.get()); CHECK_NOT_NULL(rsa); const BIGNUM* n; @@ -504,11 +506,12 @@ Maybe GetRsaKeyDetail( const BIGNUM* e; // Public Exponent const BIGNUM* n; // Modulus - ManagedEVPPKey pkey = key->GetAsymmetricKey(); - int type = EVP_PKEY_id(pkey.get()); + ManagedEVPPKey m_pkey = key->GetAsymmetricKey(); + Mutex::ScopedLock lock(*m_pkey.mutex()); + int type = EVP_PKEY_id(m_pkey.get()); CHECK(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA_PSS); - RSA* rsa = EVP_PKEY_get0_RSA(pkey.get()); + RSA* rsa = EVP_PKEY_get0_RSA(m_pkey.get()); CHECK_NOT_NULL(rsa); RSA_get0_key(rsa, &n, &e, nullptr); diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc index a5a95878a9eeec..d4fd9521c5eb5c 100644 --- a/src/crypto/crypto_sig.cc +++ b/src/crypto/crypto_sig.cc @@ -96,9 +96,9 @@ AllocatedBuffer Node_SignFinal(Environment* env, return AllocatedBuffer(); } -int GetDefaultSignPadding(const ManagedEVPPKey& key) { - return EVP_PKEY_id(key.get()) == EVP_PKEY_RSA_PSS ? RSA_PKCS1_PSS_PADDING : - RSA_PKCS1_PADDING; +int GetDefaultSignPadding(const ManagedEVPPKey& m_pkey) { + return EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_RSA_PSS ? RSA_PKCS1_PSS_PADDING : + RSA_PKCS1_PADDING; } unsigned int GetBytesOfRS(const ManagedEVPPKey& pkey) { @@ -742,11 +742,11 @@ Maybe 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( - params->key->GetAsymmetricKey(), - signature.ToByteSource()); + ConvertFromWebCryptoSignature(m_pkey, signature.ToByteSource()); } else { params->signature = mode == kCryptoJobAsync ? signature.ToCopy() @@ -764,6 +764,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); @@ -772,7 +774,7 @@ bool SignTraits::DeriveBits( &ctx, params.digest, nullptr, - params.key->GetAsymmetricKey().get())) { + m_pkey.get())) { return false; } break; @@ -783,7 +785,7 @@ bool SignTraits::DeriveBits( &ctx, params.digest, nullptr, - params.key->GetAsymmetricKey().get())) { + m_pkey.get())) { return false; } break; @@ -791,13 +793,13 @@ bool SignTraits::DeriveBits( int padding = params.flags & SignConfiguration::kHasPadding ? params.padding - : GetDefaultSignPadding(params.key->GetAsymmetricKey()); + : GetDefaultSignPadding(m_pkey); Maybe salt_length = params.flags & SignConfiguration::kHasSaltLength ? Just(params.salt_length) : Nothing(); if (!ApplyRSAOptions( - params.key->GetAsymmetricKey(), + m_pkey, ctx, padding, salt_length)) { @@ -822,8 +824,8 @@ bool SignTraits::DeriveBits( // If this is an EC key (assuming ECDSA) we have to // convert the signature in to the proper format. - if (EVP_PKEY_id(params.key->GetAsymmetricKey().get()) == EVP_PKEY_EC) { - *out = ConvertToWebCryptoSignature(params.key->GetAsymmetricKey(), buf); + if (EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_EC) { + *out = ConvertToWebCryptoSignature(m_pkey, buf); } else { buf.Resize(len); *out = std::move(buf);