Only check end-entity certificate revocation status on Android

android/rustls-platform-verifier/src/androidTest/java/org/rustls/platformverifier/CertificateVerifierTests.kt

@@ -4,7 +4,6 @@ import android.content.Context
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
 import org.junit.Assert.assertEquals
-import org.junit.Assert.assertTrue
 import org.junit.BeforeClass
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -51,22 +50,4 @@ class CertificateVerifierTests {
         val result = verifyMockRootUsage(context)
         assertEquals(FAILURE_MSG, SUCCESS_MARKER, result)
     }
-
-    // Note:
-    //
-    // - Full negative path (`CertificateVerifier`'s flow for unknown roots,
-    // end-entity-only revocation check) already exercised via `runMockTestSuite`.
-    //
-    // - Full positive path (`CertificateVerifier`'s flow for known roots,
-    // full-chain revocation check) already exercised via `runRealWorldTestSuite`.
-    @Test
-    fun runTestIsPublicRoot() {
-        val rootCAs = CertificateVerifier.getSystemRootCAs()
-
-        // Positive - can ID known roots
-        assertTrue(rootCAs.isNotEmpty())
-        for (ca in rootCAs) {
-            assertTrue(CertificateVerifier.isKnownRoot(ca))
-        }
-    }
 }

android/rustls-platform-verifier/src/main/java/org/rustls/platformverifier/CertificateVerifier.kt

