Upstream merge 2024 03 18 #1501
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nebeid
force-pushed
the
upstream-merge-2024-03-18
branch
from
6cf04fa
to
bbd3a0f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1501 +/- ##
==========================================
+ Coverage 76.98% 77.19% +0.21%
==========================================
Files 425 425
Lines 71553 71264 -289
==========================================
- Hits 55082 55012 -70
+ Misses 16471 16252 -219 ☔ View full report in Codecov by Sentry. |
Update-Note: Though exported, this was an internal flag to the delta CRL implementation. Remove it. Bug: 601 Change-Id: Ic7f99da94391aea861fd7ea9ad79a3fb66cc649e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63930 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 3e28180d3a337a524c07c9301a967aaa739a10ec)
These extensions were only parsed for delta CRL support. Instead, ignore CRL numbers and treat critical delta CRL extensions as unrecognized critical extensions. This trims a pile of dead, untested code from the verifier. Update-Note: While this is broadly a no-op, this may change behavior slightly at the edges. Invalid CRL number extensions will now be ignored instead of treated as a parse error. A delta CRL that incorrectly marks its delta CRL extension as non-critical will be interpreted as a normal CRL. (This is the expected behavior for an implementation which does not implement delta CRLs. Extensions like this are supposed to be marked critical.) Bug: 601 Change-Id: Iece9a8acce83a7c59e129499517fc8eb0d048e98 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63931 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 0c4481340ba40d1aa06f2e5dce8f9cee5d6d5b31)
This is now always NULL. Bug: 601 Change-Id: I030b41fd65973ca13b96c426962430ea8a6ae198 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63932 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit d9a58a171a15a469b37b00e8b5f3c6a098a1297f)
This is part of delta CRLs, where OpenSSL would return 2 instead of 1 to signal a removedFromCRL entry. The logic that caught that was removed in the preceeding CL. Bug: 601 Change-Id: I5ac0b32a864b09d303198ca99ecbfc1219906e36 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63933 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 9177d65315147771026640d9af4c101435eb737d)
This is part of onlySomeReasons handling, which was removed with extended CRL support. Demonstrating that the code to be removed is a no-op takes a few steps, so I'm splitting this into a couple CLs for ease of review. First, every CRL either has the IDP_REASONS set, or idp_reasons is CRLDP_ALL_REASONS. The only thing that reads idp_reasons is crl_crldp_check, which is run after get_crl_score skips all IDP_REASONS certificates. Thus, the field can be assumed to be CRLDP_ALL_REASONS. Bug: 601 Change-Id: I4d41b5665d35db10dd9752e47553ae90f46f1e44 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63934 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> (cherry picked from commit 39cc892c73d6c3faf2e604c44509f132c232f24c)
This isn't *quite* a no-op, but it's the other half of removing support for partitioned CRLs. The distribution point's reasons field and the CRL's onlySomeReasons field are masked together to determine which reasons we have covered so far. This is used in some complex logic from RFC 5280, section 6.3.3 to loop through a bunch of CRLs before determining that we've covered evertything. OpenSSL's "extended CRL" feature skipped all CRLs with an onlySomeReasons field, but did not condition on the distribution point, so the loop was still active in some cases. The new verifier from Chromium doesn't support either. If the distribution point has a reasons field, we ignore it. Align with Chromium. Now the reasons field is always the special all-reasons value. As part of this, this removed the dp_reasons field from DIST_POINT. This is a public struct, but was unused outside of cl/581761514, so we can stop computing it. Update-Note: See above. Bug: 601 Change-Id: I9b0a9766b281d3486874e1b6d4d415a51e50ba59 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63935 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 580c04109e8a63d08582b3d948cf54849371a73e)
Now that we only process CRLs that cover all reasons: 1. A successful get_crl will always set current_reasons to CRLDP_ALL_REASONS. 2. The last_reasons == current_reasons will never happen. 3. The loop always makes exactly one iteration. A footnote on point 1: it is also possible for the caller to override get_crl. In that case, the caller's get_crl was previously responsible for setting current_reasons, but there was no way to do so. In reality, that callback was actually impossible to use correctly. See openssl/openssl#21679 and openssl/openssl#10211. I previously attempted to remove those first, but gRPC did not notice it was unusable and use it anyway. Instead, they're suppressing X509_V_ERR_UNABLE_TO_GET_CRL via the callback, which is probably working around the bug in their get_crl implementation. Later, when we tackle the callback, we'll probably need to unwind the gRPC mess and, in the process, add a X509_STORE_CTX_set_current_reasons for them to call for OpenSSL compatibility. For now, this change has the side effect of removing the need for them to call that. Bug: 601 Change-Id: Icc5c0fb195d9f66991d0e560911f304e82afa5fd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63936 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 8cae3d04ed7264ae99024caff4da79e9a796b915)
We never set CRL_SCORE_TIME_DELTA. Bug: 601 Change-Id: Ic7497492565bda9f3e9d7091f5e16b81e111cfa1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63937 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 9bed3737639dfaf1394cf0518e3de896a0390f0e)
We'll never accept CRLs where the issuers don't match. That means CRL_SCORE_ISSUER_NAME is always set, so we can remove code that conditions on it. Update-Note: This also makes a corresponding distribution point change to ignore distribution points with a CRLissuer field. Before, we would check for it to match the CRL issuer, but this field is only meant to be used with indirect CRLs (RFC 5280, section 6.3.3, step b.1). The old code didn't include this, so I think it isn't *quite* a no-op on some invalid DP/CRL pairs, but it matches the new verifier from Chromium. Bug: 601 Change-Id: Ibe409b88cae1c2b78b3924e61270884d6e0eb436 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63938 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 54eaf6ba8c88ac4ce8847e7fd5cb33c6c59e92bc)
Avoid polluting the global namespace a bit. The idp_flags field itself is private, so it is impossible for a caller to do anything useful with it. Change-Id: I5853e72a3f37a93c75fb73ed5d7ca467b9ba01a3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63939 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit d53bbba11f164a275659d48356e992537e07774e)
ASN1_BOOLEAN is int, so this is a no-op. But they're parsed as ASN1_FBOOLEAN (boolean with default false), so we should use the typedef. Also leave some TODOs for future cleanup opportunities. Change-Id: I7b060e7530f0ef1afb3818c043727feb6fd4fcbf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63940 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 33d6e8170aaaa24b019e730051f163ea62c9b7c1)
This is part of indirect CRLs, though it wasn't correctly conditioned on them. All CRL entries are now expected to relate to the CRL issuer. This removes the very complicated delta encoding this extension uses. (This extension applies to not just the current CRL entry, but all subsequent CRL entries that don't override it. This also means sorting a CRL is destructive.) Bug: 601 Change-Id: I388baee64719007648fb3b5defea82a4661735d2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63941 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit ae40ad9b2191723bee9d7beac343dd54afea81e3)
crl_akid_check now always, on success, leaves CRL_SCORE_SAME_PATH in the score. (Note CRL_SCORE_ISSUER_CERT contains CRL_SCORE_SAME_PATH.) This means the recursive validation logic in check_crl_path never runs, and we can remove a very worrying re-entrant call in the validator. The CRL must be issued by either the issuer, or some ancestor in the chain. (This also matches the behavior of the new validator.) Bug: 601 Change-Id: Ie5c0feb5bb5ade3bfd49e338a637196fce29fd2a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63942 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit a4851dd8c67a2b311403d67270e02e7296d31157)
nebeid
force-pushed
the
upstream-merge-2024-03-18
branch
from
cea6d94
to
1ae65ac
Compare
justsmth
approved these changes
Apr 2, 2024
skmcgrail
approved these changes
Apr 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Merging from Upstream considering commits from google/boringssl@3e28180 (Nov 17, 2023) to google/boringssl@a4851dd (Nov 20, 2023).
Call-outs:
See internal document as well as "AWS-LC" notes inserted in some of the commit messages for additions/deviations from the upstream commit.
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.