diff --git a/android/rustls-platform-verifier/src/androidTest/java/org/rustls/platformverifier/CertificateVerifierTests.kt b/android/rustls-platform-verifier/src/androidTest/java/org/rustls/platformverifier/CertificateVerifierTests.kt index e7cf3116..60b43f8b 100644 --- a/android/rustls-platform-verifier/src/androidTest/java/org/rustls/platformverifier/CertificateVerifierTests.kt +++ b/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)) - } - } } diff --git a/android/rustls-platform-verifier/src/main/java/org/rustls/platformverifier/CertificateVerifier.kt b/android/rustls-platform-verifier/src/main/java/org/rustls/platformverifier/CertificateVerifier.kt index 893b39ac..95f0e400 100644 --- a/android/rustls-platform-verifier/src/main/java/org/rustls/platformverifier/CertificateVerifier.kt +++ b/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 - } } diff --git a/src/tests/verification_real_world/letsencrypt_org_valid_1.crt b/src/tests/verification_real_world/letsencrypt_org_valid_1.crt new file mode 100644 index 00000000..4dbed56c Binary files /dev/null and b/src/tests/verification_real_world/letsencrypt_org_valid_1.crt differ diff --git a/src/tests/verification_real_world/letsencrypt_org_valid_2.crt b/src/tests/verification_real_world/letsencrypt_org_valid_2.crt new file mode 100644 index 00000000..2d66ea72 Binary files /dev/null and b/src/tests/verification_real_world/letsencrypt_org_valid_2.crt differ diff --git a/src/tests/verification_real_world/letsencrypt_org_valid_3.crt b/src/tests/verification_real_world/letsencrypt_org_valid_3.crt new file mode 100644 index 00000000..79a33ba5 Binary files /dev/null and b/src/tests/verification_real_world/letsencrypt_org_valid_3.crt differ diff --git a/src/tests/verification_real_world/mod.rs b/src/tests/verification_real_world/mod.rs index 609f8e39..8df61b9a 100644 --- a/src/tests/verification_real_world/mod.rs +++ b/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. // diff --git a/src/tests/verification_real_world/update_valid_1_cert.bash b/src/tests/verification_real_world/update_valid_1_cert.bash deleted file mode 100755 index 6b413feb..00000000 --- a/src/tests/verification_real_world/update_valid_1_cert.bash +++ /dev/null @@ -1,10 +0,0 @@ -#!/bin/bash -set -euo pipefail - -echo 'This script only updates 1password_com_valid_1.crt' -echo 'It can likely be extended to download the whole chain.' - -DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" - -echo -n | openssl s_client -connect my.1password.com:443 -servername my.1password.com \ - | openssl x509 -outform DER > "$DIR/1password_com_valid_1.crt" diff --git a/src/tests/verification_real_world/update_valid_ee_certs.bash b/src/tests/verification_real_world/update_valid_ee_certs.bash new file mode 100755 index 00000000..0eb95beb --- /dev/null +++ b/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"