Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct Android revocation implementation for intermediate root(s) #13

Closed
complexspaces opened this issue Mar 31, 2023 · 5 comments
Closed
Assignees
Labels
bug Something isn't working O-Android Work related to the Android verifier implementation

Comments

@complexspaces
Copy link
Collaborator

complexspaces commented Mar 31, 2023

The current implementation of revocation checking for Android is not fully reliable/correct (failing closed, not open). In some cases, it incorrectly believes that a certificate chain has been revoked.

Notably, it seems to dislike LetsEncrypt's R3 intermediate CA. This can be demonstrated with this snippet placed in rustls_platform_verifier_real_world_test_suite:

let builder = reqwest::blocking::ClientBuilder::new()
    .use_preconfigured_tls(crate::tls_config());

let client = builder.build().expect("TLS builder should be accepted");

log::info!("making request to problematic site");
match client
    .get("https://letsencrypt.org")
    .send()
{
    Ok(_) => log::info!("test passed"),
    Err(e) => log::error!("failed to talk to LE: {e}"),
}

You also need to put this in the middle of AndroidManifest.xml:

<uses-permission android:name="android.permission.INTERNET" />

With the above, it logs a revocation error and fails:

certificate was revoked: java.security.cert.CertPathValidatorException: Certificate does not specify OCSP responder
failed to verify TLS certificate: invalid peer certificate contents: certificate has been revoked
Sending fatal alert BadCertificate
failed to talk to LE: error sending request for url (https://letsencrypt.org/): error trying to connect: invalid peer certificate contents: certificate has been revoked

If we look at LetsEncrypt's intermediate CA, we can see that it indeed doesn't have one and only has a CRL:
image

In comparison to one of Amazon's intermediate, which is used in our current real world test suite:
image

The error is thrown here in the Java revocation checker, but the root cause seems to be further up where the check codepath decides if it will try the OCSP path or CRL one first. If it takes either of the OCSP paths for a certificate without a stapled response or OCSP responder URL attached, it throws and then gets re-thrown because it considers it "revoked" if one isn't present.

We should figure out if there's a way to fix this while still getting the desired behavior:

  • If there's a stapled OCSP response for a certificate, always solely use that.
  • Prefer any OCSP URL attached to a leaf certificate or intermediate root.
  • If none of the above is available, fallback to using included CRLs.
@complexspaces complexspaces added bug Something isn't working O-Android Work related to the Android verifier implementation labels Mar 31, 2023
@complexspaces complexspaces added this to the 0.1 crates.io release milestone Mar 31, 2023
@djc
Copy link
Member

djc commented Aug 16, 2023

@jsha would it be feasible to get an OCSP responder in future intermediate certificates from Let's Encrypt?

@jsha
Copy link

jsha commented Aug 16, 2023

I'm afraid not. OCSP and CRL generation is one of our more compliance-critical activities, so we try to minimize the different mechanisms we support for any given profile in order to minimize risk.

I'm surprised this issue doesn't come up more often, though. Do most apps using the Android verifier not do revocation checking? Or perhaps only check the leaf?

@complexspaces
Copy link
Collaborator Author

Do most apps using the Android verifier not do revocation checking? Or perhaps only check the leaf?

I don't how much of it is an issue with Android's implementation vs the way I wrote it being wrong, or not using some common abstraction I'm unaware of that works around the issue. Only checking the leaf seems like it could be common though.

Either way, I don't think this is something that needs fixed on the CA side of things.

@cpu cpu self-assigned this Aug 31, 2023
@cpu cpu changed the title Correct Android revocation implementation for intermdiate root(s) Correct Android revocation implementation for intermediate root(s) Sep 5, 2023
@cpu
Copy link
Member

cpu commented Sep 6, 2023

I have some webpki related work in the queue ahead of this, but I put together a branch that reproduces the issue using the instructions linked above. I plan to dig in more once I've finished the webpki tasks.

@cpu
Copy link
Member

cpu commented Nov 27, 2023

I think we can consider this resolved based on #40.

@cpu cpu closed this as completed Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working O-Android Work related to the Android verifier implementation
Projects
None yet
Development

No branches or pull requests

4 participants