Correct handling of ATTEST_KEYs.

server/build.gradle

@@ -27,7 +27,7 @@ repositories {
 dependencies {
     // Bouncy Castle Cryptography APIs used for certificate verification
     compile 'org.bouncycastle:bcpkix-jdk15on:1.61'
-    compile 'com.google.guava:guava:27.0.1-android'
+    compile 'com.google.guava:guava:31.1-jre'
     compile 'com.google.errorprone:error_prone_annotations:2.3.1'
     // Gson used for decoding certificate status list
     compile 'com.google.code.gson:gson:2.8.5'

server/src/main/java/com/android/example/KeyAttestationExample.java

@@ -26,7 +26,7 @@
 import com.google.android.attestation.ParsedAttestationRecord;
 import com.google.android.attestation.RootOfTrust;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableList.Builder;
+
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.nio.file.Files;

server/src/main/java/com/google/android/attestation/Constants.java

@@ -102,6 +102,8 @@ public class Constants {
   static final int KM_VERIFIED_BOOT_STATE_SELF_SIGNED = 1;
   static final int KM_VERIFIED_BOOT_STATE_UNVERIFIED = 2;
   static final int KM_VERIFIED_BOOT_STATE_FAILED = 3;
+  static final int KM_KEY_PURPOSE_SIGN = 2;
+  static final int KM_KEY_PURPOSE_ATTEST_KEY = 7;
   // Unsigned max value of 32-bit integer, 2^32 - 1
   static final long UINT32_MAX = (((long) Integer.MAX_VALUE) << 1) + 1;
 }

server/src/main/java/com/google/android/attestation/ParsedAttestationRecord.java

@@ -21,6 +21,7 @@
 import static com.google.android.attestation.Constants.KEYMASTER_SECURITY_LEVEL_INDEX;
 import static com.google.android.attestation.Constants.KEYMASTER_VERSION_INDEX;
 import static com.google.android.attestation.Constants.KEY_DESCRIPTION_OID;
+import static com.google.android.attestation.Constants.KM_KEY_PURPOSE_ATTEST_KEY;
 import static com.google.android.attestation.Constants.KM_SECURITY_LEVEL_SOFTWARE;
 import static com.google.android.attestation.Constants.KM_SECURITY_LEVEL_STRONG_BOX;
 import static com.google.android.attestation.Constants.KM_SECURITY_LEVEL_TRUSTED_ENVIRONMENT;
@@ -30,7 +31,10 @@
 
 import java.io.IOException;
 import java.security.cert.X509Certificate;
+import java.util.Collections;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 import org.bouncycastle.asn1.ASN1Encodable;
 import org.bouncycastle.asn1.ASN1Enumerated;
 import org.bouncycastle.asn1.ASN1InputStream;
@@ -100,20 +104,51 @@ private ParsedAttestationRecord(
   public static ParsedAttestationRecord createParsedAttestationRecord(List<X509Certificate> certs)
       throws IOException {
 
-    // Parse the attestation record that is closest to the root. This prevents an adversary from
-    // attesting an attestation record of their choice with an otherwise trusted chain using the
-    // following attack:
-    // 1) having the TEE attest a key under the adversary's control,
-    // 2) using that key to sign a new leaf certificate with an attestation extension that has their chosen attestation record, then
-    // 3) appending that certificate to the original certificate chain.
-    for (int i = certs.size() - 1; i >= 0; i--) {
-      byte[] attestationExtensionBytes = certs.get(i).getExtensionValue(KEY_DESCRIPTION_OID);
-      if (attestationExtensionBytes != null && attestationExtensionBytes.length != 0) {
-        return new ParsedAttestationRecord(extractAttestationSequence(attestationExtensionBytes));
+    if (certs.isEmpty()) {
+      throw new IllegalArgumentException("Empty certificate list, couldn't get attestation data.");
+    }
+
+    // Leaf certificate should contain the attestation record we're looking for.
+    final ParsedAttestationRecord retval = extractParsedAttestationRecord(certs.get(0));
+    if (retval == null) {
+      throw new IllegalArgumentException("Leaf certificate doesn't contain attestation data.");
+    }
+
+    // Make sure the rest of the chain is correct.  After the leaf we can have zero or more
+    // attestations of ATTEST_KEYs, then the remaining certificates must not be attestations.  So if
+    // we find any attestations after the first non-attestation, or if we find any attestations that
+    // aren't for ATTEST_KEYs, throw.
+    boolean foundNonAttestationCert = false;
+    for (X509Certificate cert : certs.stream().skip(1).collect(Collectors.toList())) {
+      final ParsedAttestationRecord record = extractParsedAttestationRecord(cert);
+      if (record == null) {
+        foundNonAttestationCert = true;
+      } else {
+        if (foundNonAttestationCert) {
+          throw new IllegalArgumentException(
+              "Found an attestation certificate after non-attestation cert(s). This should be impossible.");
+        }
+        if (!isAttestKeyAttestation(record)) {
+          throw new IllegalArgumentException("Found non-ATTEST_KEY attestation after leaf.");
+        }
       }
     }
 
-    throw new IllegalArgumentException("Couldn't find the keystore attestation extension data.");
+    return retval;
+  }
+
+  private static ParsedAttestationRecord extractParsedAttestationRecord(X509Certificate cert)
+      throws IOException {
+    byte[] attestationExtensionBytes = cert.getExtensionValue(KEY_DESCRIPTION_OID);
+    if (attestationExtensionBytes == null) {
+      return null;
+    }
+    return new ParsedAttestationRecord(extractAttestationSequence(attestationExtensionBytes));
+  }
+
+  private static boolean isAttestKeyAttestation(ParsedAttestationRecord record) {
+    Set<Integer> purposes = record.teeEnforced.purpose.orElse(Collections.emptySet());
+    return purposes.size() == 1 && purposes.contains(KM_KEY_PURPOSE_ATTEST_KEY);
   }
 
   public static ParsedAttestationRecord create(ASN1Sequence extensionData) {

server/src/test/java/com/google/android/attestation/ParsedAttestationRecordTest.java

@@ -15,109 +15,68 @@
 
 package com.google.android.attestation;
 
+import static com.google.android.attestation.Constants.KM_KEY_PURPOSE_SIGN;
 import static com.google.common.truth.Truth.assertThat;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.android.attestation.AuthorizationList.UserAuthType;
 import com.google.android.attestation.ParsedAttestationRecord.SecurityLevel;
-import java.io.ByteArrayInputStream;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateFactory;
 import java.security.cert.X509Certificate;
-import java.util.Arrays;
-
-import com.google.common.collect.ImmutableSet;
+import java.util.Collections;
+import java.util.Set;
 import org.bouncycastle.asn1.ASN1Sequence;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /** Test for {@link ParsedAttestationRecord}. */
 @RunWith(JUnit4.class)
 public class ParsedAttestationRecordTest {
-
-  // Certificate generated by TestDPC with RSA Algorithm and StrongBox Security Level
-  private static final String CERT =
-      "-----BEGIN CERTIFICATE-----\n"
-          + "MIIGCDCCBHCgAwIBAgIBATANBgkqhkiG9w0BAQsFADApMRkwFwYDVQQFExAyZGM1OGIyZDFhMjQx"
-          + "MzI2MQwwCgYDVQQMDANURUUwIBcNNzAwMTAxMDAwMDAwWhgPMjEwNjAyMDcwNjI4MTVaMB8xHTAb"
-          + "BgNVBAMMFEFuZHJvaWQgS2V5c3RvcmUgS2V5MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC"
-          + "AQEApNVcnyN40MANMbbo2nMGNq2NNysDSjfLm0W3i6wPKf0ffCYkhWM4dCmQKKf50uAZTBeTit4c"
-          + "NwXeZn3qellMlOsIN3Qc384rfN/8cikrRvUAgibz0Jy7STykjwa7x6tKwqITxbO8HqAhKo8/BQXU"
-          + "xzrOdIg5ciy+UM7Vgh7a7ogen0KL2iGgrsalb1ti7Vlzb6vIJ4WzIC3TGD2sCkoPahghwqFDZZCo"
-          + "/FzaLoNY0jAUX2mL+kf8aUaoxz7xA9FTvgara+1pLBR1s4c8xPS2HdZipcVXWfey0wujv1VAKs4+"
-          + "tXjKlHkYBHBBceEjxUtEmrapSQEdpHPv7Xh9Uanq4QIDAQABo4ICwTCCAr0wDgYDVR0PAQH/BAQD"
-          + "AgeAMIICqQYKKwYBBAHWeQIBEQSCApkwggKVAgEDCgEBAgEECgEBBANhYmMEADCCAc2/hT0IAgYB"
-          + "ZOYGEYe/hUWCAbsEggG3MIIBszGCAYswDAQHYW5kcm9pZAIBHTAZBBRjb20uYW5kcm9pZC5rZXlj"
-          + "aGFpbgIBHTAZBBRjb20uYW5kcm9pZC5zZXR0aW5ncwIBHTAZBBRjb20ucXRpLmRpYWdzZXJ2aWNl"
-          + "cwIBHTAaBBVjb20uYW5kcm9pZC5keW5zeXN0ZW0CAR0wHQQYY29tLmFuZHJvaWQuaW5wdXRkZXZp"
-          + "Y2VzAgEdMB8EGmNvbS5hbmRyb2lkLmxvY2FsdHJhbnNwb3J0AgEdMB8EGmNvbS5hbmRyb2lkLmxv"
-          + "Y2F0aW9uLmZ1c2VkAgEdMB8EGmNvbS5hbmRyb2lkLnNlcnZlci50ZWxlY29tAgEdMCAEG2NvbS5h"
-          + "bmRyb2lkLndhbGxwYXBlcmJhY2t1cAIBHTAhBBxjb20uZ29vZ2xlLlNTUmVzdGFydERldGVjdG9y"
-          + "AgEdMCIEHWNvbS5nb29nbGUuYW5kcm9pZC5oaWRkZW5tZW51AgEBMCMEHmNvbS5hbmRyb2lkLnBy"
-          + "b3ZpZGVycy5zZXR0aW5ncwIBHTEiBCAwGqPLCBE0UBxF8UIqvGbCQiT9Xe1f3I8X5pcXb9hmqjCB"
-          + "rqEIMQYCAQICAQOiAwIBAaMEAgIIAKUFMQMCAQSmCDEGAgEDAgEFv4FIBQIDAQABv4N3AgUAv4U+"
-          + "AwIBAL+FQEwwSgQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAQAKAQIEIHKNsSdP"
-          + "HxzxVx3kOAsEilVKxKOA529TVQg1KQhKk3gBv4VBAwIBAL+FQgUCAwMUs7+FTgUCAwMUs7+FTwUC"
-          + "AwMUszANBgkqhkiG9w0BAQsFAAOCAYEAJMIuzdNUdfrE6sIdmsnMn/scSG2odbphj8FkX9JGdF2S"
-          + "OT599HuDY9qhvkru2Dza4sLKK3f4ViBhuR9lpfeprKvstxbtBO7jkLYfVn0ZRzHRHVEyiW5IVKh+"
-          + "qOXVJ9S1lMShOTlsaYJytLKIlcrRAZBEXZiNbzTuVh1CH6X9Ni1dog14snm+lcOeORdL9fht2CHa"
-          + "u/caRnpWiZbjoAoJp0O89uBrRkXPpln51+3jPY6AFny30grNAvKguauDcPPhNV1yR+ylSsQi2gm3"
-          + "Rs4pgtlxFLMfZLgT0cbkl+9zk/QUqlpBP8ftUBsOI0ARr8xhFN3cvq9kXGLtJ9hEP9PRaflAFREk"
-          + "DK3IBIbVcAFZBFoAQOdE9zy0+F5bQrznPGaZg4Dzhcx33qMDUTgHtWoy+k3ePGQMEtmoTTLgQywW"
-          + "OIkXEoFqqGi9GKJXUT1KYi5NsigaYqu7FoN4Qsvs61pMUEfZSPP2AFwkA8uNFbmb9uxcxaGHCA8i"
-          + "3i9VM6yOLIrP\n"
-          + "-----END CERTIFICATE-----";
-  private static final String CERT2 =
-      "-----BEGIN CERTIFICATE-----\n"
-          + "MIIEFDCCAnygAwIBAgIVAKZFQPAXr5VWrosuqx4C8tai2XbHMA0GCSqGSIb3DQEB\n"
-          + "CwUAMBgxFjAUBgNVBAMMDVVua25vd25Jc3N1ZXIwHhcNMjMwMjE1MTU0MzIwWhcN\n"
-          + "MjMwMjE1MTU0ODIwWjAdMRswGQYDVQQDDBJBbmRyb2lkQXR0ZXN0ZWRLZXkwggGi\n"
-          + "MA0GCSqGSIb3DQEBAQUAA4IBjwAwggGKAoIBgQDJAVfP/7F1bUbDqxMnOVXpSjt5\n"
-          + "NJwYemBJkN7l7TTbAhTfMW91006Si/snd79Y6bsJklVoiEN9LGL7tQrJEf5lSSLX\n"
-          + "ZeppjsbLqKnogFHhDJy2vaSiypV2wZdX+kO0qqIKjRvgSqHuTz3gemI1rWilrG3C\n"
-          + "vd3iHGlkw/4X5PpHQKz99/20p85HP6f/jydMHewFDRQCbkbo2pJ5WrJsyPe9me3o\n"
-          + "QE0O3lgij7jJ/UBHyb9iH0w13yi+1yZ/jgyojL4QNUeWZnxW656zfHCB8weePD+l\n"
-          + "tX4AAztZTziJQwk3zVClw4xIPTeztQV6ddRQgjSjGvWanpXqhJx8mq11gWaVJoCl\n"
-          + "q/I0KOguVsKq42M25uhF7/iAQjC+6lOUUfi2+aPwyTUfGHc5Bw/rTSw2LzvZDnUW\n"
-          + "8/yw4OUTyDravVcQLeoBES4+O5cVL0yTKDY0THG+ymgsFNgFS7PXUnAbXczYzvg8\n"
-          + "ldXKOXxnF5nWgg55n2iSQ6mqtHDEUsjcxjmuFcMCAwEAAaNQME4wTAYKKwYBBAHW\n"
-          + "eQIBEQQ+MDwCAQEKAQECAQIKAQEEEkEgcmFuZG9tIGNoYWxsZW5nZQQAMAAwFr+F\n"
-          + "SQgEBlNFUklBTL+FSgYEBElNRUkwDQYJKoZIhvcNAQELBQADggGBAHSms4IBjkc8\n"
-          + "1ZLHu5l70Ih2RrNU4XAc2E/oJX8OsBte9ZRwDT3TdcfLeg0rSneS+aB4xN1BGfmL\n"
-          + "DPZ1epRzMY4RagVhzBEauHpTaM2imRT9RN5TxbFvuMC4ELICYr5qHfqeALIlMET3\n"
-          + "TbCAo3njpNh5ids6qdlmpZRoYBQNMKfWJn8SUtCmVMk87FA7RZZCqCiRk+PBnciT\n"
-          + "O3LLbwT4aBlMinQ84gBfVXRqOvGAeGOgojDqGyK3tDMjIS7itpGb23vGogxHiHjA\n"
-          + "i8hiQhsHA+C89duCdeGyWZGmxwln7QRsosFI7G4ZOufXPLZt/DauNAC2Mb2OPcDw\n"
-          + "4tSKQvzQiL9UG4X3Cck0JnATxjT5sLttshJl98V6jQHcWSnjg8+oa3B8WgcePX8E\n"
-          + "QgcLhYaEGo9WDYJQvHfuUE5AquTxdTRbeiDbV7W+FAOQ5zi/wiGit86gF26120OQ\n"
-          + "KzQHP94/ORuAT/lkv3Fp3HytF4n3scur1nI0WqrfKpbUuPkmndCIbg==\n"
-          + "-----END CERTIFICATE-----\n";
-
-  private static final int EXPECTED_ATTESTATION_VERSION = 3;
+  public static final int EXPECTED_KEY_PURPOSE = KM_KEY_PURPOSE_SIGN;
+  // The cert chains at the following paths were all generated on a Pixel 7 Pro, with
+  // remotely-provisioned certificates.
+  private static final String TEST_NORMAL_CERT_CHAIN_PATH =
+      "src/test/resources/normal_cert_chain.pem";
+  private static final String TEST_ATTEST_KEY_CERT_CHAIN_PATH =
+      "src/test/resources/cert_chain_with_attest_keys.pem";
+  private static final String TEST_FAKE_KEY_CERT_CHAIN_PATH =
+      "src/test/resources/fake_cert_chain_with_attest_keys.pem";
+  // The values specified in these expectation constants must match what's in the cert chains
+  // mentioned above.
+  private static final int EXPECTED_ATTESTATION_VERSION = 200;
   private static final SecurityLevel EXPECTED_ATTESTATION_SECURITY_LEVEL =
       SecurityLevel.TRUSTED_ENVIRONMENT;
-  private static final int EXPECTED_KEYMASTER_VERSION = 4;
+  private static final int EXPECTED_KEYMASTER_VERSION = 200;
   private static final SecurityLevel EXPECTED_KEYMASTER_SECURITY_LEVEL =
       SecurityLevel.TRUSTED_ENVIRONMENT;
-  private static final byte[] EXPECTED_ATTESTATION_CHALLENGE = "abc".getBytes(UTF_8);
+  private static final byte[] EXPECTED_ATTESTATION_CHALLENGE = "challenge".getBytes(UTF_8);
   private static final byte[] EXPECTED_UNIQUE_ID = "".getBytes(UTF_8);
+  @Rule public ExpectedException fakeLeafException = ExpectedException.none();
 
-  private static X509Certificate getAttestationRecord(String certStr) throws CertificateException {
-    CertificateFactory factory = CertificateFactory.getInstance("X509");
-    X509Certificate cert =
-        (X509Certificate)
-            factory.generateCertificate(new ByteArrayInputStream(certStr.getBytes(UTF_8)));
-    return cert;
+  private ImmutableList<X509Certificate> loadCertificateChain(String filePath)
+      throws FileNotFoundException, CertificateException {
+    return CertificateFactory //
+        .getInstance("X.509")
+        .generateCertificates(new FileInputStream(filePath))
+        .stream()
+        .map(c -> (X509Certificate) c)
+        .collect(ImmutableList.toImmutableList());
   }
 
   @Test
   public void testParseAttestationRecord() throws CertificateException, IOException {
-    X509Certificate x509Certificate = getAttestationRecord(CERT);
-    X509Certificate x509Certificate2 = getAttestationRecord(CERT2);
     ParsedAttestationRecord attestationRecord =
-        ParsedAttestationRecord.createParsedAttestationRecord(Arrays.asList(x509Certificate2, x509Certificate));
+        ParsedAttestationRecord.createParsedAttestationRecord(
+            loadCertificateChain(TEST_NORMAL_CERT_CHAIN_PATH));
 
     assertThat(attestationRecord.attestationVersion).isEqualTo(EXPECTED_ATTESTATION_VERSION);
     assertThat(attestationRecord.attestationSecurityLevel)
@@ -129,13 +88,40 @@ public void testParseAttestationRecord() throws CertificateException, IOExceptio
     assertThat(attestationRecord.uniqueId).isEqualTo(EXPECTED_UNIQUE_ID);
     assertThat(attestationRecord.softwareEnforced).isNotNull();
     assertThat(attestationRecord.teeEnforced).isNotNull();
+    Set<Integer> actual = attestationRecord.teeEnforced.purpose.orElse(Collections.emptySet());
+    assertThat(actual).containsExactly(KM_KEY_PURPOSE_SIGN);
+  }
+
+  @Test
+  public void testParseAttestationRecordWithAttestKeys() throws IOException, CertificateException {
+    ParsedAttestationRecord attestationRecord =
+        ParsedAttestationRecord.createParsedAttestationRecord(
+            loadCertificateChain(TEST_ATTEST_KEY_CERT_CHAIN_PATH));
+
+    assertThat(attestationRecord.attestationVersion).isEqualTo(EXPECTED_ATTESTATION_VERSION);
+    assertThat(attestationRecord.attestationSecurityLevel)
+        .isEqualTo(EXPECTED_ATTESTATION_SECURITY_LEVEL);
+    assertThat(attestationRecord.keymasterVersion).isEqualTo(EXPECTED_KEYMASTER_VERSION);
+    assertThat(attestationRecord.keymasterSecurityLevel)
+        .isEqualTo(EXPECTED_KEYMASTER_SECURITY_LEVEL);
+    assertThat(attestationRecord.attestationChallenge).isEqualTo(EXPECTED_ATTESTATION_CHALLENGE);
+    assertThat(attestationRecord.uniqueId).isEqualTo(EXPECTED_UNIQUE_ID);
+    assertThat(attestationRecord.softwareEnforced).isNotNull();
+    assertThat(attestationRecord.teeEnforced).isNotNull();
+    Set<Integer> actual = attestationRecord.teeEnforced.purpose.orElse(Collections.emptySet());
+    assertThat(actual).containsExactly(EXPECTED_KEY_PURPOSE);
+  }
+
+  @Test
+  public void testParseAttestationRecordWithAttestKeysAndFakeLeaf() throws Exception {
+    fakeLeafException.expect(IllegalArgumentException.class);
+    fakeLeafException.expectMessage("Found non-ATTEST_KEY attestation after leaf.");
+    ParsedAttestationRecord.createParsedAttestationRecord(
+        loadCertificateChain(TEST_FAKE_KEY_CERT_CHAIN_PATH));
   }
 
   @Test
   public void testCreateAndParseAttestationRecord() {
-    AuthorizationList.Builder teeEnforcedBuilder = AuthorizationList.builder();
-    teeEnforcedBuilder.userAuthType = ImmutableSet.of(UserAuthType.FINGERPRINT);
-    teeEnforcedBuilder.attestationIdBrand = "free food".getBytes(UTF_8);
     ParsedAttestationRecord expected =
         ParsedAttestationRecord.create(
             /* attestationVersion= */ 2,
@@ -147,7 +133,8 @@ public void testCreateAndParseAttestationRecord() {
             /* softwareEnforced= */ AuthorizationList.builder().build(),
             /* teeEnforced= */ AuthorizationList.builder()
                 .setUserAuthType(ImmutableSet.of(UserAuthType.FINGERPRINT))
-                .setAttestationIdBrand("free food".getBytes(UTF_8)).build());
+                .setAttestationIdBrand("free food".getBytes(UTF_8))
+                .build());
     ASN1Sequence seq = expected.toAsn1Sequence();
     ParsedAttestationRecord actual = ParsedAttestationRecord.create(seq);
     assertThat(actual.attestationVersion).isEqualTo(expected.attestationVersion);