Skip to content

Commit

Permalink
Android: Don't attempt to check revocation on non-public certificates
Browse files Browse the repository at this point in the history
  • Loading branch information
complexspaces committed Jul 4, 2024
1 parent b7ac0cf commit c799953
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 1 deletion.
1 change: 1 addition & 0 deletions android/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@
.externalNativeBuild
.cxx
local.properties
emulator.log
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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
}
}
15 changes: 14 additions & 1 deletion rustls-platform-verifier/src/tests/verification_mock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down

0 comments on commit c799953

Please sign in to comment.