From 7dff4fc9b7c4ab3124dcffa78ddd49eee97dd577 Mon Sep 17 00:00:00 2001 From: David Drysdale Date: Fri, 10 Dec 2021 10:10:52 +0000 Subject: [PATCH] KeyMint: new version number in attestation For the time being, allow the version number in the attestation record to be 100 even if the AIDL version is 2, so that implementations don't have to update both versions simultaneously. Bug: 194358913 Test: TreeHugger, VtsAidlKeyMintTargetTest Change-Id: I9aae69327a62014e286ce30ca2a4d91c4c280714 --- .../security/keymint/KeyCreationResult.aidl | 4 +- .../aidl/vts/functional/AttestKeyTest.cpp | 37 ++++++++++++------- .../DeviceUniqueAttestationTest.cpp | 5 ++- .../vts/functional/KeyMintAidlTestBase.cpp | 26 +++++++++++-- .../aidl/vts/functional/KeyMintAidlTestBase.h | 4 +- .../aidl/vts/functional/KeyMintTest.cpp | 31 +++++++++------- .../VtsRemotelyProvisionedComponentTests.cpp | 4 +- 7 files changed, 74 insertions(+), 37 deletions(-) diff --git a/security/keymint/aidl/android/hardware/security/keymint/KeyCreationResult.aidl b/security/keymint/aidl/android/hardware/security/keymint/KeyCreationResult.aidl index fd6bf65230..16bbc5c569 100644 --- a/security/keymint/aidl/android/hardware/security/keymint/KeyCreationResult.aidl +++ b/security/keymint/aidl/android/hardware/security/keymint/KeyCreationResult.aidl @@ -122,9 +122,9 @@ parcelable KeyCreationResult { * straightforward translation of the KeyMint tag/value parameter lists to ASN.1. * * KeyDescription ::= SEQUENCE { - * attestationVersion INTEGER, # Value 100 + * attestationVersion INTEGER, # Value 200 * attestationSecurityLevel SecurityLevel, # See below - * keyMintVersion INTEGER, # Value 100 + * keyMintVersion INTEGER, # Value 200 * keymintSecurityLevel SecurityLevel, # See below * attestationChallenge OCTET_STRING, # Tag::ATTESTATION_CHALLENGE from attestParams * uniqueId OCTET_STRING, # Empty unless key has Tag::INCLUDE_UNIQUE_ID diff --git a/security/keymint/aidl/vts/functional/AttestKeyTest.cpp b/security/keymint/aidl/vts/functional/AttestKeyTest.cpp index 73c382092e..e20b314ada 100644 --- a/security/keymint/aidl/vts/functional/AttestKeyTest.cpp +++ b/security/keymint/aidl/vts/functional/AttestKeyTest.cpp @@ -81,7 +81,8 @@ TEST_P(AttestKeyTest, AllRsaSizes) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), attested_key_cert_chain[0].encodedCertificate)); // Attestation by itself is not valid (last entry is not self-signed). @@ -113,7 +114,8 @@ TEST_P(AttestKeyTest, AllRsaSizes) { hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); - EXPECT_TRUE(verify_attestation_record("foo2", "bar2", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo2", "bar2", sw_enforced, + hw_enforced, SecLevel(), attested_key_cert_chain[0].encodedCertificate)); // Attestation by itself is not valid (last entry is not self-signed). @@ -154,12 +156,13 @@ TEST_P(AttestKeyTest, AllRsaSizes) { sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); // The client-specified CREATION_DATETIME should be in sw_enforced. - // Its presence will also trigger verify_attestation_record() to check that it - // is in the attestation extension with a matching value. + // Its presence will also trigger verify_attestation_record() to check that + // it is in the attestation extension with a matching value. EXPECT_TRUE(sw_enforced.Contains(TAG_CREATION_DATETIME, timestamp)) << "expected CREATION_TIMESTAMP in sw_enforced:" << sw_enforced << " not in hw_enforced:" << hw_enforced; - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), attested_key_cert_chain[0].encodedCertificate)); // Attestation by itself is not valid (last entry is not self-signed). @@ -217,7 +220,7 @@ TEST_P(AttestKeyTest, RsaAttestedAttestKeys) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attest_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attest_key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, app_id, // + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, // sw_enforced, hw_enforced, SecLevel(), attest_key_cert_chain[0].encodedCertificate)); @@ -252,7 +255,8 @@ TEST_P(AttestKeyTest, RsaAttestedAttestKeys) { AuthorizationSet hw_enforced2 = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced2 = SwEnforcedAuthorizations(attested_key_characteristics); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced2, hw_enforced2, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced2, hw_enforced2, + SecLevel(), attested_key_cert_chain[0].encodedCertificate)); // Attestation by itself is not valid (last entry is not self-signed). @@ -313,7 +317,8 @@ TEST_P(AttestKeyTest, RsaAttestKeyChaining) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); ASSERT_GT(cert_chain_list[i].size(), 0); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), cert_chain_list[i][0].encodedCertificate)); if (i > 0) { @@ -385,7 +390,8 @@ TEST_P(AttestKeyTest, EcAttestKeyChaining) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); ASSERT_GT(cert_chain_list[i].size(), 0); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), cert_chain_list[i][0].encodedCertificate)); if (i > 0) { @@ -474,7 +480,8 @@ TEST_P(AttestKeyTest, AlternateAttestKeyChaining) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); ASSERT_GT(cert_chain_list[i].size(), 0); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), cert_chain_list[i][0].encodedCertificate)); if (i > 0) { @@ -588,7 +595,8 @@ TEST_P(AttestKeyTest, AllEcCurves) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), attested_key_cert_chain[0].encodedCertificate)); // Attestation by itself is not valid (last entry is not self-signed). @@ -619,7 +627,8 @@ TEST_P(AttestKeyTest, AllEcCurves) { hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "foo", "bar", sw_enforced, hw_enforced, + SecLevel(), attested_key_cert_chain[0].encodedCertificate)); // Attestation by itself is not valid (last entry is not self-signed). @@ -724,8 +733,8 @@ TEST_P(AttestKeyTest, EcdsaAttestationID) { // attestation extension should contain them, so make sure the extra tag is added. hw_enforced.push_back(tag); - EXPECT_TRUE(verify_attestation_record("challenge", "foo", sw_enforced, hw_enforced, - SecLevel(), + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "challenge", "foo", sw_enforced, + hw_enforced, SecLevel(), attested_key_cert_chain[0].encodedCertificate)); } CheckedDeleteKey(&attest_key.keyBlob); diff --git a/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp b/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp index 3cbffbfb63..d4bbd693b8 100644 --- a/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp +++ b/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp @@ -52,8 +52,9 @@ class DeviceUniqueAttestationTest : public KeyMintAidlTestBase { EXPECT_TRUE(ChainSignaturesAreValid(cert_chain_, /* strict_issuer_check= */ false)); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record("challenge", "foo", sw_enforced, hw_enforced, - SecLevel(), cert_chain_[0].encodedCertificate)); + EXPECT_TRUE(verify_attestation_record(AidlVersion(), "challenge", "foo", sw_enforced, + hw_enforced, SecLevel(), + cert_chain_[0].encodedCertificate)); } }; diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp index 6140df1350..3695f1e094 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp @@ -127,6 +127,16 @@ ASN1_OCTET_STRING* get_attestation_record(X509* certificate) { return attest_rec; } +void check_attestation_version(uint32_t attestation_version, int32_t aidl_version) { + // Version numbers in attestation extensions should be a multiple of 100. + EXPECT_EQ(attestation_version % 100, 0); + + // The multiplier should never be higher than the AIDL version, but can be less + // (for example, if the implementation is from an earlier version but the HAL service + // uses the default libraries and so reports the current AIDL version). + EXPECT_TRUE((attestation_version / 100) <= aidl_version); +} + bool avb_verification_enabled() { char value[PROPERTY_VALUE_MAX]; return property_get("ro.boot.vbmeta.device_state", value, "") != 0; @@ -223,6 +233,15 @@ void KeyMintAidlTestBase::InitializeKeyMint(std::shared_ptr keyM vendor_patch_level_ = getVendorPatchlevel(); } +int32_t KeyMintAidlTestBase::AidlVersion() { + int32_t version = 0; + auto status = keymint_->getInterfaceVersion(&version); + if (!status.isOk()) { + ADD_FAILURE() << "Failed to determine interface version"; + } + return version; +} + void KeyMintAidlTestBase::SetUp() { if (AServiceManager_isDeclared(GetParam().c_str())) { ::ndk::SpAIBinder binder(AServiceManager_waitForService(GetParam().c_str())); @@ -1304,7 +1323,8 @@ void verify_subject_and_serial(const Certificate& certificate, // verify_subject(cert.get(), subject, self_signed); } -bool verify_attestation_record(const string& challenge, // +bool verify_attestation_record(int32_t aidl_version, // + const string& challenge, // const string& app_id, // AuthorizationSet expected_sw_enforced, // AuthorizationSet expected_hw_enforced, // @@ -1342,7 +1362,7 @@ bool verify_attestation_record(const string& challenge, // EXPECT_EQ(ErrorCode::OK, error); if (error != ErrorCode::OK) return false; - EXPECT_EQ(att_attestation_version, 100U); + check_attestation_version(att_attestation_version, aidl_version); vector appId(app_id.begin(), app_id.end()); // check challenge and app id only if we expects a non-fake certificate @@ -1353,7 +1373,7 @@ bool verify_attestation_record(const string& challenge, // expected_sw_enforced.push_back(TAG_ATTESTATION_APPLICATION_ID, appId); } - EXPECT_EQ(att_keymint_version, 100U); + check_attestation_version(att_keymint_version, aidl_version); EXPECT_EQ(security_level, att_keymint_security_level); EXPECT_EQ(security_level, att_attestation_security_level); diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h index 7b3b9d4b4b..61f9d4d59a 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h @@ -73,6 +73,7 @@ class KeyMintAidlTestBase : public ::testing::TestWithParam { void InitializeKeyMint(std::shared_ptr keyMint); IKeyMintDevice& keyMint() { return *keymint_; } + int32_t AidlVersion(); uint32_t os_version() { return os_version_; } uint32_t os_patch_level() { return os_patch_level_; } uint32_t vendor_patch_level() { return vendor_patch_level_; } @@ -333,7 +334,8 @@ void verify_subject_and_serial(const Certificate& certificate, // const uint64_t expected_serial, // const string& subject, bool self_signed); -bool verify_attestation_record(const string& challenge, // +bool verify_attestation_record(int aidl_version, // + const string& challenge, // const string& app_id, // AuthorizationSet expected_sw_enforced, // AuthorizationSet expected_hw_enforced, // diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp index 161531d8f8..49fe5fbea5 100644 --- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp @@ -942,7 +942,7 @@ TEST_P(NewKeyGenerationTest, RsaWithAttestation) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, app_id, // + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, // sw_enforced, hw_enforced, SecLevel(), cert_chain_[0].encodedCertificate)); @@ -1093,7 +1093,7 @@ TEST_P(NewKeyGenerationTest, RsaEncryptionWithAttestation) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, app_id, // + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, // sw_enforced, hw_enforced, SecLevel(), cert_chain_[0].encodedCertificate)); @@ -1315,7 +1315,7 @@ TEST_P(NewKeyGenerationTest, LimitedUsageRsaWithAttestation) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, app_id, // + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, // sw_enforced, hw_enforced, SecLevel(), cert_chain_[0].encodedCertificate)); @@ -1444,7 +1444,7 @@ TEST_P(NewKeyGenerationTest, EcdsaAttestation) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, app_id, // + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, // sw_enforced, hw_enforced, SecLevel(), cert_chain_[0].encodedCertificate)); @@ -1523,8 +1523,9 @@ TEST_P(NewKeyGenerationTest, EcdsaAttestationTags) { // Verifying the attestation record will check for the specific tag because // it's included in the authorizations. - EXPECT_TRUE(verify_attestation_record(challenge, app_id, sw_enforced, hw_enforced, - SecLevel(), cert_chain_[0].encodedCertificate)); + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, sw_enforced, + hw_enforced, SecLevel(), + cert_chain_[0].encodedCertificate)); CheckedDeleteKey(&key_blob); } @@ -1621,8 +1622,9 @@ TEST_P(NewKeyGenerationTest, EcdsaAttestationIdTags) { // Verifying the attestation record will check for the specific tag because // it's included in the authorizations. - EXPECT_TRUE(verify_attestation_record(challenge, app_id, sw_enforced, hw_enforced, - SecLevel(), cert_chain_[0].encodedCertificate)); + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, sw_enforced, + hw_enforced, SecLevel(), + cert_chain_[0].encodedCertificate)); CheckedDeleteKey(&key_blob); } @@ -1668,9 +1670,9 @@ TEST_P(NewKeyGenerationTest, EcdsaAttestationUniqueId) { AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics_); // Check that the unique ID field in the extension is non-empty. - EXPECT_TRUE(verify_attestation_record(challenge, app_id, sw_enforced, hw_enforced, - SecLevel(), cert_chain_[0].encodedCertificate, - unique_id)); + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, sw_enforced, + hw_enforced, SecLevel(), + cert_chain_[0].encodedCertificate, unique_id)); EXPECT_GT(unique_id->size(), 0); CheckedDeleteKey(); }; @@ -1765,8 +1767,9 @@ TEST_P(NewKeyGenerationTest, EcdsaAttestationTagNoApplicationId) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, attest_app_id, sw_enforced, hw_enforced, - SecLevel(), cert_chain_[0].encodedCertificate)); + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, attest_app_id, sw_enforced, + hw_enforced, SecLevel(), + cert_chain_[0].encodedCertificate)); // Check that the app id is not in the cert. string app_id = "clientid"; @@ -1919,7 +1922,7 @@ TEST_P(NewKeyGenerationTest, AttestationApplicationIDLengthProperlyEncoded) { AuthorizationSet hw_enforced = HwEnforcedAuthorizations(key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(key_characteristics); - EXPECT_TRUE(verify_attestation_record(challenge, app_id, // + EXPECT_TRUE(verify_attestation_record(AidlVersion(), challenge, app_id, // sw_enforced, hw_enforced, SecLevel(), cert_chain_[0].encodedCertificate)); diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index 76fb79b618..c9d506f788 100644 --- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -236,9 +236,11 @@ TEST_P(GenerateKeyTests, generateAndUseEcdsaP256Key_prodMode) { vector attested_key_cert_chain = std::move(creationResult.certificateChain); EXPECT_EQ(attested_key_cert_chain.size(), 1); + int32_t aidl_version = 0; + ASSERT_TRUE(keyMint->getInterfaceVersion(&aidl_version).isOk()); AuthorizationSet hw_enforced = HwEnforcedAuthorizations(attested_key_characteristics); AuthorizationSet sw_enforced = SwEnforcedAuthorizations(attested_key_characteristics); - EXPECT_TRUE(verify_attestation_record("foo", "bar", sw_enforced, hw_enforced, + EXPECT_TRUE(verify_attestation_record(aidl_version, "foo", "bar", sw_enforced, hw_enforced, info.securityLevel, attested_key_cert_chain[0].encodedCertificate));