From 03d7a1a4f3982ce3da9ecfb23d7d65fa148f677d Mon Sep 17 00:00:00 2001 From: Eran Messeri Date: Tue, 6 Jul 2021 12:07:57 +0100 Subject: [PATCH] KeyMint: Fix device-unique attestation chain specification Fix the device-unique attestation chain specification: The chain should have two or three certificates. In case of two certificates, the device-unique key should be used for the self-signed root. In case of three certificates, the device-unique key should be certified by another key (ideally shared by all StrongBox instances from the same manufacturer, to ease validation). Adjust the device-unique attestation tests to accept two or three certificates in the chain. Additionally, the current StrongBox KeyMint implementation can not yet generate fully-valid chains (with matching subjects and issuers), so relax that check. Bug: 191361618 Test: m VtsAidlKeyMintTargetTest Change-Id: I6e6bca33ebb4af67cac8e41a39e9c305d0f1345f --- .../android/hardware/security/keymint/Tag.aidl | 17 ++++++++++++++--- .../functional/DeviceUniqueAttestationTest.cpp | 13 +++++++++---- .../aidl/vts/functional/KeyMintAidlTestBase.cpp | 5 +++-- .../aidl/vts/functional/KeyMintAidlTestBase.h | 3 ++- 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl b/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl index e8ff14f3c6..d11fcadf8b 100644 --- a/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl +++ b/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl @@ -827,11 +827,22 @@ enum Tag { /** * DEVICE_UNIQUE_ATTESTATION is an argument to IKeyMintDevice::attested key generation/import * operations. It indicates that attestation using a device-unique key is requested, rather - * than a batch key. When a device-unique key is used, the returned chain should contain two - * certificates: + * than a batch key. When a device-unique key is used, the returned chain should contain two or + * three certificates. + * + * In case the chain contains two certificates, they should be: * * The attestation certificate, containing the attestation extension, as described in - KeyCreationResult.aidl. + * KeyCreationResult.aidl. * * A self-signed root certificate, signed by the device-unique key. + * + * In case the chain contains three certificates, they should be: + * * The attestation certificate, containing the attestation extension, as described in + * KeyCreationResult.aidl, signed by the device-unique key. + * * An intermediate certificate, containing the public portion of the device-unique key. + * * A self-signed root certificate, signed by a dedicated key, certifying the + * intermediate. Ideally, the dedicated key would be the same for all StrongBox + * instances of the same manufacturer to ease validation. + * * No additional chained certificates are provided. Only SecurityLevel::STRONGBOX * IKeyMintDevices may support device-unique attestations. SecurityLevel::TRUSTED_ENVIRONMENT * IKeyMintDevices must return ErrorCode::INVALID_ARGUMENT if they receive diff --git a/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp b/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp index a3ed3ad4a0..d7abf0790c 100644 --- a/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp +++ b/security/keymint/aidl/vts/functional/DeviceUniqueAttestationTest.cpp @@ -40,11 +40,16 @@ class DeviceUniqueAttestationTest : public KeyMintAidlTestBase { AuthorizationSet crypto_params = SecLevelAuthorizations(key_characteristics); - // The device-unique attestation chain should contain exactly two certificates: + // The device-unique attestation chain should contain exactly three certificates: // * The leaf with the attestation extension. - // * A self-signed root, signed using the device-unique key. - ASSERT_EQ(cert_chain_.size(), 2); - EXPECT_TRUE(ChainSignaturesAreValid(cert_chain_)); + // * An intermediate, signing the leaf using the device-unique key. + // * A self-signed root, signed using some authority's key, certifying + // the device-unique key. + const size_t chain_length = cert_chain_.size(); + ASSERT_TRUE(chain_length == 2 || chain_length == 3); + // TODO(b/191361618): Once StrongBox implementations use a correctly-issued + // certificate chain, do not skip issuers matching. + 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, diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp index 5359b3b667..20324117b9 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp @@ -1493,7 +1493,8 @@ AuthorizationSet SwEnforcedAuthorizations(const vector& key_ return authList; } -AssertionResult ChainSignaturesAreValid(const vector& chain) { +AssertionResult ChainSignaturesAreValid(const vector& chain, + bool strict_issuer_check) { std::stringstream cert_data; for (size_t i = 0; i < chain.size(); ++i) { @@ -1520,7 +1521,7 @@ AssertionResult ChainSignaturesAreValid(const vector& chain) { string cert_issuer = x509NameToStr(X509_get_issuer_name(key_cert.get())); string signer_subj = x509NameToStr(X509_get_subject_name(signing_cert.get())); - if (cert_issuer != signer_subj) { + if (cert_issuer != signer_subj && strict_issuer_check) { return AssertionFailure() << "Cert " << i << " has wrong issuer.\n" << " Signer subject is " << signer_subj << " Issuer subject is " << cert_issuer << endl diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h index d592d3686b..ec3fcf6a3e 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.h @@ -349,7 +349,8 @@ void p256_pub_key(const vector& coseKeyData, EVP_PKEY_Ptr* signingKey); AuthorizationSet HwEnforcedAuthorizations(const vector& key_characteristics); AuthorizationSet SwEnforcedAuthorizations(const vector& key_characteristics); -::testing::AssertionResult ChainSignaturesAreValid(const vector& chain); +::testing::AssertionResult ChainSignaturesAreValid(const vector& chain, + bool strict_issuer_check = true); #define INSTANTIATE_KEYMINT_AIDL_TEST(name) \ INSTANTIATE_TEST_SUITE_P(PerInstance, name, \