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.

Sign up for GitHub

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

Jump to bottom

Upstream merge 2024 06 03 #1621

Merged
merged 15 commits into from
Jun 12, 2024
Merged

Upstream merge 2024 06 03 #1621

merged 15 commits into from
Jun 12, 2024

Conversation

samuel40791765
Copy link
Contributor

Description of changes:

Merging from Upstream considering commits

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.

@samuel40791765 samuel40791765 changed the title Upstream merge 2024 06 03 update Upstream merge 2024 06 03 Jun 4, 2024
@samuel40791765 samuel40791765 force-pushed the upstream-merge-2024-06-03-update branch from ad76563 to 2b6a5d3 Compare June 4, 2024 20:17
@samuel40791765
Copy link
Contributor Author

#1620 needs to be merged in order for the tests to pass here.

@samuel40791765 samuel40791765 force-pushed the upstream-merge-2024-06-03-update branch from 2b6a5d3 to 6b5fba5 Compare June 4, 2024 22:23
@samuel40791765 samuel40791765 marked this pull request as ready for review June 4, 2024 22:23
@samuel40791765 samuel40791765 requested a review from a team as a code owner June 4, 2024 22:23
@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.61088% with 32 lines in your changes missing coverage. Please review.

Project coverage is 78.18%. Comparing base (bbfb3b2) to head (8ce36af).

Files Patch % Lines
crypto/x509/x509_vfy.c 68.75% 10 Missing ⚠️
crypto/x509/x509_test.cc 93.98% 8 Missing ⚠️
crypto/x509/x509_req.c 63.15% 7 Missing ⚠️
crypto/x509/x509_cmp.c 81.25% 3 Missing ⚠️
crypto/x509/x_pubkey.c 93.33% 2 Missing ⚠️
crypto/x509/t_req.c 0.00% 1 Missing ⚠️
crypto/x509/x509spki.c 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1621      +/-   ##
==========================================
+ Coverage   78.09%   78.18%   +0.08%     
==========================================
  Files         562      562              
  Lines       94666    94717      +51     
  Branches    13578    13578              
==========================================
+ Hits        73933    74050     +117     
+ Misses      20139    20073      -66     
  Partials      594      594

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

@samuel40791765 samuel40791765 force-pushed the upstream-merge-2024-06-03-update branch 2 times, most recently from 5d2a4e8 to 7a17a22 Compare June 5, 2024 17:37
@samuel40791765 samuel40791765 force-pushed the upstream-merge-2024-06-03-update branch 2 times, most recently from a9caca2 to 7f38813 Compare June 7, 2024 21:17
briansmith and others added 14 commits June 11, 2024 21:18
@briansmith @samuel40791765
chacha: Move ARMv8 OPENSSL_armcap_P dispatching from assembly to C.
001e90d 
Take a step towards removing all uses of OPENSSL_armcap_P from the
ARMv8 assembly code.

Change-Id: Ic1a75e107017b33f3e88b8eae503b788e37ca70a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64207
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit 5b6a9cf31d13d3b35a59fa674b566cfb553ef22d)
@davidben @samuel40791765
chacha: Move 32-bit Arm CPU dispatch from assembly to C
22d6f7a 
This also removes handling of the empty input, to match what was done
for aarch64. (The C code ensures the function is never called in this
case.)

Bug: 673
Change-Id: I7e868a9eb0b022c22c3f4ba2c8782ae1464c5a52
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64967
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit b3cda5cf4549b131b09e6fc67775717dcfeeae13)
@davidben @samuel40791765
Remove X509_STORE_set_get_issuer
884ad00 
This is unused. Removing it removes a codepath where callers may
inadvertently break internal invariants of the verifier.

It also removes an attractive nuisance: pyca/cryptograpy at one point
intended to use this callback for AIA fetching. They are lucky they
never did because that would have been a security bug. Certificates
returned by this callback are "trusted" which means, if they satisfy the
X509_TRUST criteria (e.g. are self-signed), they would become trust
anchors!

Also remove the getters for the callbacks, as no one is using them. Not
much good can be done by extracting callbacks. Either it is your
X509_STORE, in which case you know your own callbacks, or it is someone
else's, in which case it probably depends on some application-specific
state that you don't know about.

Update-Note: Removed a handful of unused functions.

