From e60248c8e7c064833e2a88db2a6f84b9471d44c9 Mon Sep 17 00:00:00 2001
From: David Drysdale <drysdale@google.com>
Date: Mon, 4 Oct 2021 12:54:13 +0100
Subject: [PATCH] KeyMint VTS: ATTEST_KEY has no other purpose

The KeyMint spec has always required that keys with the ATTEST_KEY
purpose "must not have any other purpose".

Add explicit tests for combined-purpose keys to be rejected.

Also expand the spec text to require a specific error code, and to
explain the rationale for single-purpose ATTEST_KEY keys.

Bug: 197096139
Test: VtsAidlKeyMintTargetTest
Change-Id: I2a2014f0ddc497128ba51bb3f43671f759789912
---
 .../hardware/security/keymint/KeyPurpose.aidl |  6 +++-
 .../aidl/vts/functional/AttestKeyTest.cpp     | 36 +++++++++++++++++++
 .../aidl/vts/functional/KeyMintTest.cpp       | 36 +++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/security/keymint/aidl/android/hardware/security/keymint/KeyPurpose.aidl b/security/keymint/aidl/android/hardware/security/keymint/KeyPurpose.aidl
index e141e5544d..fd103ef390 100644
--- a/security/keymint/aidl/android/hardware/security/keymint/KeyPurpose.aidl
+++ b/security/keymint/aidl/android/hardware/security/keymint/KeyPurpose.aidl
@@ -44,6 +44,10 @@ enum KeyPurpose {
     AGREE_KEY = 6,
 
     /* Usable as an attestation signing key.  Keys with this purpose must not have any other
-     * purpose. */
+     * purpose; if they do, key generation/import must be rejected with
+     * ErrorCode::INCOMPATIBLE_PURPOSE. (Rationale: If key also included KeyPurpose::SIGN, then
+     * it could be used to sign arbitrary data, including any tbsCertificate, and so an
+     * attestation produced by the key would have no security properties.)
+     */
     ATTEST_KEY = 7,
 }
diff --git a/security/keymint/aidl/vts/functional/AttestKeyTest.cpp b/security/keymint/aidl/vts/functional/AttestKeyTest.cpp
index 64550eff2d..a74a0b69a0 100644
--- a/security/keymint/aidl/vts/functional/AttestKeyTest.cpp
+++ b/security/keymint/aidl/vts/functional/AttestKeyTest.cpp
@@ -174,6 +174,24 @@ TEST_P(AttestKeyTest, AllRsaSizes) {
     }
 }
 
+/*
+ * AttestKeyTest.RsaAttestKeyMultiPurposeFail
+ *
+ * This test attempts to create an RSA attestation key that also allows signing.
+ */
+TEST_P(AttestKeyTest, RsaAttestKeyMultiPurposeFail) {
+    vector<uint8_t> attest_key_blob;
+    vector<KeyCharacteristics> attest_key_characteristics;
+    vector<Certificate> attest_key_cert_chain;
+    ASSERT_EQ(ErrorCode::INCOMPATIBLE_PURPOSE,
+              GenerateKey(AuthorizationSetBuilder()
+                                  .RsaSigningKey(2048, 65537)
+                                  .AttestKey()
+                                  .SetDefaultValidity(),
+                          {} /* attestation signing key */, &attest_key_blob,
+                          &attest_key_characteristics, &attest_key_cert_chain));
+}
+
 /*
  * AttestKeyTest.RsaAttestedAttestKeys
  *
@@ -411,6 +429,24 @@ TEST_P(AttestKeyTest, EcAttestKeyChaining) {
     }
 }
 
+/*
+ * AttestKeyTest.EcAttestKeyMultiPurposeFail
+ *
+ * This test attempts to create an EC attestation key that also allows signing.
+ */
+TEST_P(AttestKeyTest, EcAttestKeyMultiPurposeFail) {
+    vector<uint8_t> attest_key_blob;
+    vector<KeyCharacteristics> attest_key_characteristics;
+    vector<Certificate> attest_key_cert_chain;
+    ASSERT_EQ(ErrorCode::INCOMPATIBLE_PURPOSE,
+              GenerateKey(AuthorizationSetBuilder()
+                                  .EcdsaSigningKey(EcCurve::P_256)
+                                  .AttestKey()
+                                  .SetDefaultValidity(),
+                          {} /* attestation signing key */, &attest_key_blob,
+                          &attest_key_characteristics, &attest_key_cert_chain));
+}
+
 /*
  * AttestKeyTest.AlternateAttestKeyChaining
  *
diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp
index 670043d0dd..186873880c 100644
--- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp
+++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp
@@ -3283,6 +3283,26 @@ TEST_P(ImportKeyTest, RsaPublicExponentMismatch) {
                         KeyFormat::PKCS8, rsa_key));
 }
 
+/*
+ * ImportKeyTest.RsaAttestMultiPurposeFail
+ *
+ * Verifies that importing an RSA key pair with purpose ATTEST_KEY+SIGN fails.
+ */
+TEST_P(ImportKeyTest, RsaAttestMultiPurposeFail) {
+    uint32_t key_size = 2048;
+    string key = rsa_2048_key;
+
+    ASSERT_EQ(ErrorCode::INCOMPATIBLE_PURPOSE,
+              ImportKey(AuthorizationSetBuilder()
+                                .Authorization(TAG_NO_AUTH_REQUIRED)
+                                .RsaSigningKey(key_size, 65537)
+                                .AttestKey()
+                                .Digest(Digest::SHA_2_256)
+                                .Padding(PaddingMode::RSA_PSS)
+                                .SetDefaultValidity(),
+                        KeyFormat::PKCS8, key));
+}
+
 /*
  * ImportKeyTest.EcdsaSuccess
  *
@@ -3401,6 +3421,22 @@ TEST_P(ImportKeyTest, EcdsaCurveMismatch) {
                         KeyFormat::PKCS8, ec_256_key));
 }
 
+/*
+ * ImportKeyTest.EcdsaAttestMultiPurposeFail
+ *
+ * Verifies that importing and using an ECDSA P-256 key pair with purpose ATTEST_KEY+SIGN fails.
+ */
+TEST_P(ImportKeyTest, EcdsaAttestMultiPurposeFail) {
+    ASSERT_EQ(ErrorCode::INCOMPATIBLE_PURPOSE,
+              ImportKey(AuthorizationSetBuilder()
+                                .Authorization(TAG_NO_AUTH_REQUIRED)
+                                .EcdsaSigningKey(EcCurve::P_256)
+                                .AttestKey()
+                                .Digest(Digest::SHA_2_256)
+                                .SetDefaultValidity(),
+                        KeyFormat::PKCS8, ec_256_key));
+}
+
 /*
  * ImportKeyTest.AesSuccess
  *