This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new ed1fe26c6 [util] fix at-rest {en,de}cryption for OpenSSL 3
ed1fe26c6 is described below
commit ed1fe26c6d7dce49496db1750771963ae4a5f5fa
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]>
---
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 e7c69f186..c73cff393 100644
--- a/src/kudu/util/env-test.cc
+++ b/src/kudu/util/env-test.cc
@@ -1293,8 +1293,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());
@@ -1317,8 +1317,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 519699c77..9b7eb936d 100644
--- a/src/kudu/util/env_posix.cc
+++ b/src/kudu/util/env_posix.cc
@@ -476,7 +476,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];
@@ -488,10 +490,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,
@@ -501,6 +499,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();
}
@@ -515,7 +520,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];
@@ -533,8 +540,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,
@@ -542,6 +547,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();
}
