From ceb1d5e00a0a020f06e822d04c71d003c4beadef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 24 Jun 2024 00:11:18 +0200 Subject: [PATCH] crypto: avoid taking ownership of OpenSSL objects It is often unnecessary to obtain (shared) ownership of OpenSSL objects in this code, and it generally is more costly to do so as opposed to just obtaining a pointer to the respective OpenSSL object. Therefore, this patch replaces various OpenSSL function calls that take ownership with ones that do not. Refs: https://github.com/nodejs/node/pull/53436 PR-URL: https://github.com/nodejs/node/pull/53460 Reviewed-By: Daniel Lemire Reviewed-By: Rafael Gonzaga --- src/crypto/crypto_common.cc | 8 ------ src/crypto/crypto_common.h | 8 ++++++ src/crypto/crypto_keys.cc | 52 ++++++++++++++++++------------------- 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 962018583360a1..85d48dfd2c15c4 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -21,14 +21,6 @@ #include #include -// Some OpenSSL 1.1.1 functions unnecessarily operate on and return non-const -// pointers, whereas the same functions in OpenSSL 3 use const pointers. -#if OPENSSL_VERSION_MAJOR >= 3 -#define OSSL3_CONST const -#else -#define OSSL3_CONST -#endif - namespace node { using v8::Array; diff --git a/src/crypto/crypto_common.h b/src/crypto/crypto_common.h index 9eb3ff4460badd..a11565c1ac0e6c 100644 --- a/src/crypto/crypto_common.h +++ b/src/crypto/crypto_common.h @@ -10,6 +10,14 @@ #include +// Some OpenSSL 1.1.1 functions unnecessarily operate on and return non-const +// pointers, whereas the same functions in OpenSSL 3 use const pointers. +#if OPENSSL_VERSION_MAJOR >= 3 +#define OSSL3_CONST const +#else +#define OSSL3_CONST +#endif + namespace node { namespace crypto { diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index a4979cf5586a7b..d837b265c1ff4a 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -289,11 +289,9 @@ MaybeLocal BIOToStringOrBuffer( } } - -MaybeLocal WritePrivateKey( - Environment* env, - EVP_PKEY* pkey, - const PrivateKeyEncodingConfig& config) { +MaybeLocal WritePrivateKey(Environment* env, + OSSL3_CONST EVP_PKEY* pkey, + const PrivateKeyEncodingConfig& config) { BIOPointer bio(BIO_new(BIO_s_mem())); CHECK(bio); @@ -327,20 +325,21 @@ MaybeLocal WritePrivateKey( // PKCS#1 is only permitted for RSA keys. CHECK_EQ(EVP_PKEY_id(pkey), EVP_PKEY_RSA); - RSAPointer rsa(EVP_PKEY_get1_RSA(pkey)); + OSSL3_CONST RSA* rsa = EVP_PKEY_get0_RSA(pkey); if (config.format_ == kKeyFormatPEM) { // Encode PKCS#1 as PEM. - err = PEM_write_bio_RSAPrivateKey( - bio.get(), rsa.get(), - config.cipher_, - reinterpret_cast(pass), - pass_len, - nullptr, nullptr) != 1; + err = PEM_write_bio_RSAPrivateKey(bio.get(), + rsa, + config.cipher_, + reinterpret_cast(pass), + pass_len, + nullptr, + nullptr) != 1; } else { // Encode PKCS#1 as DER. This does not permit encryption. CHECK_EQ(config.format_, kKeyFormatDER); CHECK_NULL(config.cipher_); - err = i2d_RSAPrivateKey_bio(bio.get(), rsa.get()) != 1; + err = i2d_RSAPrivateKey_bio(bio.get(), rsa) != 1; } } else if (encoding_type == kKeyEncodingPKCS8) { if (config.format_ == kKeyFormatPEM) { @@ -367,20 +366,21 @@ MaybeLocal WritePrivateKey( // SEC1 is only permitted for EC keys. CHECK_EQ(EVP_PKEY_id(pkey), EVP_PKEY_EC); - ECKeyPointer ec_key(EVP_PKEY_get1_EC_KEY(pkey)); + OSSL3_CONST EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(pkey); if (config.format_ == kKeyFormatPEM) { // Encode SEC1 as PEM. - err = PEM_write_bio_ECPrivateKey( - bio.get(), ec_key.get(), - config.cipher_, - reinterpret_cast(pass), - pass_len, - nullptr, nullptr) != 1; + err = PEM_write_bio_ECPrivateKey(bio.get(), + ec_key, + config.cipher_, + reinterpret_cast(pass), + pass_len, + nullptr, + nullptr) != 1; } else { // Encode SEC1 as DER. This does not permit encryption. CHECK_EQ(config.format_, kKeyFormatDER); CHECK_NULL(config.cipher_); - err = i2d_ECPrivateKey_bio(bio.get(), ec_key.get()) != 1; + err = i2d_ECPrivateKey_bio(bio.get(), ec_key) != 1; } } @@ -391,20 +391,20 @@ MaybeLocal WritePrivateKey( return BIOToStringOrBuffer(env, bio.get(), config.format_); } -bool WritePublicKeyInner(EVP_PKEY* pkey, +bool WritePublicKeyInner(OSSL3_CONST EVP_PKEY* pkey, const BIOPointer& bio, const PublicKeyEncodingConfig& config) { if (config.type_.ToChecked() == kKeyEncodingPKCS1) { // PKCS#1 is only valid for RSA keys. CHECK_EQ(EVP_PKEY_id(pkey), EVP_PKEY_RSA); - RSAPointer rsa(EVP_PKEY_get1_RSA(pkey)); + OSSL3_CONST RSA* rsa = EVP_PKEY_get0_RSA(pkey); if (config.format_ == kKeyFormatPEM) { // Encode PKCS#1 as PEM. - return PEM_write_bio_RSAPublicKey(bio.get(), rsa.get()) == 1; + return PEM_write_bio_RSAPublicKey(bio.get(), rsa) == 1; } else { // Encode PKCS#1 as DER. CHECK_EQ(config.format_, kKeyFormatDER); - return i2d_RSAPublicKey_bio(bio.get(), rsa.get()) == 1; + return i2d_RSAPublicKey_bio(bio.get(), rsa) == 1; } } else { CHECK_EQ(config.type_.ToChecked(), kKeyEncodingSPKI); @@ -420,7 +420,7 @@ bool WritePublicKeyInner(EVP_PKEY* pkey, } MaybeLocal WritePublicKey(Environment* env, - EVP_PKEY* pkey, + OSSL3_CONST EVP_PKEY* pkey, const PublicKeyEncodingConfig& config) { BIOPointer bio(BIO_new(BIO_s_mem())); CHECK(bio);