From c799953a8dc15564b6bf4e2c8c2100876dd109f1 Mon Sep 17 00:00:00 2001 From: ComplexSpaces Date: Thu, 4 Jul 2024 15:25:58 -0500 Subject: [PATCH] Android: Don't attempt to check revocation on non-public certificates --- android/.gitignore | 1 + .../CertificateVerifierTests.kt | 19 +++++ .../platformverifier/CertificateVerifier.kt | 72 +++++++++++++++++++ .../src/tests/verification_mock/mod.rs | 15 +++- 4 files changed, 106 insertions(+), 1 deletion(-) diff --git a/android/.gitignore b/android/.gitignore index faf530b2..6c988242 100644 --- a/android/.gitignore +++ b/android/.gitignore @@ -8,3 +8,4 @@ .externalNativeBuild .cxx local.properties +emulator.log \ No newline at end of file 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 60b43f8b..6e92be95 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,6 +4,7 @@ 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 @@ -50,4 +51,22 @@ class CertificateVerifierTests { val result = verifyMockRootUsage(context) assertEquals(FAILURE_MSG, SUCCESS_MARKER, result) } + + // Note: + // + // - Full negative path (`CertificateVerifier`'s flow for unknown roots, + // are already exercised via `runMockTestSuite`. + // + // - Full positive path (`CertificateVerifier`'s flow for known roots, + // partial-chain revocation checks) 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 79421ed9..da2f34e6 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 @@ -308,6 +308,21 @@ internal object CertificateVerifier { // 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. + // + // In the case of a non-public root, such as an internal CA or self-signed certificate, + // we opt to skip revocation checks entirely. The only exception is if the server + // provided stapled OCSP data, which is an explicit signal and won't introduce non-ideal + // platform behavior when attempting validation. + // + // This is because these are cases where a user or administrator has explicitly opted to + // trust a certificate they (at least believe) have control over. These certificates rarely + // contain revocation information as well, so these cases don't lose much. + // See https://github.com/rustls/rustls-platform-verifier/issues/69 as well. + if (ocspResponse == null && !isKnownRoot(validChain.last())) { + // Chain validation must have succeeded by this point. + return VerificationResult(StatusCode.Ok) + } + val parameters = PKIXBuilderParameters(keystore, null) val validator = CertPathValidator.getInstance("PKIX") @@ -386,4 +401,61 @@ 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/user. + // + // 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 user 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/rustls-platform-verifier/src/tests/verification_mock/mod.rs b/rustls-platform-verifier/src/tests/verification_mock/mod.rs index 4971e106..9509bcf2 100644 --- a/rustls-platform-verifier/src/tests/verification_mock/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_mock/mod.rs @@ -176,10 +176,23 @@ mock_root_test_cases! { expected_result: Ok(()), other_error: no_error!(), }, - // Uses a separate certificate from the one used in the "good" case to deal + + // The revocation tests use a separate certificate from the one used in the "good" case to deal // with operating systems with validation data caches (e.g. Windows). // Linux is not included, since the webpki verifier does not presently support OCSP revocation // checking. + + // Check that self-signed certificates, which may or may not be revokved, do not return any + // kind of revocation error. It is expected that non-public certificates without revocation information + // have no revocation checking performed across platforms. + revoked_dns [ any(windows, target_os = "android", target_os = "macos") ] => TestCase { + reference_id: EXAMPLE_COM, + chain: &[include_bytes!("root1-int1-ee_example.com-revoked.crt"), ROOT1_INT1], + stapled_ocsp: None, + verification_time: verification_time(), + expected_result: Ok(()), + other_error: no_error!(), + }, stapled_revoked_dns [ any(windows, target_os = "android", target_os = "macos") ] => TestCase { reference_id: EXAMPLE_COM, chain: &[include_bytes!("root1-int1-ee_example.com-revoked.crt"), ROOT1_INT1],