From 0c5b6503ed6d6a54b87b6be33f7662960a84f4bd Mon Sep 17 00:00:00 2001 From: Karuna Wadhera Date: Tue, 9 Jul 2024 20:21:44 +0000 Subject: [PATCH] Amend tests on GSI that rely on RKP-only props MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GSI replaces the values for remote_prov_prop properties (since they’re system_internal_prop properties), so on GSI the properties are not reliable indicators of whether StrongBox/TEE are RKP-only or not. Also included is the removal of the helper skipAttestKeyTestIfNeeded() so the skipping can happen in the tests directly. Bug: 348159232 Test: VtsAidlKeyMintTargetTest Change-Id: I2075e1f76ddd0f87620a212e1aa389803139a117 --- .../aidl/vts/functional/AttestKeyTest.cpp | 31 ++++++++++++------- .../vts/functional/KeyMintAidlTestBase.cpp | 23 +++++++------- .../aidl/vts/functional/KeyMintAidlTestBase.h | 3 +- 3 files changed, 33 insertions(+), 24 deletions(-) diff --git a/security/keymint/aidl/vts/functional/AttestKeyTest.cpp b/security/keymint/aidl/vts/functional/AttestKeyTest.cpp index 5106561984..464883eba2 100644 --- a/security/keymint/aidl/vts/functional/AttestKeyTest.cpp +++ b/security/keymint/aidl/vts/functional/AttestKeyTest.cpp @@ -39,7 +39,9 @@ bool IsSelfSigned(const vector& chain) { class AttestKeyTest : public KeyMintAidlTestBase { public: void SetUp() override { - skipAttestKeyTestIfNeeded(); + if (shouldSkipAttestKeyTest()) { + GTEST_SKIP() << "Test using ATTEST_KEY is not applicable on waivered device"; + } KeyMintAidlTestBase::SetUp(); } }; @@ -251,7 +253,11 @@ TEST_P(AttestKeyTest, RsaAttestedAttestKeys) { .SetDefaultValidity(), {} /* attestation signing key */, &attest_key.keyBlob, &attest_key_characteristics, &attest_key_cert_chain); - if (isRkpOnly() && result == ErrorCode::ATTESTATION_KEYS_NOT_PROVISIONED) { + std::optional rkpOnly = isRkpOnly(); + if (!rkpOnly.has_value()) { + GTEST_SKIP() << "Test not applicable because RKP-only status cannot be determined"; + } + if (rkpOnly.value() && result == ErrorCode::ATTESTATION_KEYS_NOT_PROVISIONED) { GTEST_SKIP() << "RKP-only devices do not have a factory key"; } ASSERT_EQ(ErrorCode::OK, result); @@ -355,7 +361,8 @@ TEST_P(AttestKeyTest, RsaAttestKeyChaining) { .Authorization(TAG_CERTIFICATE_SUBJECT, subject_der) .SetDefaultValidity(); // In RKP-only systems, the first key cannot be attested due to lack of batch key - if (!isRkpOnly() || i > 0) { + bool confirmedNotRkpOnly = !isRkpOnly().value_or(true); + if (confirmedNotRkpOnly || i > 0) { auth_set_builder.AttestationChallenge("foo"); } auto result = GenerateAttestKey(auth_set_builder, attest_key_opt, &key_blob_list[i], @@ -363,7 +370,7 @@ TEST_P(AttestKeyTest, RsaAttestKeyChaining) { ASSERT_EQ(ErrorCode::OK, result); deleters.push_back(KeyBlobDeleter(keymint_, key_blob_list[i])); - if (!isRkpOnly() || i > 0) { + if (confirmedNotRkpOnly || i > 0) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); ASSERT_GT(cert_chain_list[i].size(), 0); @@ -386,7 +393,7 @@ TEST_P(AttestKeyTest, RsaAttestKeyChaining) { } EXPECT_TRUE(ChainSignaturesAreValid(cert_chain_list[i])); - EXPECT_GT(cert_chain_list[i].size(), i + (isRkpOnly() ? 0 : 1)); + EXPECT_GT(cert_chain_list[i].size(), i + (confirmedNotRkpOnly ? 1 : 0)); verify_subject_and_serial(cert_chain_list[i][0], serial_int, subject, false); } } @@ -432,7 +439,8 @@ TEST_P(AttestKeyTest, EcAttestKeyChaining) { .Authorization(TAG_NO_AUTH_REQUIRED) .SetDefaultValidity(); // In RKP-only systems, the first key cannot be attested due to lack of batch key - if (!isRkpOnly() || i > 0) { + bool confirmedNotRkpOnly = !isRkpOnly().value_or(true); + if (confirmedNotRkpOnly || i > 0) { auth_set_builder.AttestationChallenge("foo"); } auto result = GenerateAttestKey(auth_set_builder, attest_key_opt, &key_blob_list[i], @@ -440,7 +448,7 @@ TEST_P(AttestKeyTest, EcAttestKeyChaining) { ASSERT_EQ(ErrorCode::OK, result); deleters.push_back(KeyBlobDeleter(keymint_, key_blob_list[i])); - if (!isRkpOnly() || i > 0) { + if (confirmedNotRkpOnly || i > 0) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); ASSERT_GT(cert_chain_list[i].size(), 0); @@ -459,7 +467,7 @@ TEST_P(AttestKeyTest, EcAttestKeyChaining) { } EXPECT_TRUE(ChainSignaturesAreValid(cert_chain_list[i])); - EXPECT_GT(cert_chain_list[i].size(), i + (isRkpOnly() ? 0 : 1)); + EXPECT_GT(cert_chain_list[i].size(), i + (confirmedNotRkpOnly ? 1 : 0)); verify_subject_and_serial(cert_chain_list[i][0], serial_int, subject, false); } } @@ -530,7 +538,8 @@ TEST_P(AttestKeyTest, AlternateAttestKeyChaining) { .Authorization(TAG_NO_AUTH_REQUIRED) .SetDefaultValidity(); // In RKP-only systems, the first key cannot be attested due to lack of batch key - if (!isRkpOnly() || i > 0) { + bool confirmedNotRkpOnly = !isRkpOnly().value_or(true); + if (confirmedNotRkpOnly || i > 0) { auth_set_builder.AttestationChallenge("foo"); } if ((i & 0x1) == 1) { @@ -543,7 +552,7 @@ TEST_P(AttestKeyTest, AlternateAttestKeyChaining) { ASSERT_EQ(ErrorCode::OK, result); deleters.push_back(KeyBlobDeleter(keymint_, key_blob_list[i])); - if (!isRkpOnly() || i > 0) { + if (confirmedNotRkpOnly || i > 0) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); ASSERT_GT(cert_chain_list[i].size(), 0); @@ -566,7 +575,7 @@ TEST_P(AttestKeyTest, AlternateAttestKeyChaining) { } EXPECT_TRUE(ChainSignaturesAreValid(cert_chain_list[i])); - EXPECT_GT(cert_chain_list[i].size(), i + (isRkpOnly() ? 0 : 1)); + EXPECT_GT(cert_chain_list[i].size(), i + (confirmedNotRkpOnly ? 1 : 0)); verify_subject_and_serial(cert_chain_list[i][0], serial_int, subject, false); } } diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp index cef8120b88..2ba75a3135 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp @@ -250,7 +250,13 @@ bool KeyMintAidlTestBase::isSecondImeiIdAttestationRequired() { return AidlVersion() >= 3 && property_get_int32("ro.vendor.api_level", 0) > __ANDROID_API_T__; } -bool KeyMintAidlTestBase::isRkpOnly() { +std::optional KeyMintAidlTestBase::isRkpOnly() { + // GSI replaces the values for remote_prov_prop properties (since they’re system_internal_prop + // properties), so on GSI the properties are not reliable indicators of whether StrongBox/TEE is + // RKP-only or not. + if (is_gsi_image()) { + return std::nullopt; + } if (SecLevel() == SecurityLevel::STRONGBOX) { return property_get_bool("remote_provisioning.strongbox.rkp_only", false); } @@ -318,8 +324,11 @@ ErrorCode KeyMintAidlTestBase::GenerateKey(const AuthorizationSet& key_desc, vector attest_cert_chain; // If an attestation is requested, but the system is RKP-only, we need to supply an explicit // attestation key. Else the result is a key without an attestation. - if (isRkpOnly() && key_desc.Contains(TAG_ATTESTATION_CHALLENGE)) { - skipAttestKeyTestIfNeeded(); + // If the RKP-only value is undeterminable (i.e., when running on GSI), generate and use the + // attest key anyways. In the case that using an attest key is not supported + // (shouldSkipAttestKeyTest), assume the device has factory keys (so not RKP-only). + if (isRkpOnly().value_or(true) && key_desc.Contains(TAG_ATTESTATION_CHALLENGE) && + !shouldSkipAttestKeyTest()) { AuthorizationSet attest_key_desc = AuthorizationSetBuilder().EcdsaKey(EcCurve::P_256).AttestKey().SetDefaultValidity(); attest_key.emplace(); @@ -1677,14 +1686,6 @@ bool KeyMintAidlTestBase::shouldSkipAttestKeyTest(void) const { is_attest_key_feature_disabled()); } -// Skip a test that involves use of the ATTEST_KEY feature in specific configurations -// where ATTEST_KEY is not supported (for either StrongBox or TEE). -void KeyMintAidlTestBase::skipAttestKeyTestIfNeeded() const { - if (shouldSkipAttestKeyTest()) { - GTEST_SKIP() << "Test using ATTEST_KEY is not applicable on waivered device"; - } -} - void verify_serial(X509* cert, const uint64_t expected_serial) { BIGNUM_Ptr ser(BN_new()); EXPECT_TRUE(ASN1_INTEGER_to_BN(X509_get_serialNumber(cert), ser.get())); diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h index 1bf2d9d59e..0368bbac6c 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h @@ -104,7 +104,7 @@ class KeyMintAidlTestBase : public ::testing::TestWithParam { uint32_t boot_patch_level(); bool isDeviceIdAttestationRequired(); bool isSecondImeiIdAttestationRequired(); - bool isRkpOnly(); + std::optional isRkpOnly(); bool Curve25519Supported(); @@ -356,7 +356,6 @@ class KeyMintAidlTestBase : public ::testing::TestWithParam { bool is_strongbox_enabled(void) const; bool is_chipset_allowed_km4_strongbox(void) const; bool shouldSkipAttestKeyTest(void) const; - void skipAttestKeyTestIfNeeded() const; void assert_mgf_digests_present_or_not_in_key_characteristics( const vector& key_characteristics,