Skip to content

Check CRLs using a loop - #3496

Draft
nhatnghiho wants to merge 1 commit into
aws:mainfrom
nhatnghiho:crl-loop
Draft

Check CRLs using a loop#3496
nhatnghiho wants to merge 1 commit into
aws:mainfrom
nhatnghiho:crl-loop

Conversation

@nhatnghiho

@nhatnghiho nhatnghiho commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Context and motivation

When checking for revoked certificates, X509_verify_cert selects a single "best" CRL (i.e. highest score) and checks the certificate only against that one. RFC 5280 §6.3.3 specifies checking the certificate against every applicable CRL. Under single-CRL selection, a revocation carried by a non-selected-but-valid CRL was silently missed, so a revoked certificate could verify as X509_V_OK.

Description of changes

X509_verify_cert now checks a certificate against all valid CRLs, not just the single highest-scoring one. If the cert shows up in any of them, it's revoked. The loop bails out on the first CRL that lists it.

When there's no fully valid CRL, it falls back to the best near-match so the error is still specific (expired, bad critical extension) instead of a generic UNABLE_TO_GET_CRL. The best near-match CRL will be the one with the highest scores still. In case of a tie, the freshest CRL will be picked. In the case of no usable CRL at all, the function behaves exactly as before.

Testing

New and updated unit tests in crypto/x509/x509_test.cc

Review considerations

X509_STORE_set_get_crl and X509_STORE_set_check_crl let a caller override how CRLs are fetched and validated — the hooks the old single-CRL path relied on. Both are already documented as do-not-use, have no downstream dependencies, and were removed by BoringSSL (8a0da66) as "impossible to use correctly" and the cause of "weird statefulness around ctx->error_depth." This PR no longer routes verification through them. Deleting the API surface itself is a follow-up.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter

codecov-commenter commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.46445% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.33%. Comparing base (ebe9cfe) to head (585548d).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
crypto/x509/x509_vfy.c 73.87% 29 Missing ⚠️
crypto/x509/x509_test.cc 92.00% 5 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3496      +/-   ##
==========================================
+ Coverage   78.04%   78.33%   +0.28%     
==========================================
  Files         700      700              
  Lines      124704   125659     +955     
  Branches    17323    17380      +57     
==========================================
+ Hits        97331    98430    +1099     
+ Misses      26505    26355     -150     
- Partials      868      874       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🔒 Security ReviewView Report

Please review before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants