From 4c1f6ac496dbf96f9649c04aaa0dcf4089be1234 Mon Sep 17 00:00:00 2001 From: David Drysdale Date: Thu, 25 Nov 2021 16:08:29 +0000 Subject: [PATCH] Fix flaky corrupted padding tests Transfer the fix in http://aosp/1745035 from the KeyMint VTS test back into the keymaster VTS test. Bug: 189261752 Test: VtsHalKeymasterV4_0TargetTest Change-Id: I5f0a69255cfe980dd6e71fa29ff06a84cb668f6d --- .../functional/keymaster_hidl_hal_test.cpp | 76 ++++++++++++++----- .../aidl/vts/functional/KeyMintTest.cpp | 2 +- 2 files changed, 59 insertions(+), 19 deletions(-) diff --git a/keymaster/4.0/vts/functional/keymaster_hidl_hal_test.cpp b/keymaster/4.0/vts/functional/keymaster_hidl_hal_test.cpp index 476eed8b19..773715eabf 100644 --- a/keymaster/4.0/vts/functional/keymaster_hidl_hal_test.cpp +++ b/keymaster/4.0/vts/functional/keymaster_hidl_hal_test.cpp @@ -81,6 +81,12 @@ bool operator==(const KeyCharacteristics& a, const KeyCharacteristics& b) { namespace test { namespace { +// The maximum number of times we'll attempt to verify that corruption +// of an encrypted blob results in an error. Retries are necessary as there +// is a small (roughly 1/256) chance that corrupting ciphertext still results +// in valid PKCS7 padding. +constexpr size_t kMaxPaddingCorruptionRetries = 8; + template bool contains(hidl_vec& set, TypedTag ttag, ValueT expected_value) { size_t count = std::count_if(set.begin(), set.end(), [&](const KeyParameter& param) { @@ -2849,11 +2855,22 @@ TEST_P(EncryptionOperationsTest, AesEcbPkcs7PaddingCorrupted) { string ciphertext = EncryptMessage(message, params); EXPECT_EQ(16U, ciphertext.size()); EXPECT_NE(ciphertext, message); - ++ciphertext[ciphertext.size() / 2]; - EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, params)); - string plaintext; - EXPECT_EQ(ErrorCode::INVALID_INPUT_LENGTH, Finish(message, &plaintext)); + for (size_t i = 0; i < kMaxPaddingCorruptionRetries; ++i) { + ++ciphertext[ciphertext.size() / 2]; + + EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, params)); + string plaintext; + ErrorCode error = Finish(message, &plaintext); + if (error == ErrorCode::INVALID_INPUT_LENGTH) { + // This is the expected error, we can exit the test now. + return; + } else { + // Very small chance we got valid decryption, so try again. + ASSERT_EQ(error, ErrorCode::OK); + } + } + FAIL() << "Corrupt ciphertext should have failed to decrypt by now."; } HidlBuf CopyIv(const AuthorizationSet& set) { @@ -3876,17 +3893,30 @@ TEST_P(EncryptionOperationsTest, TripleDesEcbPkcs7PaddingCorrupted) { string ciphertext = EncryptMessage(message, BlockMode::ECB, PaddingMode::PKCS7); EXPECT_EQ(8U, ciphertext.size()); EXPECT_NE(ciphertext, message); - ++ciphertext[ciphertext.size() / 2]; AuthorizationSetBuilder begin_params; begin_params.push_back(TAG_BLOCK_MODE, BlockMode::ECB); begin_params.push_back(TAG_PADDING, PaddingMode::PKCS7); - EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, begin_params)); - string plaintext; - size_t input_consumed; - EXPECT_EQ(ErrorCode::OK, Update(ciphertext, &plaintext, &input_consumed)); - EXPECT_EQ(ciphertext.size(), input_consumed); - EXPECT_EQ(ErrorCode::INVALID_ARGUMENT, Finish(&plaintext)); + + for (size_t i = 0; i < kMaxPaddingCorruptionRetries; ++i) { + ++ciphertext[ciphertext.size() / 2]; + + EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, begin_params)); + string plaintext; + + size_t input_consumed; + EXPECT_EQ(ErrorCode::OK, Update(ciphertext, &plaintext, &input_consumed)); + EXPECT_EQ(ciphertext.size(), input_consumed); + ErrorCode error = Finish(&plaintext); + if (error == ErrorCode::INVALID_ARGUMENT) { + // This is the expected error, we can exit the test now. + return; + } else { + // Very small chance we got valid decryption, so try again. + ASSERT_EQ(error, ErrorCode::OK); + } + } + FAIL() << "Corrupt ciphertext should have failed to decrypt by now."; } struct TripleDesTestVector { @@ -4187,18 +4217,28 @@ TEST_P(EncryptionOperationsTest, TripleDesCbcPkcs7PaddingCorrupted) { string ciphertext = EncryptMessage(message, BlockMode::CBC, PaddingMode::PKCS7, &iv); EXPECT_EQ(8U, ciphertext.size()); EXPECT_NE(ciphertext, message); - ++ciphertext[ciphertext.size() / 2]; auto begin_params = AuthorizationSetBuilder() .BlockMode(BlockMode::CBC) .Padding(PaddingMode::PKCS7) .Authorization(TAG_NONCE, iv); - EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, begin_params)); - string plaintext; - size_t input_consumed; - EXPECT_EQ(ErrorCode::OK, Update(ciphertext, &plaintext, &input_consumed)); - EXPECT_EQ(ciphertext.size(), input_consumed); - EXPECT_EQ(ErrorCode::INVALID_ARGUMENT, Finish(&plaintext)); + for (size_t i = 0; i < kMaxPaddingCorruptionRetries; ++i) { + ++ciphertext[ciphertext.size() / 2]; + EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, begin_params)); + string plaintext; + size_t input_consumed; + EXPECT_EQ(ErrorCode::OK, Update(ciphertext, &plaintext, &input_consumed)); + EXPECT_EQ(ciphertext.size(), input_consumed); + ErrorCode error = Finish(&plaintext); + if (error == ErrorCode::INVALID_ARGUMENT) { + // This is the expected error, we can exit the test now. + return; + } else { + // Very small chance we got valid decryption, so try again. + ASSERT_EQ(error, ErrorCode::OK); + } + } + FAIL() << "Corrupt ciphertext should have failed to decrypt by now."; } /* diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp index 92aa2ac64b..53a5490cce 100644 --- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp @@ -70,7 +70,7 @@ namespace aidl::android::hardware::security::keymint::test { namespace { // The maximum number of times we'll attempt to verify that corruption -// of an ecrypted blob results in an error. Retries are necessary as there +// of an encrypted blob results in an error. Retries are necessary as there // is a small (roughly 1/256) chance that corrupting ciphertext still results // in valid PKCS7 padding. constexpr size_t kMaxPaddingCorruptionRetries = 8;