From 534085b76a747e2ce7a6bcf28f9bff4a1e5717ed Mon Sep 17 00:00:00 2001 From: Shawn Willden Date: Tue, 26 Nov 2019 14:58:00 -0700 Subject: [PATCH] Remove creation time checks. We should not be relying on the HAL service to add CREATION_TIME to keys. It was always intended to be an optional tag that could be added by keystore, or maybe the caller of keystore. One widespread Keymaster implementation started adding it (somewhat erroneously) if it wasn't provided, and it appears that this implementation's behavior became assumed to be the required behavior. Test: VtsHalKeymasterV4_0TargetTest Change-Id: I34267c4e1f59fd8ee5f898f8c746a7b49f4d74a5 --- .../4.0/vts/functional/KeymasterHidlTest.cpp | 16 ------ .../4.0/vts/functional/KeymasterHidlTest.h | 3 -- .../functional/keymaster_hidl_hal_test.cpp | 49 ++----------------- 3 files changed, 4 insertions(+), 64 deletions(-) diff --git a/keymaster/4.0/vts/functional/KeymasterHidlTest.cpp b/keymaster/4.0/vts/functional/KeymasterHidlTest.cpp index 07409f6734..7241984694 100644 --- a/keymaster/4.0/vts/functional/KeymasterHidlTest.cpp +++ b/keymaster/4.0/vts/functional/KeymasterHidlTest.cpp @@ -201,22 +201,6 @@ void KeymasterHidlTest::CheckedDeleteKey() { CheckedDeleteKey(&key_blob_); } -void KeymasterHidlTest::CheckCreationDateTime( - const AuthorizationSet& sw_enforced, - std::chrono::time_point creation) { - for (int i = 0; i < sw_enforced.size(); i++) { - if (sw_enforced[i].tag == TAG_CREATION_DATETIME) { - std::chrono::time_point now = - std::chrono::system_clock::now(); - std::chrono::time_point reported_time{ - std::chrono::milliseconds(sw_enforced[i].f.dateTime)}; - // The test is flaky for EC keys, so a buffer time of 120 seconds will be added. - EXPECT_LE(creation - std::chrono::seconds(120), reported_time); - EXPECT_LE(reported_time, now + std::chrono::seconds(1)); - } - } -} - void KeymasterHidlTest::CheckGetCharacteristics(const HidlBuf& key_blob, const HidlBuf& client_id, const HidlBuf& app_data, KeyCharacteristics* key_characteristics) { diff --git a/keymaster/4.0/vts/functional/KeymasterHidlTest.h b/keymaster/4.0/vts/functional/KeymasterHidlTest.h index adceead2e7..4bd8b26db9 100644 --- a/keymaster/4.0/vts/functional/KeymasterHidlTest.h +++ b/keymaster/4.0/vts/functional/KeymasterHidlTest.h @@ -113,9 +113,6 @@ class KeymasterHidlTest : public ::testing::TestWithParam { void CheckedDeleteKey(HidlBuf* key_blob, bool keep_key_blob = false); void CheckedDeleteKey(); - static void CheckCreationDateTime(const AuthorizationSet& sw_enforced, - std::chrono::time_point creation); - void CheckGetCharacteristics(const HidlBuf& key_blob, const HidlBuf& client_id, const HidlBuf& app_data, KeyCharacteristics* key_characteristics); ErrorCode GetCharacteristics(const HidlBuf& key_blob, const HidlBuf& client_id, 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 35a2fb15a9..d2dfebfb53 100644 --- a/keymaster/4.0/vts/functional/keymaster_hidl_hal_test.cpp +++ b/keymaster/4.0/vts/functional/keymaster_hidl_hal_test.cpp @@ -320,8 +320,7 @@ bool avb_verification_enabled() { bool verify_attestation_record(const string& challenge, const string& app_id, AuthorizationSet expected_sw_enforced, AuthorizationSet expected_hw_enforced, SecurityLevel security_level, - const hidl_vec& attestation_cert, - std::chrono::time_point creation_time) { + const hidl_vec& attestation_cert) { X509_Ptr cert(parse_cert_blob(attestation_cert)); EXPECT_TRUE(!!cert.get()); if (!cert.get()) return false; @@ -405,8 +404,6 @@ bool verify_attestation_record(const string& challenge, const string& app_id, EXPECT_FALSE(expected_hw_enforced.Contains(TAG_TRUSTED_USER_PRESENCE_REQUIRED)); EXPECT_FALSE(att_hw_enforced.Contains(TAG_TRUSTED_USER_PRESENCE_REQUIRED)); - KeymasterHidlTest::CheckCreationDateTime(att_sw_enforced, creation_time); - if (att_hw_enforced.Contains(TAG_ALGORITHM, Algorithm::EC)) { // For ECDSA keys, either an EC_CURVE or a KEY_SIZE can be specified, but one must be. EXPECT_TRUE(att_hw_enforced.Contains(TAG_EC_CURVE) || @@ -552,24 +549,6 @@ TEST_P(NewKeyGenerationTest, Rsa) { } } -/* - * NewKeyGenerationTest.RsaCheckCreationDateTime - * - * Verifies that creation date time is correct. - */ -TEST_P(NewKeyGenerationTest, RsaCheckCreationDateTime) { - KeyCharacteristics key_characteristics; - auto creation_time = std::chrono::system_clock::now(); - ASSERT_EQ(ErrorCode::OK, GenerateKey(AuthorizationSetBuilder() - .Authorization(TAG_NO_AUTH_REQUIRED) - .RsaSigningKey(2048, 3) - .Digest(Digest::NONE) - .Padding(PaddingMode::NONE))); - GetCharacteristics(key_blob_, &key_characteristics); - AuthorizationSet sw_enforced = key_characteristics.softwareEnforced; - CheckCreationDateTime(sw_enforced, creation_time); -} - /* * NewKeyGenerationTest.NoInvalidRsaSizes * @@ -634,23 +613,6 @@ TEST_P(NewKeyGenerationTest, Ecdsa) { } } -/* - * NewKeyGenerationTest.EcCheckCreationDateTime - * - * Verifies that creation date time is correct. - */ -TEST_P(NewKeyGenerationTest, EcCheckCreationDateTime) { - KeyCharacteristics key_characteristics; - auto creation_time = std::chrono::system_clock::now(); - ASSERT_EQ(ErrorCode::OK, GenerateKey(AuthorizationSetBuilder() - .Authorization(TAG_NO_AUTH_REQUIRED) - .EcdsaSigningKey(256) - .Digest(Digest::NONE))); - GetCharacteristics(key_blob_, &key_characteristics); - AuthorizationSet sw_enforced = key_characteristics.softwareEnforced; - CheckCreationDateTime(sw_enforced, creation_time); -} - /* * NewKeyGenerationTest.EcdsaDefaultSize * @@ -4232,7 +4194,6 @@ typedef KeymasterHidlTest AttestationTest; * Verifies that attesting to RSA keys works and generates the expected output. */ TEST_P(AttestationTest, RsaAttestation) { - auto creation_time = std::chrono::system_clock::now(); ASSERT_EQ(ErrorCode::OK, GenerateKey(AuthorizationSetBuilder() .Authorization(TAG_NO_AUTH_REQUIRED) .RsaSigningKey(2048, 65537) @@ -4257,7 +4218,7 @@ TEST_P(AttestationTest, RsaAttestation) { EXPECT_TRUE(verify_attestation_record("challenge", "foo", // key_characteristics_.softwareEnforced, // key_characteristics_.hardwareEnforced, // - SecLevel(), cert_chain[0], creation_time)); + SecLevel(), cert_chain[0])); } /* @@ -4286,7 +4247,6 @@ TEST_P(AttestationTest, RsaAttestationRequiresAppId) { * Verifies that attesting to EC keys works and generates the expected output. */ TEST_P(AttestationTest, EcAttestation) { - auto creation_time = std::chrono::system_clock::now(); ASSERT_EQ(ErrorCode::OK, GenerateKey(AuthorizationSetBuilder() .Authorization(TAG_NO_AUTH_REQUIRED) .EcdsaSigningKey(EcCurve::P_256) @@ -4308,7 +4268,7 @@ TEST_P(AttestationTest, EcAttestation) { EXPECT_TRUE(verify_attestation_record("challenge", "foo", // key_characteristics_.softwareEnforced, // key_characteristics_.hardwareEnforced, // - SecLevel(), cert_chain[0], creation_time)); + SecLevel(), cert_chain[0])); } /* @@ -4341,7 +4301,6 @@ TEST_P(AttestationTest, EcAttestationRequiresAttestationAppId) { TEST_P(AttestationTest, AttestationApplicationIDLengthProperlyEncoded) { std::vector app_id_lengths{143, 258}; for (uint32_t length : app_id_lengths) { - auto creation_time = std::chrono::system_clock::now(); ASSERT_EQ(ErrorCode::OK, GenerateKey(AuthorizationSetBuilder() .Authorization(TAG_NO_AUTH_REQUIRED) .EcdsaSigningKey(EcCurve::P_256) @@ -4359,7 +4318,7 @@ TEST_P(AttestationTest, AttestationApplicationIDLengthProperlyEncoded) { EXPECT_TRUE(verify_attestation_record("challenge", app_id, // key_characteristics_.softwareEnforced, // key_characteristics_.hardwareEnforced, // - SecLevel(), cert_chain[0], creation_time)); + SecLevel(), cert_chain[0])); CheckedDeleteKey(); } }