@@ -247,6 +247,9 @@ internal object CertificateVerifier {
         // hostname verifier. Additionally, even the RFC 2818 verifier is not available until API 24.
         //
         // `serverName` is only used for pinning/CT requirements.
+        //
+        // Returns the "the properly ordered chain used for verification as a list of X509Certificates.",
+        // meaning a list from end-entity certificate to trust-anchor.
         val validChain = try {
             trustManager.checkServerTrusted(certificateChain.toTypedArray(), authMethod, serverName)
         } catch (e: CertificateException) {
@@ -273,38 +276,23 @@ internal object CertificateVerifier {
             //
             // 2: Likely because of 1, Android requires all issued certificates to have some form of
             // revocation included in their authority information. This doesn't work universally as
-            // internal CAs managed by companies aren't required to follow this (and generally don't),
-            // so verifying those certificates would fail.
-            //
-            // Revocation checking has two factors:
-            //
-            // 1. Is the root CA known (installed in system trust store)?
-            // 2. Did the server staple an OSCP response for it's own leaf certificate?
-            //
-            // Thus the below revocation logic handles four cases:
+            // issuing certificates in use may omit authority access information (for example the
+            // Let's Encrypt R3 Intermediate Certificate).
             //
-            // 1. Known root + OSCP stapled -> Full-chain revocation, no extra network use
-            // 2. Known root + no OSCP stapled -> Full-chain revocation, with extra network use
-            // 3. Unknown root + OSCP stapled -> End-entity-only revocation, no extra network use
-            // 4. Unknown root + no OSCP stapled -> End-entity-only revocation, with extra network use
+            // Given these constraints, the best option is to only check revocation information
+            // at the end-entity depth. We will prefer OCSP (to use stapled information if possible).
+            // If there is no stapled OCSP response, Android may use the network to attempt to fetch
+            // one. If OCSP checking fails, it may fall back to fetching CRLs. We allow "soft"
+            // failures, for example transient network errors.
             val parameters = PKIXBuilderParameters(keystore, null)
 
             val validator = CertPathValidator.getInstance("PKIX")
             val revocationChecker = validator.revocationChecker as PKIXRevocationChecker
 
-            // `PKIXRevocationChecker` checks the entire chain by default.
-            // We allow it to fail if there are network issues.
-            // If the chain's root is known, use this default setting for full-chain
-            // revocation (excludes root itself, see below).
-            // Else, only check revocation status for the end-entity.
-            revocationChecker.options = if (isKnownRoot(validChain.last())) {
-                EnumSet.of(PKIXRevocationChecker.Option.SOFT_FAIL)
-            } else {
-                EnumSet.of(
-                    PKIXRevocationChecker.Option.SOFT_FAIL,
-                    PKIXRevocationChecker.Option.ONLY_END_ENTITY
-                )
-            }
+            revocationChecker.options = EnumSet.of(
+                PKIXRevocationChecker.Option.SOFT_FAIL,
+                PKIXRevocationChecker.Option.ONLY_END_ENTITY
+            )
 
             // Use the OCSP data `rustls` provided, if present.
             // Its expected that the server only sends revocation data for its own leaf certificate.
@@ -317,17 +305,17 @@ internal object CertificateVerifier {
             }
 
             // Use the custom revocation definition.
+            // "Note that when a `PKIXRevocationChecker` is added to `PKIXParameters`, it clones the `PKIXRevocationChecker`;
+            // thus any subsequent modifications to the `PKIXRevocationChecker` have no effect."
+            //  - https://developer.android.com/reference/java/security/cert/PKIXRevocationChecker
             parameters.certPathCheckers = listOf(revocationChecker)
+            // "When supplying a revocation checker in this manner, it will be used to check revocation
+            // irrespective of the setting of the `RevocationEnabled` flag."
+            //  - https://developer.android.com/reference/java/security/cert/PKIXRevocationChecker
             parameters.isRevocationEnabled = false
 
-            // Validate the revocation status of all non-root certificates in the chain.
+            // Validate the revocation status of the end entity certificate.
             try {
-                // `checkServerTrusted` always returns a trusted full chain. However, root CAs
-                // don't have revocation properties so attempting to validate them as such fails.
-                // To avoid this, always remove the root CA from the chain before validating its
-                // revocation status. This is identical to the `CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT`
-                // flag in the Win32 API.
-                validChain.removeLast()
                 validator.validate(certFactory.generateCertPath(validChain), parameters)
             } catch (e: CertPathValidatorException) {
                 return VerificationResult(StatusCode.Revoked, e.toString())
@@ -374,62 +362,4 @@ internal object CertificateVerifier {
 
         return String(hexChars)
     }
-
-    // Check if CA root is known or not.
-    // Known means installed in root CA store, either a preset public CA or  a custom one installed by an enterprise.
-    // Function is public for testing only.
-    //
-    // Ref: https://source.chromium.org/chromium/chromium/src/+/main:net/android/java/src/org/chromium/net/X509Util.java;l=351
-    fun isKnownRoot(root: X509Certificate): Boolean {
-        // System keystore and cert directory must be non-null to perform checking
-        systemKeystore?.let { loadedSystemKeystore ->
-            systemCertificateDirectory?.let { loadedSystemCertificateDirectory ->
-
-                // Check the in-memory cache first
-                val key = Pair(root.subjectX500Principal, root.publicKey)
-                if (systemTrustAnchorCache.contains(key)) {
-                    return true
-                }
-
-                // System trust anchors are stored under a hash of the principal.
-                // In case of collisions, append number.
-                val hash = hashPrincipal(root.subjectX500Principal)
-                var i = 0
-                while (true) {
-                    val alias = "$hash.$i"
-
-                    if (!File(loadedSystemCertificateDirectory, alias).exists()) {
-                        break
-                    }
-
-                    val anchor = loadedSystemKeystore.getCertificate("system:$alias")
-
-                    // It's possible for `anchor` to be `null` if the user deleted a trust anchor.
-                    // Continue iterating as there may be further collisions after the deleted anchor.
-                    if (anchor == null) {
-                        continue
-                        // This should never happen
-                    } else if (anchor !is X509Certificate) {
-                        // SAFETY: This logs a unique identifier (hash value) only in cases where a file within the
-                        // system's root trust store is not a valid X509 certificate (extremely unlikely error).
-                        // The hash doesn't tell us any sensitive information about the invalid cert or reveal any of
-                        // its contents - it just lets us ID the bad file if a customer is having TLS failure issues.
-                        Log.e(TAG, "anchor is not a certificate, alias: $alias")
-                        continue
-                        // If subject and public key match, it's a system root.
-                    } else {
-                        if ((root.subjectX500Principal == anchor.subjectX500Principal) && (root.publicKey == anchor.publicKey)) {
-                            systemTrustAnchorCache.add(key)
-                            return true
-                        }
-                    }
-
-                    i += 1
-                }
-            }
-        }
-
-        // Not found in cache or store: non-public
-        return false
-    }
 }

src/tests/verification_real_world/mod.rs

@@ -85,6 +85,14 @@ const VALID_UNRELATED_CHAIN: &[&[u8]] = &[
     SHARED_CHAIN[2],
 ];
 
+const LETSENCRYPT_ORG: &str = "letsencrypt.org";
+
+const VALID_LETSENCRYPT_ORG_CHAIN: &[&[u8]] = &[
+    include_bytes!("letsencrypt_org_valid_1.crt"),
+    include_bytes!("letsencrypt_org_valid_2.crt"),
+    include_bytes!("letsencrypt_org_valid_3.crt"),
+];
+
 macro_rules! real_world_test_cases {
     { $( $name:ident => $test_case:expr ),+ , } => {
         real_world_test_cases!(@ $($name => $test_case),+,);
@@ -210,6 +218,13 @@ real_world_test_cases! {
         expected_result: Err(TlsError::InvalidCertificate(CertificateError::NotValidForName)),
         other_error: no_error!(),
     },
+    letsencrypt => TestCase {
+        reference_id: LETSENCRYPT_ORG,
+        chain: VALID_LETSENCRYPT_ORG_CHAIN,
+        stapled_ocsp: None,
+        expected_result: Ok(()),
+        other_error: no_error!(),
+    },
 
     // OCSP stapling works.
     //

src/tests/verification_real_world/update_valid_ee_certs.bash

@@ -0,0 +1,21 @@
+#!/usr/bin/env bash
+
+set -euo pipefail
+
+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+
+fetch_ee_cert() {
+  local domain="$1"
+  local out_file="$2"
+
+  echo -n |
+    openssl s_client \
+      -connect "$domain:443" \
+      -servername "$domain" |
+    openssl x509 \
+      -outform DER > "$DIR/$out_file"
+}
+
+fetch_ee_cert "my.1password.com" "1password_com_valid_1.crt"
+fetch_ee_cert "agilebits.com" "agilebits_com_valid_1.crt"
+fetch_ee_cert "lencr.org" "letsencrypt_org_valid_1.crt"