This is an automated email from the ASF dual-hosted git repository. laiyingchun pushed a commit to branch branch-1.17.x in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 0f800db7b62f77a4862cf04721a813f4d7631c63 Author: Alexey Serbin <[email protected]> AuthorDate: Tue Jun 20 14:44:53 2023 -0700 [util] fix at-rest {en,de}cryption for OpenSSL 3 It seems that with OpenSSL 3.0 padding is enabled by default [1]. However, the code in env-posix.cc was relying on an incorrect assumption that the padding would be always disabled for AES-CTR ciphers, and there were no calls to EVP_{En,De}cryptFinal_ex() in Do{En,De}cryptV() correspondingly. This patch addresses the issue, disabling padding for the cipher context explicitly. I also updated the order of the expected/actual arguments for ASSERT_EQ() because the error message was a bit hard to read and comprehend if the corresponding test failed. The motivation for this patch were reports on failed env-test if built and run on RHEL9. Probably, adding calls to EVP_{En,De}cryptFinal_ex() into Do{En,De}cryptV() and making sure they return nothing would be a good invariant to check in DEBUG builds. I added corresponding TODOs in the code since it's better to address that in a separate patch. This is a follow-up to de02a34390f9bdb722be39f96ef9ad9573c0eeeb. [1] https://www.openssl.org/docs/man3.0/man3/EVP_EncryptUpdate.html Change-Id: Ia49a51488a0a39953249f24598daa8219583d575 Reviewed-on: http://gerrit.cloudera.org:8080/20100 Reviewed-by: Attila Bukor <[email protected]> Tested-by: Alexey Serbin <[email protected]> (cherry picked from commit ed1fe26c6d7dce49496db1750771963ae4a5f5fa) Reviewed-on: http://gerrit.cloudera.org:8080/20413 Tested-by: Kudu Jenkins Reviewed-by: Yingchun Lai <[email protected]> --- src/kudu/util/env-test.cc | 8 ++++---- src/kudu/util/env_posix.cc | 26 ++++++++++++++++++-------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/kudu/util/env-test.cc b/src/kudu/util/env-test.cc index c5fad1d06..ae2dd7f2e 100644 --- a/src/kudu/util/env-test.cc +++ b/src/kudu/util/env-test.cc @@ -1292,8 +1292,8 @@ TEST_P(TestEncryptedEnv, TestEncryption) { // Reading back from the RWFile should succeed ASSERT_OK(rw->ReadV(env_->GetEncryptionHeaderSize() + 13, results)); - ASSERT_EQ(result1, "This text"); - ASSERT_EQ(result2, " is slightly longer"); + ASSERT_EQ("This text", result1); + ASSERT_EQ(" is slightly longer", result2); ASSERT_OK(rw->Close()); @@ -1316,8 +1316,8 @@ TEST_P(TestEncryptedEnv, TestEncryption) { ASSERT_OK(seq_file->Read(&result1)); ASSERT_OK(seq_file->Read(&result2)); - ASSERT_EQ(result1, "Hello wor"); - ASSERT_EQ(result2, "ld! This text is sl"); + ASSERT_EQ("Hello wor", result1); + ASSERT_EQ("ld! This text is sl", result2); // Check if the file can be read into a RandomAccessFile if treated properly // as an encrypted file. diff --git a/src/kudu/util/env_posix.cc b/src/kudu/util/env_posix.cc index 82b4d9398..27713f15e 100644 --- a/src/kudu/util/env_posix.cc +++ b/src/kudu/util/env_posix.cc @@ -457,7 +457,9 @@ Status DoEncryptV(const EncryptionHeader* eh, OPENSSL_RET_NOT_OK(EVP_EncryptInit_ex(ctx.get(), GetEVPCipher(eh->algorithm), nullptr, eh->key, iv), "Failed to initialize encryption"); - size_t offset_mod = offset % kEncryptionBlockSize; + OPENSSL_RET_NOT_OK(EVP_CIPHER_CTX_set_padding(ctx.get(), 0), + "failed to disable padding"); + const size_t offset_mod = offset % kEncryptionBlockSize; if (offset_mod) { unsigned char scratch_clear[kEncryptionBlockSize]; unsigned char scratch_cipher[kEncryptionBlockSize]; @@ -469,10 +471,6 @@ Status DoEncryptV(const EncryptionHeader* eh, } for (auto i = 0; i < cleartext.size(); ++i) { int out_length; - // Normally, EVP_EncryptFinal_ex() would be needed after the last chunk of - // data encrypted with EVP_EncryptUpdate(). In Kudu, we only use AES-CTR - // which requires no padding or authentication tags, so - // EVP_EncryptFinal_ex() doesn't actually add anything. OPENSSL_RET_NOT_OK(EVP_EncryptUpdate(ctx.get(), ciphertext[i].mutable_data(), &out_length, @@ -482,6 +480,13 @@ Status DoEncryptV(const EncryptionHeader* eh, DCHECK_EQ(out_length, cleartext[i].size()); DCHECK_LE(out_length, ciphertext[i].size()); } + // EVP_EncryptFinal_ex() would be needed after the last chunk of data + // encrypted with EVP_EncryptUpdate() when padding is enabled. However, + // the logic above assumes no padding, and it is turned off explicitly + // for the cipher context above. + // + // TODO(aserbin): maybe, call EVP_EncryptFinal_ex() in DEBUG mode to make sure + // no data remains in a partial block? return Status::OK(); } @@ -496,7 +501,9 @@ Status DoDecryptV(const EncryptionHeader* eh, uint64_t offset, ArrayView<Slice> OPENSSL_RET_NOT_OK(EVP_DecryptInit_ex(ctx.get(), GetEVPCipher(eh->algorithm), nullptr, eh->key, iv), "Failed to initialize decryption"); - size_t offset_mod = offset % kEncryptionBlockSize; + OPENSSL_RET_NOT_OK(EVP_CIPHER_CTX_set_padding(ctx.get(), 0), + "failed to disable padding"); + const size_t offset_mod = offset % kEncryptionBlockSize; if (offset_mod) { unsigned char scratch_clear[kEncryptionBlockSize]; unsigned char scratch_cipher[kEncryptionBlockSize]; @@ -514,8 +521,6 @@ Status DoDecryptV(const EncryptionHeader* eh, uint64_t offset, ArrayView<Slice> int in_length = ciphertext_slice.size(); if (!in_length || IsAllZeros(ciphertext_slice)) continue; int out_length; - // We don't call EVP_DecryptFinal_ex() after EVP_DecryptUpdate() for the - // same reason we don't call EVP_EncryptFinal_ex(). OPENSSL_RET_NOT_OK(EVP_DecryptUpdate(ctx.get(), data[i].mutable_data(), &out_length, @@ -523,6 +528,11 @@ Status DoDecryptV(const EncryptionHeader* eh, uint64_t offset, ArrayView<Slice> in_length), "Failed to decrypt data"); } + // We don't call EVP_DecryptFinal_ex() after EVP_DecryptUpdate() for the + // same reason we don't call EVP_EncryptFinal_ex() in DoEncryptV(). + // + // TODO(aserbin): maybe, call EVP_DecryptFinal_ex() in DEBUG mode to make sure + // no data remains in a partial block? return Status::OK(); }
