IMPALA-7018: fix spill-to-disk encryption err handling The EVP_CIPHER_CTX_ctrl() function was being misused: 1. It was called before initialising the context 2. Errors were not being handled (including the error from #1)
Testing: Added some checks to assert that the OpenSSL error queue is empty. Change-Id: I054a5e76df51b293f4728d30fd3b3cd7640624fb Reviewed-on: http://gerrit.cloudera.org:8080/10385 Reviewed-by: Tim Armstrong <tarmstr...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/675e4c16 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/675e4c16 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/675e4c16 Branch: refs/heads/master Commit: 675e4c161fb39b5825af5ecff10a1a41ebdb3d16 Parents: 3033f3b Author: Tim Armstrong <tarmstr...@cloudera.com> Authored: Fri May 11 16:43:21 2018 -0700 Committer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Committed: Tue May 15 03:37:27 2018 +0000 ---------------------------------------------------------------------- be/src/util/openssl-util-test.cc | 5 ++++- be/src/util/openssl-util.cc | 32 +++++++++++++++++++------------- 2 files changed, 23 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/675e4c16/be/src/util/openssl-util-test.cc ---------------------------------------------------------------------- diff --git a/be/src/util/openssl-util-test.cc b/be/src/util/openssl-util-test.cc index 76f65a5..77cb255 100644 --- a/be/src/util/openssl-util-test.cc +++ b/be/src/util/openssl-util-test.cc @@ -18,6 +18,7 @@ #include <random> #include <gtest/gtest.h> +#include <openssl/err.h> #include <openssl/rand.h> #include "common/init.h" @@ -32,7 +33,9 @@ namespace impala { class OpenSSLUtilTest : public ::testing::Test { protected: virtual void SetUp() { SeedOpenSSLRNG(); } - virtual void TearDown() {} + virtual void TearDown() { + EXPECT_EQ(ERR_peek_error(), 0) << "Did not clear OpenSSL error queue"; + } /// Fill buffer 'data' with 'len' bytes of random binary data from 'rng_'. /// 'len' must be a multiple of 8 bytes'. http://git-wip-us.apache.org/repos/asf/impala/blob/675e4c16/be/src/util/openssl-util.cc ---------------------------------------------------------------------- diff --git a/be/src/util/openssl-util.cc b/be/src/util/openssl-util.cc index 2b368da..065dbc8 100644 --- a/be/src/util/openssl-util.cc +++ b/be/src/util/openssl-util.cc @@ -100,10 +100,10 @@ static int OpenSSLErrCallback(const char* buf, size_t len, void* ctx) { } // Called upon OpenSSL errors; returns a non-OK status with an error message. -static Status OpenSSLErr(const string& function) { +static Status OpenSSLErr(const string& function, const string& context) { stringstream errstream; ERR_print_errors_cb(OpenSSLErrCallback, &errstream); - return Status(Substitute("OpenSSL error in $0: $1", function, errstream.str())); + return Status(Substitute("OpenSSL error in $0 $1: $2", function, context, errstream.str())); } void SeedOpenSSLRNG() { @@ -113,6 +113,7 @@ void SeedOpenSSLRNG() { void IntegrityHash::Compute(const uint8_t* data, int64_t len) { // Explicitly ignore the return value from SHA256(); it can't fail. (void)SHA256(data, len, hash_); + DCHECK_EQ(ERR_peek_error(), 0) << "Did not clear OpenSSL error queue"; } bool IntegrityHash::Verify(const uint8_t* data, int64_t len) const { @@ -144,15 +145,12 @@ Status EncryptionKey::EncryptInternal( bool encrypt, const uint8_t* data, int64_t len, uint8_t* out) { DCHECK(initialized_); DCHECK_GE(len, 0); + const char* err_context = encrypt ? "encrypting" : "decrypting"; // Create and initialize the context for encryption EVP_CIPHER_CTX ctx; EVP_CIPHER_CTX_init(&ctx); EVP_CIPHER_CTX_set_padding(&ctx, 0); - if (IsGcmMode()) { - EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, AES_BLOCK_SIZE, NULL); - } - // Start encryption/decryption. We use a 256-bit AES key, and the cipher block mode // is either CTR or CFB(stream cipher), both of which support arbitrary length // ciphertexts - it doesn't have to be a multiple of 16 bytes. Additionally, CTR @@ -161,9 +159,13 @@ Status EncryptionKey::EncryptInternal( const EVP_CIPHER* evpCipher = GetCipher(); int success = encrypt ? EVP_EncryptInit_ex(&ctx, evpCipher, NULL, key_, iv_) : EVP_DecryptInit_ex(&ctx, evpCipher, NULL, key_, iv_); - if (success != 1) { - return OpenSSLErr(encrypt ? "EVP_EncryptInit_ex" : "EVP_DecryptInit_ex"); + return OpenSSLErr(encrypt ? "EVP_EncryptInit_ex" : "EVP_DecryptInit_ex", err_context); + } + if (IsGcmMode()) { + if (EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, AES_BLOCK_SIZE, NULL) != 1) { + return OpenSSLErr("EVP_CIPHER_CTX_ctrl", err_context); + } } // The OpenSSL encryption APIs use ints for buffer lengths for some reason. To support @@ -176,7 +178,7 @@ Status EncryptionKey::EncryptInternal( EVP_EncryptUpdate(&ctx, out + offset, &out_len, data + offset, in_len) : EVP_DecryptUpdate(&ctx, out + offset, &out_len, data + offset, in_len); if (success != 1) { - return OpenSSLErr(encrypt ? "EVP_EncryptUpdate" : "EVP_DecryptUpdate"); + return OpenSSLErr(encrypt ? "EVP_EncryptUpdate" : "EVP_DecryptUpdate", err_context); } // This is safe because we're using CTR/CFB mode without padding. DCHECK_EQ(in_len, out_len); @@ -185,7 +187,9 @@ Status EncryptionKey::EncryptInternal( if (IsGcmMode() && !encrypt) { // Set expected tag value - EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_TAG, AES_BLOCK_SIZE, gcm_tag_); + if (EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_TAG, AES_BLOCK_SIZE, gcm_tag_) != 1) { + return OpenSSLErr("EVP_CIPHER_CTX_ctrl", err_context); + } } // Finalize encryption or decryption. @@ -193,15 +197,17 @@ Status EncryptionKey::EncryptInternal( success = encrypt ? EVP_EncryptFinal_ex(&ctx, out + offset, &final_out_len) : EVP_DecryptFinal_ex(&ctx, out + offset, &final_out_len); if (success != 1) { - return OpenSSLErr(encrypt ? "EVP_EncryptFinal" : "EVP_DecryptFinal"); + return OpenSSLErr(encrypt ? "EVP_EncryptFinal" : "EVP_DecryptFinal", err_context); } if (IsGcmMode() && encrypt) { - EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_GET_TAG, AES_BLOCK_SIZE, gcm_tag_); + if (EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_GET_TAG, AES_BLOCK_SIZE, gcm_tag_) != 1) { + return OpenSSLErr("EVP_CIPHER_CTX_ctrl", err_context); + } } - // Again safe due to GCM/CTR/CFB with no padding DCHECK_EQ(final_out_len, 0); + DCHECK_EQ(ERR_peek_error(), 0) << "Did not clear OpenSSL error queue"; return Status::OK(); }
