From 61c49c37b7f14905f093fcb803c1e39ea81fed3f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH] src: remove superfluous cipher_ data member The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance, no need to store it separately. This brought to light the somewhat dubious practice of accessing the EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed. It's mostly harmless due to the static nature of built-in EVP_CIPHER instances but it segfaults when the cipher is provided by an ENGINE and the ENGINE is unloaded because its reference count drops to zero. PR-URL: https://github.com/nodejs/node/pull/14122 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- src/node_crypto.cc | 33 ++++++++++++++++++--------------- src/node_crypto.h | 2 -- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 31096b6a82..bd048b5d20 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3332,16 +3332,16 @@ void CipherBase::Init(const char* cipher_type, } #endif // NODE_FIPS_MODE - CHECK_EQ(cipher_, nullptr); - cipher_ = EVP_get_cipherbyname(cipher_type); - if (cipher_ == nullptr) { + CHECK_EQ(initialised_, false); + const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type); + if (cipher == nullptr) { return env()->ThrowError("Unknown cipher"); } unsigned char key[EVP_MAX_KEY_LENGTH]; unsigned char iv[EVP_MAX_IV_LENGTH]; - int key_len = EVP_BytesToKey(cipher_, + int key_len = EVP_BytesToKey(cipher, EVP_md5(), nullptr, reinterpret_cast(key_buf), @@ -3352,7 +3352,7 @@ void CipherBase::Init(const char* cipher_type, EVP_CIPHER_CTX_init(&ctx_); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt); + EVP_CipherInit_ex(&ctx_, cipher, nullptr, nullptr, nullptr, encrypt); if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { EVP_CIPHER_CTX_cleanup(&ctx_); return env()->ThrowError("Invalid key length"); @@ -3394,13 +3394,13 @@ void CipherBase::InitIv(const char* cipher_type, int iv_len) { HandleScope scope(env()->isolate()); - cipher_ = EVP_get_cipherbyname(cipher_type); - if (cipher_ == nullptr) { + const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type); + if (cipher == nullptr) { return env()->ThrowError("Unknown cipher"); } - const int expected_iv_len = EVP_CIPHER_iv_length(cipher_); - const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher_)); + const int expected_iv_len = EVP_CIPHER_iv_length(cipher); + const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher)); if (is_gcm_mode == false && iv_len != expected_iv_len) { return env()->ThrowError("Invalid IV length"); @@ -3408,7 +3408,7 @@ void CipherBase::InitIv(const char* cipher_type, EVP_CIPHER_CTX_init(&ctx_); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt); + EVP_CipherInit_ex(&ctx_, cipher, nullptr, nullptr, nullptr, encrypt); if (is_gcm_mode && !EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) { @@ -3454,10 +3454,10 @@ void CipherBase::InitIv(const FunctionCallbackInfo& args) { bool CipherBase::IsAuthenticatedMode() const { - // check if this cipher operates in an AEAD mode that we support. - if (!cipher_) - return false; - int mode = EVP_CIPHER_mode(cipher_); + // Check if this cipher operates in an AEAD mode that we support. + CHECK_EQ(initialised_, true); + const EVP_CIPHER* const cipher = EVP_CIPHER_CTX_cipher(&ctx_); + int mode = EVP_CIPHER_mode(cipher); return mode == EVP_CIPH_GCM_MODE; } @@ -3644,11 +3644,14 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { CipherBase* cipher; ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); + if (!cipher->initialised_) return env->ThrowError("Unsupported state"); unsigned char* out_value = nullptr; int out_len = -1; Local outString; + // Check IsAuthenticatedMode() first, Final() destroys the EVP_CIPHER_CTX. + const bool is_auth_mode = cipher->IsAuthenticatedMode(); bool r = cipher->Final(&out_value, &out_len); if (out_len <= 0 || !r) { @@ -3656,7 +3659,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { out_value = nullptr; out_len = 0; if (!r) { - const char* msg = cipher->IsAuthenticatedMode() ? + const char* msg = is_auth_mode ? "Unsupported state or unable to authenticate data" : "Unsupported state"; diff --git a/src/node_crypto.h b/src/node_crypto.h index db7a97920b..f46e77d6bc 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -467,7 +467,6 @@ class CipherBase : public BaseObject { v8::Local wrap, CipherKind kind) : BaseObject(env, wrap), - cipher_(nullptr), initialised_(false), kind_(kind), auth_tag_len_(0) { @@ -476,7 +475,6 @@ class CipherBase : public BaseObject { private: EVP_CIPHER_CTX ctx_; /* coverity[member_decl] */ - const EVP_CIPHER* cipher_; /* coverity[member_decl] */ bool initialised_; CipherKind kind_; unsigned int auth_tag_len_;