chore: sync BoringSSL upstream - #4
Merged
Merged
Conversation
Previously, invalid values of e made the function loop forever. Change-Id: I3d03e4b096f988ebb01b378e52d7dab66a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95548 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Rudolf Polzer <rpolzer@google.com>
Change-Id: I2301ea3ce75292bcd4f0e943c916973fb543fdc3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95567 Commit-Queue: Rudolf Polzer <rpolzer@google.com> Reviewed-by: Rudolf Polzer <rpolzer@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Adam Langley <agl@google.com>
Before, these functions would unconditionally copy the src's poison flag to the dst, potentially resulting in an unpoisoned context even if the reason for poisoning (e.g. a field with invalid value) remains. After this change, copying always fails if any of the two params is poisoned. The most relevant code path for this is when `X509_STORE_CTX_init` is called with a poisoned `X509_STORE`; it happily copied the poisoned context and then cleared the poison flag while inheriting from defaults. Yes, instead the calls in `X509_STORE_CTX_init` could be reversed to first `inherit` from the defaults and then to `set1` from the user provided context, which then would yield the correct copied poison flag; however it seems prudent to instead harden the public APIs, as there could be more issues of this kind. Not considering a vulnerability as the poison flag can only ever be set on a call to us if a caller used an API wrong by ignoring its return value. Of course, the whole purpose of the flag is to detect and fail such callers so no damage happens. Change-Id: Iaed97dfa21863c882a0d46b1b8439ec06a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95527 Commit-Queue: Rudolf Polzer <rpolzer@google.com> Reviewed-by: Adam Langley <agl@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
Instead of UB, this now returns an error. This makes sense, given these can both sign and decrypt, but there are conceivable applications where only one is necessary. Change-Id: Idec6c00d17594aa604170b978370be396a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95607 Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Commit-Queue: Xiangfei Ding <xfding@google.com> Reviewed-by: Xiangfei Ding <xfding@google.com>
Otherwise, trying to read from an uninitialized BIO will underflow due to -2 being used as return value to indicate that. Note that the only way to trigger this function is using the public BIO_read_asn1 API; however, it does a bio_read_full first, which makes it hard to actually have this condition happen. The BIO would basically have to succeed at first, and suddenly return -2 later. None of the built-in BIOs in BoringSSL can do that. Change-Id: I38777688490ea6cc2d9af23def2beadd6a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95628 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Rudolf Polzer <rpolzer@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
Not considering a vulnerability as any attacker who can do this can just as well inject ICMP Destination Unreachable or even just UDP flood the client. Having said that, definitely an interesting bug. Change-Id: I7608b38a26abcb41ffccaab453990c066a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95587 Commit-Queue: Rudolf Polzer <rpolzer@google.com> Reviewed-by: Adam Langley <agl@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change-Id: I5f0b162ca9c2bd4a718b4e933499f15509e420e2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95727 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Emily Stark <estark@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>
The latest MTC draft uses uint48 values. Change-Id: Ide5e405de82e1bdd6b6229d8f959973348bbc360 Bug: 452983502 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95747 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Matt Mueller <mattm@google.com>
There was a problem hiding this comment.
Pull request overview
Syncs this fork with upstream BoringSSL main (8 commits), bringing in a handful of correctness and robustness fixes across DTLS, RSA, X509 verification parameters, BIO parsing, and bytestring helpers.
Changes:
- DTLS: fix fragment-window boundary handling and add regression tests in the Go runner.
- SSL private key methods: gracefully fail when
SSL_PRIVATE_KEY_METHODhas NULL hooks; add a regression test. - Add
CBS_get_u48, tighten RSA keygen exponent validation, hardenBIO_read_asn1negative-read handling, bumpBORINGSSL_API_VERSION, and update related tests/metadata.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ssl/test/runner/runner.go | Registers the new DTLS fragment-window regression tests. |
| ssl/test/runner/dtls_tests.go | Adds DTLS tests ensuring out-of-window fragments at the boundary are discarded. |
| ssl/ssl_test.cc | Adds a regression test for NULL decrypt in SSL_PRIVATE_KEY_METHOD during RSA key exchange. |
| ssl/ssl_privkey.cc | Adds NULL-hook checks for custom private key methods (sign/decrypt/complete) to fail cleanly. |
| ssl/d1_both.cc | Adjusts DTLS fragment window comparison to discard the upper-bound sequence (off-by-one fix). |
| include/openssl/prefix_symbols.h | Adds symbol prefixing for the new CBS_get_u48 API. |
| include/openssl/bytestring.h | Declares new public API CBS_get_u48. |
| include/openssl/base.h | Bumps BORINGSSL_API_VERSION from 40 to 41. |
| crypto/x509/x509_vpm.cc | Refuses to merge/inherit poisoned X509_VERIFY_PARAM objects (and renames helper to reflect merge semantics). |
| crypto/x509/x509_test.cc | Adds tests ensuring inherit/set1 fail when either parameter object is poisoned. |
| crypto/rsa/rsa_test.cc | Adds tests validating RSA keygen rejects invalid public exponents. |
| crypto/fipsmodule/rsa/rsa_impl.cc.inc | Rejects invalid e values early in RSA keygen (negative/even/one), improving behavior and error codes. |
| crypto/bytestring/cbs.cc | Implements CBS_get_u48 via existing cbs_get_u. |
| crypto/bytestring/bytestring_test.cc | Extends integer parsing tests to cover CBS_get_u48 and adjusts expected reads. |
| crypto/bio/bio.cc | Treats any negative BIO_read result as an error in bio_read_all. |
| crypto/bio/bio_test.cc | Adds a test exercising negative read values other than -1. |
| .vac/boringssl-upstream.json | Updates upstream SHA tracking and sync timestamp for this fork. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1629
to
+1634
| if msg.Sequence == 0 && !msg.IsChangeCipherSpec { | ||
| // Injected future fragment with offset +7. | ||
| // Since SSL_MAX_HANDSHAKE_FLIGHT is 7, sequence + 7 is at the upper limit. | ||
| // This must be silently discarded by the shim. | ||
| shouldDiscard := DTLSFragment{ | ||
| Epoch: msg.Epoch, |
| WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { | ||
| for _, msg := range next { | ||
| if msg.Sequence == 0 && !msg.IsChangeCipherSpec { | ||
| // Injected future fragment with offset +7. |
This file contains hidden or 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
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.
Summary
Syncs
vacp2p/boringsslwith BoringSSL upstreammain.Security Review
https://boringssl.googlesource.com/boringsslrefs/heads/mainbeddb582d9e8786a07d79f1cf054d4792b3dd81f956bac6e4db9b33789dcedb5ea3f28e51030cead956bac6e4db9b33789dcedb5ea3f28e51030cead.vac/patches/fiat-p256-windows.patchreplayed successfully against the new upstream SHACommits Touching Monitored Crypto Paths
956bac6e4 add CBS_get_u48e2da59b2d Bump BORINGSSL_API_VERSION5ee9407bc Fix off-by-one allowing an unauthenticated handshake abort.e0d763c0a bio_read_all: bail out on every error.2d8ef80d4 RSA: handle gracefully when a SSL_PRIVATE_KEY_METHOD has NULL methods.c2c9f2f3e X509_VERIFY_PARAM_inherit/_set1: refuse if either params are poisoned.3fff7111b RSA_generate_key_ex: tighten up code a bit.28d501c20 RSA_generate_key_ex: reject invalid values of e.Commits Missing Expected BoringSSL Review Metadata
No commits in the upstream range were missing the expected review metadata.
Upstream Workflow Changes Not Imported
No upstream workflow changes were skipped.
Manual Review Checklist
crypto/,ssl/,include/openssl/, andthird_party/fiat/.fiat_p256patch is still required and correctly applied.