Skip to content

Commit

Permalink
src: add mutex to ManagedEVPPKey class
Browse files Browse the repository at this point in the history 
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:
openssl/openssl#13374
openssl/openssl#2165)
https://www.openssl.org/blog/blog/2017/02/21/threads
  • Loading branch information
@danbev
danbev committed Feb 9, 2021
1 parent beee538 commit c675689
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 35 deletions.
7 changes: 4 additions & 3 deletions src/crypto/crypto_dsa.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,12 @@ Maybe<bool> 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);
Expand Down
20 changes: 12 additions & 8 deletions src/crypto/crypto_ec.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -688,10 +690,11 @@ Maybe<bool> ExportJWKEcKey(
Environment* env,
std::shared_ptr<KeyObjectData> key,
Local<Object> 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);
Expand Down Expand Up @@ -893,10 +896,11 @@ Maybe<bool> GetEcKeyDetail(
Environment* env,
std::shared_ptr<KeyObjectData> key,
Local<Object> 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);
Expand Down
18 changes: 15 additions & 3 deletions src/crypto/crypto_keys.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -564,6 +565,8 @@ ManagedEVPPKey& ManagedEVPPKey::operator=(const ManagedEVPPKey& that) {
if (pkey_)
EVP_PKEY_up_ref(pkey_.get());

mutex_ = that.mutex_;

return *this;
}

Expand All @@ -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 +
Expand Down Expand Up @@ -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);
Expand All @@ -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;

Expand Down
2 changes: 2 additions & 0 deletions src/crypto/crypto_keys.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
23 changes: 13 additions & 10 deletions src/crypto/crypto_rsa.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -363,17 +364,18 @@ Maybe<bool> ExportJWKRsaKey(
Environment* env,
std::shared_ptr<KeyObjectData> key,
Local<Object> 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);

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

Expand Down Expand Up @@ -511,17 +513,18 @@ Maybe<bool> 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);

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

Expand Down
24 changes: 13 additions & 11 deletions src/crypto/crypto_sig.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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(
params->key->GetAsymmetricKey(),
signature.ToByteSource());
ConvertFromWebCryptoSignature(m_pkey, signature.ToByteSource());
} else {
params->signature = mode == kCryptoJobAsync
? signature.ToCopy()
Expand All @@ -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);
Expand All @@ -782,7 +784,7 @@ bool SignTraits::DeriveBits(
&ctx,
params.digest,
nullptr,
params.key->GetAsymmetricKey().get())) {
m_pkey.get())) {
return false;
}
break;
Expand All @@ -793,21 +795,21 @@ bool SignTraits::DeriveBits(
&ctx,
params.digest,
nullptr,
params.key->GetAsymmetricKey().get())) {
m_pkey.get())) {
return false;
}
break;
}

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)) {
Expand Down

0 comments on commit c675689

Please sign in to comment.