From eabd9d6d2ac5b50dc06eb0c9ac2c3d8715eec030 Mon Sep 17 00:00:00 2001 From: Brian J Murray Date: Thu, 6 Jan 2022 15:13:51 -0800 Subject: Verify op_ is not a nullptr If op_ is a nullptr, the test runner can SIGSEGV. Test: manual, atest EncryptionOperationsTest#TripleDesCbcPkcs7PaddingCorrupted Bug: None Signed-off-by: Brian J Murray Change-Id: Ibdd6139952ca8bc83ac1a82202839feee39562e1 --- security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'security/keymint/aidl') diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp index 3695f1e094..02462fce3a 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp @@ -537,6 +537,9 @@ ErrorCode KeyMintAidlTestBase::Update(const string& input, string* output) { Status result; if (!output) return ErrorCode::UNEXPECTED_NULL_POINTER; + EXPECT_NE(op_, nullptr); + if (!op_) return ErrorCode::UNEXPECTED_NULL_POINTER; + std::vector o_put; result = op_->update(vector(input.begin(), input.end()), {}, {}, &o_put); -- cgit v1.2.3 From ab1851e9f2c40942fff243504788795aeaf89961 Mon Sep 17 00:00:00 2001 From: David Drysdale Date: Tue, 14 Dec 2021 14:32:51 +0000 Subject: Alter spec text for RSA-PSS to match reality The Key{Mint,Master} spec previously said that RSA-PSS mode should use SHA-1 for the MGF1 digest, separately from whatever Tag::DIGEST gets specified as the main digest. However, both the reference implementation and the VTS/CTS tests use BoringSSL's defaults, which is to re-use the main digest as the MGF1 digest if none is separately specified. Given that this behaviour is embedded in many implementations over several years (and given that there isn't a security implication), change the spec to match this behaviour. Also update the VTS test code to make this clear/obvious. Test: VtsAidlKeyMintTargetTest, VtsHalKeymasterV4_0TargetTest Bug: 210424594 Change-Id: I4303f28d094ef4d4b9dc931d6728b1fa040de20d Ignore-AOSP-First: target internal master first due to merge conflict --- .../aidl/android/hardware/security/keymint/IKeyMintOperation.aidl | 3 ++- security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) (limited to 'security/keymint/aidl') diff --git a/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl b/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl index ce83044d0e..ca8955552e 100644 --- a/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl +++ b/security/keymint/aidl/android/hardware/security/keymint/IKeyMintOperation.aidl @@ -227,7 +227,8 @@ interface IKeyMintOperation { * o PaddingMode::RSA_PSS. For PSS-padded signature operations, the PSS salt length must match * the size of the PSS digest selected. The digest specified with Tag::DIGEST in params * on begin() must be used as the PSS digest algorithm, MGF1 must be used as the mask - * generation function and SHA1 must be used as the MGF1 digest algorithm. + * generation function and the digest specified with Tag:DIGEST in params on begin() must also + * be used as the MGF1 digest algorithm. * * -- ECDSA keys -- * diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp index 02462fce3a..374f2da7a8 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp @@ -812,6 +812,7 @@ void KeyMintAidlTestBase::LocalVerifyMessage(const string& message, const string if (padding == PaddingMode::RSA_PSS) { EXPECT_GT(EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING), 0); EXPECT_GT(EVP_PKEY_CTX_set_rsa_pss_saltlen(pkey_ctx, EVP_MD_size(md)), 0); + EXPECT_GT(EVP_PKEY_CTX_set_rsa_mgf1_md(pkey_ctx, md), 0); } ASSERT_EQ(1, EVP_DigestVerifyUpdate(&digest_ctx, -- cgit v1.2.3 From 734c841dafbf2018efd67ef214b28048998a28ce Mon Sep 17 00:00:00 2001 From: Brian J Murray Date: Thu, 13 Jan 2022 14:55:30 -0800 Subject: Block cipher fixups Various block cipher testing fixups. Some of these changes reflect edge cases I encountered when running local GSC builds. Change: * Extend ciphertext lengths. * Add SCOPED_TRACE() within for loops. * Use '\t' instead of 'a' for PKCS7 padding. Test: CTS/VTS Signed-off-by: Brian J Murray Change-Id: I4555519787e0133367ad3f40609d43a7bc71c36e --- .../keymint/aidl/vts/functional/KeyMintTest.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'security/keymint/aidl') diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp index 5b80b6fac0..d109058b8f 100644 --- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp @@ -4608,8 +4608,10 @@ TEST_P(EncryptionOperationsTest, AesEcbPkcs7Padding) { auto params = AuthorizationSetBuilder().BlockMode(BlockMode::ECB).Padding(PaddingMode::PKCS7); // Try various message lengths; all should work. - for (size_t i = 0; i < 32; ++i) { - string message(i, 'a'); + for (size_t i = 0; i <= 48; i++) { + SCOPED_TRACE(testing::Message() << "i = " << i); + // Edge case: '\t' (0x09) is also a valid PKCS7 padding character. + string message(i, '\t'); string ciphertext = EncryptMessage(message, params); EXPECT_EQ(i + 16 - (i % 16), ciphertext.size()); string plaintext = DecryptMessage(ciphertext, params); @@ -4633,7 +4635,7 @@ TEST_P(EncryptionOperationsTest, AesEcbWrongPadding) { auto params = AuthorizationSetBuilder().BlockMode(BlockMode::ECB).Padding(PaddingMode::PKCS7); // Try various message lengths; all should fail - for (size_t i = 0; i < 32; ++i) { + for (size_t i = 0; i <= 48; i++) { string message(i, 'a'); EXPECT_EQ(ErrorCode::INCOMPATIBLE_PADDING_MODE, Begin(KeyPurpose::ENCRYPT, params)); } @@ -5775,8 +5777,8 @@ TEST_P(EncryptionOperationsTest, TripleDesCbcRoundTripSuccess) { ASSERT_GT(key_blob_.size(), 0U); - // Two-block message. - string message = "1234567890123456"; + // Four-block message. + string message = "12345678901234561234567890123456"; vector iv1; string ciphertext1 = EncryptMessage(message, BlockMode::CBC, PaddingMode::NONE, &iv1); EXPECT_EQ(message.size(), ciphertext1.size()); @@ -5936,8 +5938,10 @@ TEST_P(EncryptionOperationsTest, TripleDesCbcPkcs7Padding) { .Padding(PaddingMode::PKCS7))); // Try various message lengths; all should work. - for (size_t i = 0; i < 32; ++i) { - string message(i, 'a'); + for (size_t i = 0; i <= 32; i++) { + SCOPED_TRACE(testing::Message() << "i = " << i); + // Edge case: '\t' (0x09) is also a valid PKCS7 padding character, albeit not for 3DES. + string message(i, '\t'); vector iv; string ciphertext = EncryptMessage(message, BlockMode::CBC, PaddingMode::PKCS7, &iv); EXPECT_EQ(i + 8 - (i % 8), ciphertext.size()); @@ -5959,7 +5963,7 @@ TEST_P(EncryptionOperationsTest, TripleDesCbcNoPaddingKeyWithPkcs7Padding) { .Padding(PaddingMode::NONE))); // Try various message lengths; all should fail. - for (size_t i = 0; i < 32; ++i) { + for (size_t i = 0; i <= 32; i++) { auto begin_params = AuthorizationSetBuilder().BlockMode(BlockMode::CBC).Padding(PaddingMode::PKCS7); EXPECT_EQ(ErrorCode::INCOMPATIBLE_PADDING_MODE, Begin(KeyPurpose::ENCRYPT, begin_params)); @@ -5990,6 +5994,7 @@ TEST_P(EncryptionOperationsTest, TripleDesCbcPkcs7PaddingCorrupted) { .Authorization(TAG_NONCE, iv); for (size_t i = 0; i < kMaxPaddingCorruptionRetries; ++i) { + SCOPED_TRACE(testing::Message() << "i = " << i); ++ciphertext[ciphertext.size() / 2]; EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::DECRYPT, begin_params)); string plaintext; -- cgit v1.2.3