Fix AES corrupt padding test
The AesEcbPkcs7PaddingCorrupted test has been incorrect since it was
originally introduced -- it was feeding the original message as input to
the decryption operation, rather than the corrupted ciphertext. As a
result, the expected error code was also wrong -- INVALID_INPUT_LENGTH
is appropriate for a too-short cipher text (length 1 in this case),
whereas a corrupt-but-correct-length cipher text should give
INVALID_ARGUMENT.
Fix the test, and add a separate test to cover what was inadvertently
being tested before. Add a sentence to the HAL spec to describe what
expected and tested by CTS/VTS.
Bug: 194126736
Test: VtsAidlKeyMintTargetTest, VtsHalKeymasterV4_0TargetTest
Change-Id: Iaa5e42768814197f373797831093cf344d342b77
diff --git a/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl b/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl
index ca89555..c30c183 100644
--- a/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl
+++ b/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl
@@ -242,7 +242,8 @@
* not a multiple of the AES block size, finish() must return
* ErrorCode::INVALID_INPUT_LENGTH. If padding is PaddingMode::PKCS7, pad the data per the
* PKCS#7 specification, including adding an additional padding block if the data is a
- * multiple of the block length.
+ * multiple of the block length. If padding is PaddingMode::PKCS7 and decryption does not
+ * result in valid padding, return ErrorCode::INVALID_ARGUMENT.
*
* o BlockMode::GCM. During encryption, after processing all plaintext, compute the tag
* (Tag::MAC_LENGTH bytes) and append it to the returned ciphertext. During decryption,
diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp
index 3b75c50..384c135 100644
--- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp
+++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp
@@ -5481,18 +5481,45 @@
EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, params));
string plaintext;
- ErrorCode error = Finish(message, &plaintext);
- if (error == ErrorCode::INVALID_INPUT_LENGTH) {
+ ErrorCode error = Finish(ciphertext, &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);
+ ASSERT_EQ(error, ErrorCode::OK)
+ << "Expected INVALID_ARGUMENT or (rarely) OK, got " << error;
}
}
FAIL() << "Corrupt ciphertext should have failed to decrypt by now.";
}
+/*
+ * EncryptionOperationsTest.AesEcbPkcs7CiphertextTooShort
+ *
+ * Verifies that AES decryption fails in the correct way when the padding is corrupted.
+ */
+TEST_P(EncryptionOperationsTest, AesEcbPkcs7CiphertextTooShort) {
+ ASSERT_EQ(ErrorCode::OK, GenerateKey(AuthorizationSetBuilder()
+ .Authorization(TAG_NO_AUTH_REQUIRED)
+ .AesEncryptionKey(128)
+ .Authorization(TAG_BLOCK_MODE, BlockMode::ECB)
+ .Padding(PaddingMode::PKCS7)));
+
+ auto params = AuthorizationSetBuilder().BlockMode(BlockMode::ECB).Padding(PaddingMode::PKCS7);
+
+ string message = "a";
+ string ciphertext = EncryptMessage(message, params);
+ EXPECT_EQ(16U, ciphertext.size());
+ EXPECT_NE(ciphertext, message);
+
+ // Shorten the ciphertext.
+ ciphertext.resize(ciphertext.size() - 1);
+ EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, params));
+ string plaintext;
+ EXPECT_EQ(ErrorCode::INVALID_INPUT_LENGTH, Finish(ciphertext, &plaintext));
+}
+
vector<uint8_t> CopyIv(const AuthorizationSet& set) {
auto iv = set.GetTagValue(TAG_NONCE);
EXPECT_TRUE(iv);