Upstream merge 2024-06-13 #1636
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
andrewhop
reviewed
Jun 14, 2024
andrewhop
approved these changes
Jun 14, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
==========================================
+ Coverage 78.16% 78.17% +0.01%
==========================================
Files 562 562
Lines 94683 94685 +2
Branches 13575 13575
==========================================
+ Hits 74008 74021 +13
+ Misses 20082 20069 -13
- Partials 593 595 +2 ☔ View full report in Codecov by Sentry. |
nebeid
reviewed
Jun 17, 2024
There was a problem hiding this comment.
If this fix can be an amending to the corresponding commit cbb6b24, that would be better, imo, since we will not squash, so that it's an integral change.
crypto/x509/v3_purp.c
Outdated
There was a problem hiding this comment.
If those fixes can be an amending to the corresponding commit 1e436b5, it would be better, imo, to make it an integral change since we will not squash.
dkostic
force-pushed
the
upstream-merge-2024-06-13
branch
from
40fa875 to
8634c14
Compare
All this flag does is cause verify_cb to be called with ok=2 after policy validation happens, breaking the otherwise strict 0/1 behavior of the callback. We can't quite remove the symbol because a lot of bindings libraries wrap it without realizing what it does. But no one actually uses it, because it's pretty useless. Since we now always (other than the bad_chain thing) check policies and that happens last, this flag really means "please call the verify callback an extra time at the end with ok=2". Update-Note: X509_V_FLAG_NOTIFY_POLICY is now a no-op. This is not expected to impact anyone. Change-Id: I892a872181d1c1836ef2533ac616edfb6b3b5836 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65087 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit b2e57a1c132a34938ee3051d57c5dfa2ef64ff42)
If a verify_cb consistently returns -1, it would broadly get treated as success, except the final call would leak into ok and come out of X509_verify_cert and read as failure. Prevent this ambiguity by requiring the return value be 0 or 1, and aborting otherwise. Update-Note: If the verify callback returns anything other than 0 or 1, X509_verify_cert will now crash in BSSL_CHECK. If this happens, fix the callback to use the correct return value. Change-Id: I0394e68febe9f22a42bcd5de8ea4f3a82b07c862 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65107 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit 7a813621dac6878ab53b6ed7392939a8982226e8)
All callers I found seem to be compatible with this. Using the non-const pointer isn't very useful because you cannot resize the value. Let's try const-correcting it and we'll revert if it's too annoying to fix. Update-Note: The above functions are now const-correct. Store the result in a const pointer to avoid compatibility issues. Change-Id: Id4a1c7223fbb333716906e20844bf8795118a8ea Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65128 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit 3ef8cbc419c3143fb3218eac1b1162515573ecb0)
In particular, deprecate get_crl and check_crl. Only gRPC uses them, and does so incorrectly. It is impossible to implement those callbacks correctly. In AWS-LC we didn't take this part of the commit: Also remove X509_STORE_CTX_set_cert. No one uses it, and it's redundant with X509_STORE_CTX_init. We should remove X509_STORE_CTX_set_chain too, but there are some callers to cleanup. Bug: 426 Change-Id: I846f8292a5f5d6cc3b5d2a576bfaf86e9ea84bdc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65147 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit e9539957ce42b07dc6f8b9bd23c28c7d2ef2bd3b)
Bug: 426 Change-Id: Ie1ba74a940db1525926da1856bb98d350d977674 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65149 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 2a88b4b65da2794378044be9d125ce987ffd39f3)
These tables are small enough that a linear scan is fine. This is one less thing we need to keep in sync, and means we can remove entries without renumbering them. Change-Id: If1a41397aac3917534529e7e704983489e266a0f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65150 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit 0beff26c59e67e2e19d173f1bd23241a0e946fd9)
dkostic
force-pushed
the
upstream-merge-2024-06-13
branch
from
8634c14 to
f8a37fa
Compare
nebeid
approved these changes
Jun 17, 2024
lilpoozie2005
approved these changes
Sep 11, 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.
Issues:
N/A
Description of changes:
Upstream merge.
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
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.