From 643bc49aada39d54ea0c011456a2b53359955553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 12 Jun 2018 14:15:38 +0200 Subject: [PATCH 1/3] crypto: handle OpenSSL error queue in CipherBase This handles all errors produced by OpenSSL within the CipherBase class. API functions clear the error queue on return, utility functions such as InitAuthenticated() ensure that they do not add any new errors to the queue. Previously ignored return values are now being CHECK'd. Fixes: https://github.com/nodejs/node/issues/21281 Refs: https://github.com/nodejs/node/pull/21287 --- src/node_crypto.cc | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 2339dc833590d2..16b8b3c5398931 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2590,10 +2590,12 @@ void CipherBase::Init(const char* cipher_type, 1, key, iv); + CHECK_NE(key_len, 0); ctx_.reset(EVP_CIPHER_CTX_new()); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, nullptr, nullptr, encrypt); + CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, + nullptr, nullptr, encrypt)); int mode = EVP_CIPHER_CTX_mode(ctx_.get()); if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE || @@ -2616,12 +2618,12 @@ void CipherBase::Init(const char* cipher_type, CHECK_EQ(1, EVP_CIPHER_CTX_set_key_length(ctx_.get(), key_len)); - EVP_CipherInit_ex(ctx_.get(), - nullptr, - nullptr, - reinterpret_cast(key), - reinterpret_cast(iv), - encrypt); + CHECK(EVP_CipherInit_ex(ctx_.get(), + nullptr, + nullptr, + reinterpret_cast(key), + reinterpret_cast(iv), + encrypt)); } @@ -2686,7 +2688,8 @@ void CipherBase::InitIv(const char* cipher_type, EVP_CIPHER_CTX_set_flags(ctx_.get(), EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, nullptr, nullptr, encrypt); + CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, + nullptr, nullptr, encrypt)); if (IsAuthenticatedMode()) { CHECK(has_iv); @@ -2694,17 +2697,18 @@ void CipherBase::InitIv(const char* cipher_type, return; } + ClearErrorOnReturn clear_error_on_return; if (!EVP_CIPHER_CTX_set_key_length(ctx_.get(), key_len)) { ctx_.reset(); return env()->ThrowError("Invalid key length"); } - EVP_CipherInit_ex(ctx_.get(), - nullptr, - nullptr, - reinterpret_cast(key), - reinterpret_cast(iv), - encrypt); + CHECK(EVP_CipherInit_ex(ctx_.get(), + nullptr, + nullptr, + reinterpret_cast(key), + reinterpret_cast(iv), + encrypt)); } @@ -2749,6 +2753,7 @@ static bool IsValidGCMTagLength(unsigned int tag_len) { bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len, unsigned int auth_tag_len) { CHECK(IsAuthenticatedMode()); + MarkPopErrorOnReturn mark_pop_error_on_return; if (!EVP_CIPHER_CTX_ctrl(ctx_.get(), EVP_CTRL_AEAD_SET_IVLEN, @@ -2893,6 +2898,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { bool CipherBase::SetAAD(const char* data, unsigned int len, int plaintext_len) { if (!ctx_ || !IsAuthenticatedMode()) return false; + ClearErrorOnReturn clear_error_on_return; int outlen; const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); @@ -2952,6 +2958,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data, int* out_len) { if (!ctx_) return kErrorState; + ClearErrorOnReturn clear_error_on_return; const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); @@ -2963,10 +2970,10 @@ CipherBase::UpdateResult CipherBase::Update(const char* data, // on first update: if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_len_ > 0 && auth_tag_len_ != kNoAuthTagLength && !auth_tag_set_) { - EVP_CIPHER_CTX_ctrl(ctx_.get(), - EVP_CTRL_GCM_SET_TAG, - auth_tag_len_, - reinterpret_cast(auth_tag_)); + CHECK(EVP_CIPHER_CTX_ctrl(ctx_.get(), + EVP_CTRL_GCM_SET_TAG, + auth_tag_len_, + reinterpret_cast(auth_tag_))); auth_tag_set_ = true; } @@ -3044,6 +3051,7 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { bool CipherBase::SetAutoPadding(bool auto_padding) { if (!ctx_) return false; + ClearErrorOnReturn clear_error_on_return; return EVP_CIPHER_CTX_set_padding(ctx_.get(), auto_padding); } From 0a89f4206382d422b1931cfa14eee0bd7b7a94df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 14 Jun 2018 17:46:47 +0200 Subject: [PATCH 2/3] Don't clear all errors in return Instead of clearing all errors, mark and pop only errors that were added within the current scope. --- src/node_crypto.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 16b8b3c5398931..75f08b7a180e78 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2566,6 +2566,7 @@ void CipherBase::Init(const char* cipher_type, int key_buf_len, unsigned int auth_tag_len) { HandleScope scope(env()->isolate()); + MarkPopErrorOnReturn mark_pop_error_on_return; #ifdef NODE_FIPS_MODE if (FIPS_mode()) { @@ -2658,6 +2659,7 @@ void CipherBase::InitIv(const char* cipher_type, int iv_len, unsigned int auth_tag_len) { HandleScope scope(env()->isolate()); + MarkPopErrorOnReturn mark_pop_error_on_return; const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type); if (cipher == nullptr) { @@ -2697,7 +2699,6 @@ void CipherBase::InitIv(const char* cipher_type, return; } - ClearErrorOnReturn clear_error_on_return; if (!EVP_CIPHER_CTX_set_key_length(ctx_.get(), key_len)) { ctx_.reset(); return env()->ThrowError("Invalid key length"); @@ -2898,7 +2899,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { bool CipherBase::SetAAD(const char* data, unsigned int len, int plaintext_len) { if (!ctx_ || !IsAuthenticatedMode()) return false; - ClearErrorOnReturn clear_error_on_return; + MarkPopErrorOnReturn mark_pop_error_on_return; int outlen; const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); @@ -2958,7 +2959,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data, int* out_len) { if (!ctx_) return kErrorState; - ClearErrorOnReturn clear_error_on_return; + MarkPopErrorOnReturn mark_pop_error_on_return; const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); @@ -3051,7 +3052,7 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { bool CipherBase::SetAutoPadding(bool auto_padding) { if (!ctx_) return false; - ClearErrorOnReturn clear_error_on_return; + MarkPopErrorOnReturn mark_pop_error_on_return; return EVP_CIPHER_CTX_set_padding(ctx_.get(), auto_padding); } From 2fda8de86c3ac799ae865f93eaa9e41e8d1ff765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 1 Jul 2018 13:25:00 +0200 Subject: [PATCH 3/3] Add error checking for EVP_CipherInit_ex --- src/node_crypto.cc | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 75f08b7a180e78..e463e5fa5fa50d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2595,8 +2595,11 @@ void CipherBase::Init(const char* cipher_type, ctx_.reset(EVP_CIPHER_CTX_new()); const bool encrypt = (kind_ == kCipher); - CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, - nullptr, nullptr, encrypt)); + if (1 != EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, + nullptr, nullptr, encrypt)) { + return ThrowCryptoError(env(), ERR_get_error(), + "Failed to initialize cipher"); + } int mode = EVP_CIPHER_CTX_mode(ctx_.get()); if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE || @@ -2619,12 +2622,15 @@ void CipherBase::Init(const char* cipher_type, CHECK_EQ(1, EVP_CIPHER_CTX_set_key_length(ctx_.get(), key_len)); - CHECK(EVP_CipherInit_ex(ctx_.get(), - nullptr, - nullptr, - reinterpret_cast(key), - reinterpret_cast(iv), - encrypt)); + if (1 != EVP_CipherInit_ex(ctx_.get(), + nullptr, + nullptr, + reinterpret_cast(key), + reinterpret_cast(iv), + encrypt)) { + return ThrowCryptoError(env(), ERR_get_error(), + "Failed to initialize cipher"); + } } @@ -2690,8 +2696,11 @@ void CipherBase::InitIv(const char* cipher_type, EVP_CIPHER_CTX_set_flags(ctx_.get(), EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); const bool encrypt = (kind_ == kCipher); - CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, - nullptr, nullptr, encrypt)); + if (1 != EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, + nullptr, nullptr, encrypt)) { + return ThrowCryptoError(env(), ERR_get_error(), + "Failed to initialize cipher"); + } if (IsAuthenticatedMode()) { CHECK(has_iv); @@ -2704,12 +2713,15 @@ void CipherBase::InitIv(const char* cipher_type, return env()->ThrowError("Invalid key length"); } - CHECK(EVP_CipherInit_ex(ctx_.get(), - nullptr, - nullptr, - reinterpret_cast(key), - reinterpret_cast(iv), - encrypt)); + if (1 != EVP_CipherInit_ex(ctx_.get(), + nullptr, + nullptr, + reinterpret_cast(key), + reinterpret_cast(iv), + encrypt)) { + return ThrowCryptoError(env(), ERR_get_error(), + "Failed to initialize cipher"); + } }