From f1df387332592c0ddac166bc6ebc751b8ccf05eb Mon Sep 17 00:00:00 2001
From: David Drysdale <drysdale@google.com>
Date: Tue, 26 Nov 2024 09:40:38 +0000
Subject: [PATCH] Don't pass ATTEST_KEY for symmetric key generation

Bug: 379228013
Bug: 380375179
Test: VtsAidlKeyMintTargetTest
(cherry picked from https://android-review.googlesource.com/q/commit:6d825fb225f19353065e087dfe8688bc0e04f955)
Merged-In: I6b87a8997a3b9cf0f45f362ca91707546cc953d1
Change-Id: I6b87a8997a3b9cf0f45f362ca91707546cc953d1
---
 .../vts/functional/KeyMintAidlTestBase.cpp    | 36 ++++++++++---------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp
index c19ab11e97..3b6f92c4ac 100644
--- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp
+++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp
@@ -83,6 +83,16 @@ const uint32_t kInvalidPatchlevel = 99998877;
 // additional overhead, for the digest algorithmIdentifier required by PKCS#1.
 const size_t kPkcs1UndigestedSignaturePaddingOverhead = 11;
 
+// Determine whether the key description is for an asymmetric key.
+bool is_asymmetric(const AuthorizationSet& key_desc) {
+    auto algorithm = key_desc.GetTagValue(TAG_ALGORITHM);
+    if (algorithm && (algorithm.value() == Algorithm::RSA || algorithm.value() == Algorithm::EC)) {
+        return true;
+    } else {
+        return false;
+    }
+}
+
 size_t count_tag_invalid_entries(const std::vector<KeyParameter>& authorizations) {
     return std::count_if(authorizations.begin(), authorizations.end(),
                          [](const KeyParameter& e) -> bool { return e.tag == Tag::INVALID; });
@@ -324,11 +334,14 @@ ErrorCode KeyMintAidlTestBase::GenerateKey(const AuthorizationSet& key_desc,
     vector<Certificate> 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 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 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 the key being generated is a symmetric key (from test cases that check that the
+    //   attestation parameters are correctly ignored), don't try to use an `ATTEST_KEY`.
     if (isRkpOnly().value_or(true) && key_desc.Contains(TAG_ATTESTATION_CHALLENGE) &&
-        !shouldSkipAttestKeyTest()) {
+        !shouldSkipAttestKeyTest() && is_asymmetric(key_desc)) {
         AuthorizationSet attest_key_desc =
                 AuthorizationSetBuilder().EcdsaKey(EcCurve::P_256).AttestKey().SetDefaultValidity();
         attest_key.emplace();
@@ -369,10 +382,7 @@ ErrorCode KeyMintAidlTestBase::GenerateKey(const AuthorizationSet& key_desc,
         *key_characteristics = std::move(creationResult.keyCharacteristics);
         *cert_chain = std::move(creationResult.certificateChain);
 
-        auto algorithm = key_desc.GetTagValue(TAG_ALGORITHM);
-        EXPECT_TRUE(algorithm);
-        if (algorithm &&
-            (algorithm.value() == Algorithm::RSA || algorithm.value() == Algorithm::EC)) {
+        if (is_asymmetric(key_desc)) {
             EXPECT_GE(cert_chain->size(), 1);
             if (key_desc.Contains(TAG_ATTESTATION_CHALLENGE)) {
                 if (attest_key) {
@@ -413,10 +423,7 @@ ErrorCode KeyMintAidlTestBase::ImportKey(const AuthorizationSet& key_desc, KeyFo
         *key_characteristics = std::move(creationResult.keyCharacteristics);
         cert_chain_ = std::move(creationResult.certificateChain);
 
-        auto algorithm = key_desc.GetTagValue(TAG_ALGORITHM);
-        EXPECT_TRUE(algorithm);
-        if (algorithm &&
-            (algorithm.value() == Algorithm::RSA || algorithm.value() == Algorithm::EC)) {
+        if (is_asymmetric(key_desc)) {
             EXPECT_GE(cert_chain_.size(), 1);
             if (key_desc.Contains(TAG_ATTESTATION_CHALLENGE)) EXPECT_GT(cert_chain_.size(), 1);
         } else {
@@ -461,10 +468,7 @@ ErrorCode KeyMintAidlTestBase::ImportWrappedKey(string wrapped_key, string wrapp
         for (auto& entry : key_characteristics_) {
             allAuths.push_back(AuthorizationSet(entry.authorizations));
         }
-        auto algorithm = allAuths.GetTagValue(TAG_ALGORITHM);
-        EXPECT_TRUE(algorithm);
-        if (algorithm &&
-            (algorithm.value() == Algorithm::RSA || algorithm.value() == Algorithm::EC)) {
+        if (is_asymmetric(allAuths)) {
             EXPECT_GE(cert_chain_.size(), 1);
         } else {
             // For symmetric keys there should be no certificates.