Change-Id: Ic95db40186a9107e2a3f44028aa28a335653c25a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64987
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit b083a69e5b9a38fcbae710b0c2081de0235d509b)
@davidben @samuel40791765
Simplify some logic around X509_verify_cert callbacks
6a47d6b 
If we simply set ctx->get_crl to get_crl, as the other callbacks do, we
don't need to branch at the call site. Also get_issuer no longer needs
to be a callback. We can just check if X509_STORE_CTX_set0_trusted_stack
was called.

Change-Id: I42235f141ee9f8463631f56471c12324a8755bba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64988
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit d1831d78c867ba51b3992ccc213fd201d2f4b0f1)
@davidben @samuel40791765
Warn more explicitly not to use the callback in SSL_set_verify
60755fb 
The vast majority of implementations of this callback empirically did
not understand what this API was for, and actually wanted
SSL_CTX_set_cert_verify_callback.

Change-Id: If7961db612932ac9e76ed8482a59442685ece4b5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65007
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
(cherry picked from commit b6e0eba6e62333652290514e51b75b966b27b27c)
@davidben @samuel40791765
Do a better job testing expiration checks
abd4607 
Instead of staggering the ranges, we can just generate three variants of
each certificate. Also internal_verify is written in a remarkably goofy
way, so the partial chain case is a little interesting and worth
testing.

Change-Id: I94bfe58f54c5da2b88581af204c4e9d5b919f50b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65047
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit c0ae579dbbcd47ca60fd9539bf6cfc1bd0b434e9)
@davidben @samuel40791765
Fix X509_PUBKEY_set0_param to clear the cached EVP_PKEY
d0eea90 
Change-Id: I661c1284ba23138074339aec712ee6ba86905518
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65048
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit cc696073cffe7978d489297fbdeac4c0030384aa)
@davidben @samuel40791765
Test signature verification in X509_verify_cert
2854985 
Previously, if we just skipped signature checks, zero tests would fail.
This is perhaps not ideal.

Change-Id: Ife42f32d06c01b48afa9da26a8bd25814f9a909f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65049
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit 2e0b9e030e70563afe3ecd44ea9171ea3e6ce51c)
@davidben @samuel40791765
Eagerly compute the cached EVP_PKEY in X509_PUBKEY
0917e72 
Whenever the object is mutated, we can simply refresh the cached
EVP_PKEY. This aligns with OpenSSL, which computes it eagerly these
days. This removes the need to lock things, and also makes it easy to
implement the get0 versions of the functions from OpenSSL.

Change-Id: Ib17b654af694817edc43e4742d9baf9ed05c676e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65050
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit 5b3dc49c1271554f73b976c2c625600d6bd912b0)
@davidben @samuel40791765
Use X509_get0_pubkey to simplify things slightly
3320372 
Also X509_REQ_check_private_key didn't handle unknown key type case.
Clean those up and align with X509_check_private_key.

Change-Id: I7b16f85662943e4b226221a01e1092cf62afc643
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65051
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit c1c7f0929f18fabc74004242d3e4ce03f54511d0)
@davidben @samuel40791765
Make configure_callback in x509_test.cc take the X509_STORE_CTX
b63ccff 
No change to tests, but makes it easier to test X509_STORE_CTX-level
features.

Change-Id: I26a02ce51b4970aedce06f64d4ebef0dba154597
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65052
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit e40882e848fdc03e31f974bbad6825f6886a5a38)
@davidben @samuel40791765
Test the X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE codepath
7c20b96 
This seems to only be reachable via the verify callback, but it is
possible for internal_verify to see a single-element certificate that
isn't self-signed. There's a special error code for this. We probably
can safely change it, but cover the codepath in the meantime.

Change-Id: Id4c81e1826f0b43b369e8f00de36313e5fa4360d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65053
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit 35f5a321296977809cd89a49ec400310c2bba78b)
@samuel40791765
align X509_SIG_INFO initialization with OpenSSL
0647e1a
@samuel40791765
Update build files in generated-src
1997f31
@samuel40791765 samuel40791765 force-pushed the upstream-merge-2024-06-03-update branch from 7f38813 to 8ce36af Compare June 12, 2024 04:18
@samuel40791765 samuel40791765 merged commit 5f78ef3 into aws:main Jun 12, 2024
96 of 98 checks passed
@samuel40791765 samuel40791765 deleted the upstream-merge-2024-06-03-update branch June 12, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewhop andrewhop andrewhop approved these changes

@torben-hansen torben-hansen torben-hansen approved these changes

Assignees
No one assigned
Labels
reviewers-assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@samuel40791765 @codecov-commenter @andrewhop @torben-hansen @briansmith @davidben