From 8f17861d908e210548668c2e50a9a017cc50d688 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 1 Nov 2023 16:34:09 -0400 Subject: [PATCH 1/4] android: additional comments for CertificateVerifier.kt This commit add some additional comments to the `CertificateVerifier.kt` implementation based on the Android developer docs. In particular: * A note about the return from `checkServerTrusted` being a list ordered from EE to trust anchor. * A note that after adding a `PKIXRevocationChecker` to a `PKIXParameters` it can't be modified further or the effects of the modifications will be ignored. * A note about why `isRevocationEnabled` is set to false on the `PKIXRevocationChecker`. --- .../org/rustls/platformverifier/CertificateVerifier.kt | 9 +++++++++ 1 file changed, 9 insertions(+) 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..bd8cbd8b 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) { @@ -317,7 +320,13 @@ 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. From 60b1110cd629e097d2b57200eee541cc68788204 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 1 Nov 2023 16:48:53 -0400 Subject: [PATCH 2/4] android: only check EE certificate revocation status This commit updates the `CertificateVerifier.kt` logic used on Android to change our revocation checking preferences. Now, we only check the end entity certificate revocation status, preferring OCSP, and allowing soft failure. Checking the entire certificate chain's revocation status is infeasible with the platform options exposed to us: intermediates without complete authority information access (AIA) information will hard fail. Performing only EE revocation checking is preferable to more complicated logic that attempts to decide apriori whether the chain has enough information to support complete revocation checking or not. As a result of only checking EE status the logic and unit test for determining if a certificate is a known root (one installed in the system trust store) fall away. --- .../CertificateVerifierTests.kt | 19 ---- .../platformverifier/CertificateVerifier.kt | 103 ++---------------- 2 files changed, 12 insertions(+), 110 deletions(-) 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 bd8cbd8b..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 @@ -276,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. + // issuing certificates in use may omit authority access information (for example the + // Let's Encrypt R3 Intermediate Certificate). // - // 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: - // - // 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. @@ -329,14 +314,8 @@ internal object CertificateVerifier { // - 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()) @@ -383,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 - } } From d818937935112a7c66c56b2f22af2d06d9df9202 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 1 Nov 2023 16:38:48 -0400 Subject: [PATCH 3/4] tests: add LE chain test to verification_real_world In particular this testcase ensures that we can validate a chain from EE->intermediate->trust anchor for a chain where one or more certificates (in this case, the intermediate) are missing an authority information access (AIA) extension that specifies an OCSP access method and URI. --- .../letsencrypt_org_valid_1.crt | Bin 0 -> 1137 bytes .../letsencrypt_org_valid_2.crt | Bin 0 -> 1306 bytes .../letsencrypt_org_valid_3.crt | Bin 0 -> 1380 bytes src/tests/verification_real_world/mod.rs | 15 +++++++++++++++ 4 files changed, 15 insertions(+) create mode 100644 src/tests/verification_real_world/letsencrypt_org_valid_1.crt create mode 100644 src/tests/verification_real_world/letsencrypt_org_valid_2.crt create mode 100644 src/tests/verification_real_world/letsencrypt_org_valid_3.crt 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 0000000000000000000000000000000000000000..4dbed56cf492480e6f2b59bbf47b584aa0cc1362 GIT binary patch literal 1137 zcmXqLV#zgVVh&xv%*4pVB*c7eugSqP6;12juR9zibm#jFHUnNZPOUbNw(q=*jNGgY z21bV52Apinp)72|OrgPsVg@204wo>mPil#Jv4U$}a#3YL2}}{QFjJ7Rft)z6k+FfL zk)@G=iGh(x6o_kRWMl#68b=w37z!B(fQ(@l=FCY2nxL0olx`4dAk4-NwECPa6C)c3 zn8D1*&g{g%a@q3XvX`uX&d$qS{r`=-(9{2OLN0eQUTip^$ND;9*$#oc3}iuk zRThw^IJDUqSy|bcfmTCVOa?q4Xmh0I0t=4;mjMSGTY4i8BNL-R6C)EF7ck&h z7_U__u`sbTer9R>!0?WtlwrHwZr2`Pt-==vy$*s}I|X5FywLvW)GPz1)~AnFehR(3anrFMWw zb6Wp!zk6t>AA{8T4dUCP<~q%qFx^sFSie0i{6M_0?*_2 z3b$Fw@A?#e?P~sRt@rkyO+oYQgR$~O8y!o^@{fcny;EQL&$jPkR8bp;>nhWA%?r1g KS&JYE{~5g;AP{~YV&CO&dbQi&B|cl zZ^&)H$;KSY!Y0fV8f>U(pbp}22`hN!m82HsrIsiJrzV#cWtLPb1f>?ICKe@UD7Y8p zmlha`8VG}wG7Ix~1_!w-1m)+KC`1?<$cghB85md^m>7UT6p(9bU}i>{#klr@pXRpDI%e!;XU(K zEV7jR+GOLj(l76;^&6z(8 z#AA0A<^OIxy7p3Agsu4T=bXDgYJYFLbMRJS>=n1iXV$77?AVi#UYfS~qr~?`G0%ek zTXk%6U;1BI;?)e!a{IZ#KhHBh{kp6`Tx5OnlK(R|Po8@xcsbiYk5<`*nd?+bcMG2h zV*dIzaAEgtQ6^?a2FArrj2yraVKLwX2B<7QBjbM-7GNT1Gmr)GRarnG&7sZ4$jZvj z%mimK8VG@;g+aGrR#$eo~%5Jf`OcY%mS$e637aZkrgU|Y*%2BHjp&nU}H;f0^%a4a^@zWr&>?>x!W!N-s;l2=W2SzrWwg=OMT_0*&3%7h3Gae zcy;*g4~6~lXSNqGY|pd)7B}VI6NUN9-gj?ee!gg{n9amW8&X z%-(q}=2Oav1N$lu`1j?y@Wf5pt@piK;Nc5d7tPy|3U8BlD*g~sn=(0kfov+vK`y0r z0=&-C7fQ(2J$TS&zBOv&UW5JZdD>e475$`H4}X=I{vmG7;iWsWKIrYSHs1Np``(#9 zPu90^x7i;EbvFB!@z6{>t8eDT|4SW~n`}RDA=%Q@vNZ40uCf!8nO?5+&JjPy!*p|R z$I5B||t$-I`!kiAsP`9|pUq9dAo-;c!loml7AVsQOaYrMq5%H7Z7 z3cA@JwoN{~v;R(Fp{myU`)^ePf-<@%-FbR#>*HIs7us`L6b;ukef_<2^@&b#+lM|+ zE%?6e)!sX;QRMa2+qMeJ>mn~d`VsLndWXl^e=+`In*ZcNmDisT+|c`~X7U7a{l9A# zak{(Ne|WiJ`+p7J45Mr5adMf9C-3+=w_Bh4Qjqhqe53GGU!%tR7QwBtb+KuhuXfyh uGIi_Otzkk=XOH+DQ?+mj$bEB;AyneuOV5-mey66-*%E!Ac*W`+?uP)U>;Po| literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..79a33ba5908c84a1d790715853f435daee569995 GIT binary patch literal 1380 zcmZ8hX;4#F7|qM_2mwM&*eu$Fr2?YKjgd_dWM4Hxumr^pF+@P)OL%!<8I(e(5iA&| zqJoT;#TJS}2W4?ki&C*_of<7=P#~xk;1SKNl#73 zL{gR@JrxmQN>sv%N0kbU6)a3(BsVrTIO`YaHLTUzetqh8$~a7QeHi@9kthc*3o8$h*(; z>xh^i%o)0PNl$;_a4P!xVRc4&)b8Cn=BEM-gTGL*US4Q~u_wbYZ zDfms zEcV=m+^FFg`e)|yQi5C6h`@Q4@$l*LPTIw&l7Qk`QLOI#A(OH>amFlrRJNnMV5B%{ zeU3EqneWkEITQV7PjOatYiT1?-9y?Do5l8p`h{N}<4wvnp5)_`qh2R0Je6k_sc7@c z$}5C7k)*zYK~l@=-oWYeQ08%pW#7;rSLsX-j;pQoLHYH1)08YE+^gfa^NC-io&FN- zE$)T`-TV{%2UpEI-(FU?y*6eOGpxFzwQfT!qV_#=0Ij2wvJ?OGkIl9=BWOys9MT2< z+^7}N3KISAn@@(fGWG`_!!F*zCXX;7vKI90$nb12>9`I+QbfDTxBUA9$%kud-Bfow zt`-vQYYGX0)xD$__FZky>-VdD!|Eee7Y$^d%IswB`EKKK&5ZY}6x}U%3yQ0Lp?}mm zsK-(3vU4P7$!=0!)VO8h;ny2(4!Xo5`YS2(ykI__0O6oo6%+*-45z1 zV7;~Lu{7wita7e}-xX3sR^FX4skGXKWb0;KI((AmYpRjY%j7gc^X2g$wKYGxLp>+v zx>+8)H+>?{;f&IjORWv6(Bs!${@~;~*<1Z|tiOQM!Vwzk@OJYaWjIrz+O_~17) zu&>n3$uxN&6dyyk^POI#>IOpo5|%eTxf&W;U9@xeoh90|Om%Bnkj3Kk?3fy-J53w& detY&nbKg{?>MOX?eQGZsGWX76zZsnL`3L9%8WsQm literal 0 HcmV?d00001 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. // From 57ad2ba004def0365466ebd3eebe11e12767b596 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 2 Nov 2023 10:57:46 -0400 Subject: [PATCH 4/4] tests: generalize update_valid_1_cert.bash 1. Rename to `update_valid_ee_certs.bash` 2. Don't hardcode location of `bash` for shebang. 3. Remove echo's about potential future extensions. 4. Add a helper function for fetching EE certs. 5. Use helper to update all three valid realworld testcase EE certs instead of just `1password_com_valid_1.crt` --- .../update_valid_1_cert.bash | 10 --------- .../update_valid_ee_certs.bash | 21 +++++++++++++++++++ 2 files changed, 21 insertions(+), 10 deletions(-) delete mode 100755 src/tests/verification_real_world/update_valid_1_cert.bash create mode 100755 src/tests/verification_real_world/update_valid_ee_certs.bash 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"