BoringSSL Main Console
| Legend: | Passed | Failed | Running | Exception |
Update tools and add a .bazelversion
So that MODULE.bazel.lock is actually checked to be up-to-date, we
should run with --lockfile_mode=error on CI. But since the lock file
format changes by Bazel version, that means we also need to pick a
particular Bazel version.
This matches the best practices documented here:
https://bazel.build/external/lockfile#best-practices
To update libc++, we now need to pull in a bit of LLVM libc. See
https://discourse.llvm.org/t/rfc-project-hand-in-hand-llvm-libc-libc-code-sharing/77701
Change-Id: I53b28067bab86d34be8c76625d6bedc23f0baeba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73487
Commit-Queue: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Speed up sha512 on x86
Enable avx version on amd, since sha-ni sha512 doesn't exist
Change-Id: I2ea4b74c7995ccae6b673adefc4b3e25d515e321
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73567
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
crypto: switch to C++
This change switches nearly all of BoringSSL to use C++. The public
functions still use the C ABI, and so code written in C can still use
BoringSSL. Also the use of the C++ standard library is minimal and no
run-time requirement for it is intended.
Change-Id: I902d2f51a3c8d6bd0dc4aabe1b192a15d7b788e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72747
Commit-Queue: Adam Langley <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Remove the old SSL_R_APPLICATION_DATA_INSTEAD_OF_HANDSHAKE logic
Back when we were deploying TLS 1.3, we were tracking how often some
buggy middlebox interfered with the new TLS version and prevented sites
from upgrading.
One middlebox's failure mode was that they dropped ServerHello and
passed the remaining records through. This error code was caught by
Chromium to histogram. TLS 1.3 has long since been deployed, and we've
dropped that histogramming code, so remove this special case.
See https://crbug.com/41325349 for some of the history here.
Change-Id: I70c1ada1ed3d9ba73dfe75213a0233e407599c98
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73548
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Don't access the read_buffer directly in read_v2_client_hello
I missed a spot in https://boringssl-review.googlesource.com/21865. The
function is passed an input span and should just work with that span.
This does the exact same thing rigth now in == read_buffer.span() right
now. But we'll want this if we ever add a bytes-in/bytes-out API.
Change-Id: Ie12ad783b77b78335beba3a27da4d205ec828077
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73547
Commit-Queue: Bob Beck <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Don't refer to key_update_requested responses as ACKs
It's a bit confusing when DTLS 1.3 also needs to ACK the KeyUpdate
message itself. I was going to call it a "reply", but it turned out that
none of the instances really needed to refer to this.
Bug: 42290594
Change-Id: I1cb674755920a0d7567b4c8eed46f12a0294faa5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73307
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Test ACKing and reassembly of post-handshake messages
When it comes to receiving post-handshake, we're mostly already OK. Go
ahead and test that.
I say mostly because we probably should ACK as soon as we've gotten a
complete message. (Though we'd need to be careful not to ACK too many
times we've already got a bunch of messages queued up.) On the other
hand, outside of unit tests driving KeyUpdate, there isn't really a huge
issue to delaying the ACK and thus KeyUpdate by 100ms, so maybe it's not
that big of a deal.
Bug: 42290594
Change-Id: I86d1b81ddb5ebb98022b12659fafa9f8d4fd26d0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73147
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
runner: Move some test bugs into the callback
Now that we have this callback, we can make the general logic less
complex and shift it to the tests.
Change-Id: I2523bee122a16591fb32275f27a27a31d5a0b33c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73287
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Test that the DTLS 1.3 server retransmits NewSessionTicket correctly.
Testing NewSessionTicket is tricky. First, BoringSSL sends two tickets
in a row. These are conceptually separate flights, but we test them as
one flight. I've added a mechanism in the testing callbacks to merge two
flights together.
Second, these tickets are sent concurrently with the runner's first test
message. The shim's reply will come in before any retransmit challenges.
We need to correct for this in the callback.
Bug: 42290594
Change-Id: Id6887c435f7d1d406976377ca66081869e3f1f78
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73230
Commit-Queue: Nick Harper <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Fix OPENSSL_NO_ASM build for ARM
- Added missing preprocessor checks in `cpu_arm_freebsd.c` and `cpu_arm_linux.c`
to handle `OPENSSL_NO_ASM`.
Prevents build failures on ARM platforms (FreeBSD/Linux) when `OPENSSL_NO_ASM` is set.
Change-Id: I27f70d0edc69350b2b12c1829287025eb2f38f9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73447
Commit-Queue: Bob Beck <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Document expected operations on libssl custom BIOs
The requirement to support BIO_flush is not obvious and should be
written down.
Change-Id: Ib046898c9a49a6ffe51ee3c32982db3854095db5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73467
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Gate -fno-rtti on BORINGSSL_ALLOW_CXX_RUNTIME
We used to have this toggle to control whether we passed -fno-rtti to
libssl, when libssl was not allowed to take a C++ runtime dependency.
This toggle is used by OSS-Fuzz because -fsanitize=vptr is incompatible
with -fno-rtti. Now that we're back to passing -fno-rtti, but this time
to libcrypto instead, restore this opt-out to unbreak OSS-Fuzz builds.
Bug: 377971745
Change-Id: I6058efdb020cbb5481d3d25551dbb2e986af8ccc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73387
Reviewed-by: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Tweak comment in thread.h slightly
Someone read "is thread-safe by default" to mean arbitrary operations
could be run concurrently, when it just means we don't need caller help
to provide the concurrency guarantees that we target.
Change-Id: Ic0500c7f36a3eba2f010505d428570d72ceeb0ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73407
Reviewed-by: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
Build with -Wextra-semi in Clang
This would have caught
https://boringssl-review.googlesource.com/c/boringssl/+/73029
The other instances didn't break because Chromium builds our fuzzers
(and thus our headers) with stricter warnings than our source files.
Probably worth being consistent there, but meanwhile go ahead and turn
on this warning.
Change-Id: I0cff2648f25a0d7f87de6a27f4af6ea891fff663
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73267
Commit-Queue: Bob Beck <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
runner: Move receivedFlightRecords tracking up a layer
If the test runner discovers a handshake record by trying to read
application data, it loses track of that record and never ACK it.
This fixes one of the many issues in the way of testing post-handshake
message retransmit.
Bug: 42290594
Change-Id: Ic74a91f640484aa3ca71b52a05b8aaa072480f36
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73229
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Remove outdated DTLS 1.3 test suppression
We broadly can receive NST now. Just needed to make sure the tests
actually flushed them.
Bug: 42290594
Change-Id: I7ab41b6af0e19eb548c52593cc99e787fedbdd41
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73128
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
runner: Remove unused skipPacket method
This was a remnant of the old retransmit tests.
Change-Id: Ia98b0eaa407c484e671f08b97f648c7b2a9efefd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73228
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Remove stale DTLS 1.3 TODO
I should have removed this with
https://boringssl-review.googlesource.com/c/boringssl/+/72952
Bug: 42290594
Change-Id: I77e58f3a5238881de33991893b36b2a7fa152e37
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73309
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Schedule ACKs when we receive a partial flight
This implements a very simple ACK policy: for every ACKable record (i.e.
contains no far-future fragments that we ignore), add it to a queue of
record numbers to ACK and set an ACK timer at 1/4 of the current
retransmission timeout.
RFC 9147 doesn't say a whole lot, but this is arguably slightly
different from the recommended policy. RFC 9147 has some text with
implies you're only meant to ACK the current flight and not arbitrarily
old message fragments. However, tracking that in the fully general case with
post-handshake messages is unclear. There's no harm in ACKing those
packets, so start with this. See discussion in
https://mailarchive.ietf.org/arch/msg/tls/kjJnquJOVaWxu5hUCmNzB35eqY0/
Something kind of fun is that this provision in the spec happens for
free:
> ACKs SHOULD NOT be sent for these flights unless the responding flight
> cannot be generated immediately. All other flights MUST be ACKed. In
> this case, implementations MAY send explicit ACKs for the complete
> received flight even though it will eventually also be implicitly
> acknowledged through the responding flight. A notable example for this
> is the case of client authentication in constrained environments,
> where generating the CertificateVerify message can take considerable
> time on the client.
If we generate the next flight before the ACK timer, it will be canceled
and we don't ACK. If the next flight is async and takes too long, the
ACK timer will win and we tell the peer not to retransmit.
Bug: 42290594
Change-Id: I7974499f82ce2b2c7da91f02ca65886b0f82896c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72952
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Introduce a DTLSTimer abstraction
We'll need to carry a couple of timers with DTLS 1.3. Abstract this into
a DTLSTimer.
Bug: 42290594
Change-Id: I4d57dfae9c5984cb10f5db251642af5aaec9a495
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72951
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Check for QUIC in SSL_process_quic_post_handshake
This function doesn't make sense for non-QUIC SSLs.
Change-Id: I3fc08a01f45d8c4e36e449675aecc98ce296abe1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73127
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
runner: Remove outdated FragmentClientVersion logic
A long time ago, OpenSSL negotiated versions by sniffing at the first
few bytes of the ClientHello. It couldn't handle ClientHellos that were
so fragmented that the version field was unreadable.
When it encountered a ClientHello it couldn't handle, it silently
assumed TLS 1.0, which led to CVE-2014-3511. The original fix for that
made this case an error instead. But this meant that
MaxHandshakeRecordLength had to take care never to trigger this error
case in other tests, otherwise it wouldn't exercise what we were trying
to exercise. So we addeed some funny logic in
https://boringssl-review.googlesource.com/1452.
Fast forward many years and BoringSSL no longer negotiates versions this
way. We read the whole ClientHello and then act on it. Now fragmenting
the first few bytes of the ClientHello behaves the same as any other,
and we no longer need to special case it in tests.
Change-Id: Id098f1e2066626661113ca4796250feb6cea421b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73247
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Test DTLSv1_get_timeout behavior
The next CL will rework it a bit, so add some tests for it.
Bug: 42290594
Change-Id: Ib2dc3068c446a22d27f87c238275ca740932b3ac
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72950
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
runner: Add a helper to read and downcast a message
We can't always use this because sometimes (particular in TLS 1.2), you
have to account for optional messages, but often we know exactly which
message we're expecting.
Change-Id: I4f6f59111fbf3e5f8a8fefa35802def9b2029196
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72949
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
runner: Rearrange 0-RTT code in server slightly
The point at which we flush is tricky for testing DTLS 1.3 ACKs. This
rearrangement is not sufficient to make that work, but I wanted to pull
this into a separately reviewable CL first. The changes are:
- We can derive keys and set the out keys very early
- I've removed the shouldSkipEarlyData() check. That check is
unnecessary because we're already checking for whether the server
accepted early data in EncryptedExtensions. (Also it doesn't make
sense to apply that check to reading a bit of early data, but not to
reading EndOfEarlyData. The conditions on those should match.)
Change-Id: Ie1909bb5f8a8a2aeab05ba0f95155ce45eb160f3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72948
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Return OPENSSL_timeval by value
Also resolve an old TODO.
Change-Id: I67e531d547e2a1f40a7ab547d7211ebbec28102d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72908
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Call the message callback for DTLS 1.3 ACKs
Bug: 42290594
Change-Id: Iafdd7ec447ce5bad703993b7a5d2a6c1d9654248
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72907
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Better track final vs early versions
https://boringssl-review.googlesource.com/c/boringssl/+/71534 generally
made things cleaner, but lost a subtlety: when we check if the version
is set, sometimes we care about whether it is an early version or final
version. This includes the two minor behavior changes listed there, and
a host of TODOs we kept adding to the DTLS 1.3 implementation.
We don't yet implement DTLS 1.3 0-RTT, but let's just resolve these
TODOs now rather than constantly having to make a note of this.
Bug: 42290594
Change-Id: I74e1367836762a6e6fd8d216158ee5309d387cce
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72887
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Update the default retransmit timeout for DTLS
While DTLS 1.2 recommended 1 second, it's 2024 and RTTs are generally
much lower. I believe most of our important uses already reconfigure
this, but let's default to something better.
Update-Note: The default DTLS timer is now slightly lower.
Bug: 42290594
Change-Id: Iec3f01395ac0c3c03cdfd951cc14acddb40ce72f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72868
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: Nick Harper <[email protected]>
runner: Remove unused SendHalfRTTData option
I forget why we added this. It looks like it's never actually been used.
Half-RTT vs 1-RTT data is not distinguishable from the client, so I
suspect we never actually need to simulate it in the test runner.
Change-Id: I851e2caf417d00da654ee530a240ce54af1415d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72888
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Send one ACK immediately after the handshake in DTLS 1.3 servers
This does not yet implement sending ACKs in general, just the one
immediate ACK when the handshake completes. The general case will
require scheduling an ACK-send timer, but this one can be sent
immediately.
One interesting case to test is when the server would like to ACK
Finished, but cannot because the records were merged with fragments that
we had to discard.
Bug: 42290594
Change-Id: I64b3f8ecbef4ffee68d923f83ea89d2349847f8b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72867
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
clang-format all of ssl
A mechanical clang-format -i followed by a fixup of the
affected if statements to include // on the end to preserve
the formatting.
Change-Id: If58fac0e6a2333c7fd1bbece69750dedf944a2f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73107
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
Auto-Submit: Bob Beck <[email protected]>
Include <memory> in mldsa.cc
This file uses `std::unique_ptr` and so should include the header for
it.
Change-Id: Iff962a2bad5f5bb98d0fc2049f47573c85a22643
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72987
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
Support skipping tickets in both ticket callbacks
TLS 1.2 and TLS 1.3 both support some mechanism for the server to change
its mind at the last minute and not send a ticket. In TLS 1.2, you leave
the ticket field empty. In TLS 1.3, you just don't send NewSessionTicket
in the first place.
We have two ticket encryption callbacks, the old "ticket key callback"
and the "ticket AEAD callback".
The ticket key callback has no way to trigger this behavior, but it has
an unused zero return that could be repurposed for this, in principle.
Back in 2016, OpenSSL did so in
https://github.com/openssl/openssl/pull/1098
The ticket AEAD callback could, in principle, support this by
successfully returning the empty string. This goes against the current
documented API contract, but it accidentally worked in TLS 1.2. In TLS
1.3, it caused our servers to send a syntax error.
The QUIC folks want this behavior and used to accidentally trigger the
syntax error, and have switched to returning some garbage placeholder
string. Between that, the new OpenSSL behavior, and it accidentally
working in TLS 1.2 anyway, let's just go ahead and support this.
To aid in version skew messes, this also bumps BORINGSSL_API_VERSION.
Change-Id: Id4fef7b9aa552dc8e927f89a5d746bdd2247e1c6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73028
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Exercise SSL_TICKET_AEAD_METHOD in runner
We never actually tested it in runner, though I think we did write
ssl_test.cc tests. Adding some tests now to prepare for adding and
testing some new behavior shortly.
Also fix the documentation for SSL_CTX_set_tlsext_ticket_key_cb
returning zero in the encrypt case.
Change-Id: Ic7b8d51b2433d56cc524f62565946b906d515257
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73027
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Disable sdallocx detection by default
The comment about it only detecting statically linked symbols is, as far
as I can tell, not actually true? It gets picked up when you LD_PRELOAD
libjemalloc.
This means that if you have a binary where some random dependency pulls
in libjemalloc, but you actually use a different malloc implementation
without sdallocx, things break. We've also had an issue with
https://github.com/grpc/grpc/issues/25450, though that one was a bit
more obviously a problem with their build.
Given this mess, and sdallocx not quite being standard, probably we
should just leave this opt-in. Maybe decades from now, everyone will
standardize on C23's free_sized. Or maybe we'll just be Rust by then.
Update-Note: sdallocx detection can be restored by building with
BORINGSSL_DETECT_SDALLOCX.
Fixed: 378077860
Change-Id: I7c6bd718a154f31abec09623fddbdd0637380b9a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73030
Reviewed-by: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Correctly retransmit the final flight in DTLS 1.3
This turned out to be very easy: just leave the timer on. Note this does
mean we can't interop with ourselves very well, because we don't yet
send the ACK that we now expect.
Bug: 42290594
Change-Id: I804c206098dc075e3bf7ca3510bf721495576bd7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72849
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Track SSL session types a bit better on the client
A session could be offered in one of three fields:
- The TLS 1.2 session ID
- The TLS 1.2 session ticket extension
- The TLS 1.3 PSK extension
We didn't quite keep track of which kind we had. In particular:
- We are not willing to send TLS 1.2 session tickets if SSL_OP_NO_TICKET
is set. However, if we were configured with a ticket session AND
enabled TLS 1.3, we'd send a non-empty session ID. If the server
echo'd the session ID anyway, we'd get confused and think the session
was being resumed. There's no real practical consequence to this, but
we should reject this.
- If we somehow constructed a TLS 1.3 session with ID but no ticket, we
would think it was an ID session and offer the session ID after the
cleanup in
https://boringssl-review.googlesource.com/c/boringssl/+/69947. We'd
also send a PSK extension with an empty PSK field, and then even allow
the server to resume it. This isn't completely absurd (except that PSK
identities cannot be empty), but offering the session ID would trip
QUIC up.
This case should be impossible... but before the bug fixed in
I1651e7887f9611ebc44ac54af89c85bf86a9feff, this was actually
reachable. There's no practical consequence, but we should reject this
at a better place.
- The code to decide whether the server could send pre_shared_key in
ServerHello just checked for any session at all, even a TLS 1.2
session. This has no practical consequence because we'll just catch it
later, but may as well fix this.
Fix this by adding a function to classify the SSL_SESSION and then catch
on that throughout.
Change-Id: I26a721b7c473d08525217e4ab1d0d341d651dfcb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73008
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Don't pack fragments as efficiently for plaintext records
This partially reverts
https://boringssl-review.googlesource.com/c/boringssl/+/72275
The more efficiently packing broke a project that talks to a server that
does not handle this correctly. See b/378742138. For now, just match
what we do in TLS and only pack encrypted records.
Since DTLS 1.2 only has multi-message flights in the plaintext epoch and
DTLS 1.3 only has them in encrypted epochs, this effectively limits the
optimization for DTLS 1.3. Later we'll want to add a toggle and roll
this out across the board, but for now just partially revert the
behavior.
Bug: 374991962
Change-Id: I4c11efcd06dc22a7866e096c5b7a5e379da281c4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73067
Commit-Queue: Nick Harper <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reject NewSessionTicket messages with empty tickets in TLS 1.3
In TLS 1.2, the ticket field could be empty to indicate the server
changed its mind on sending a ticket, having previously committed to
sending a NewSessionTicket message a round-trip ago.
In TLS 1.3, the server does not commit to sending NewSessionTicket. It
can always just not send it. So the ticket field is required to be
non-empty.
It's important we enforce this on the client because otherwise we
produce a mixed up SSL_SESSION object that looks like an ID session
(thanks to the placeholder ID field that was added for a still unfixed
Envoy bug, see b/200292207). That, in turn, confuses some code in
assembling the next ClientHello.
The subsequent CL will tighten that up.
Update-Note: BoringSSL TLS 1.3 clients will now correctly reject
zero-length tickets, rather than letting servers get us into a slightly
funny state.
Change-Id: I1651e7887f9611ebc44ac54af89c85bf86a9feff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73007
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Remove stray semicolon
Change-Id: I29d2cbf04becceae8725191fce487743c74a2c3b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73029
Commit-Queue: Adam Langley <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Update the timer API for DTLS 1.3
Update-Note: We used to say you only need to drive the timer when
SSL_do_handshake asked for SSL_ERROR_WANT_READ. Change to just say you
are always obligated to drive the timer. Callers will need to adapt to
this before enabling DTLS 1.3.
Bug: 42290594
Change-Id: Iaf05eb608d3f4925cfafe243ae590e29e6526ff7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72848
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
runner: ACK flights in DTLS 1.3 by default
Tests can override this behavior if they want. This required fixing up
some logic around tracking lastRecordInFlight. We implicitly assumed
that, by the time we're ready to write, there's nothing more to read in
the current record. But BoringSSL currently sends a single record with
two NewSessionTickets in it, even though they're nominally two flights.
Instead, only wipe the state if the packet is empty. There's probably a
better way to process this, but this will do.
Bug: 42290594
Change-Id: Ib22d575777eb6866dbc02b9ba3b74e8d61a74b6c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72847
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Handle implicit ACKs in DTLS 1.3
Receiving part of the next flight should implicitly ACK the previous
flight, in most cases. The final flight, and post-handshake messages are
tricky, but we'll handle and test those in later CLs.
Bug: 42290594
Change-Id: Ia47551a103c7ca224c305892f36e4a270c14a396
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72827
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nick Harper <[email protected]>
Use DTLS 1.3 ACKs to avoid retransmitting ACKed fragments
This implements the first part of ACK processing: track which parts of
each outgoing message have been ACKed, update when receiving an ACK, and
use it to reduce retransmits. To do this, we also need to track the last
handful of records we sent, and use that to correlate ACKs with packets.
Test this by extending the new retransmit fragment to manage ACKs. The
callback gets told record numbers, along with what message segments they
cover, and may choose to ACK them. If it does, the ReadRetransmit
expectations will be automatically updated.
For now, I've made no attempt to test or handle post-handshake messages.
That has a lot of subtle assumptions around there not being multiple
concurrent transactions, so I think we'll tackle this later.
This also does not handle:
- Triggering retransmits when we receive partial ACKs.
- Implicitly ACKing flights when we receive any part of the next flight.
- Any kind of sending ACKs.
Bug: 42290594
Change-Id: I9e81a7d5c8838d4d31fe828e9fd9871631fe38ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72387
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
SLH-DSA: support SHA-384 as the prehash function instead.
Change-Id: I931e660d6ad8e6fae0e17b414a40bb0a0ea7e9c3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72967
Reviewed-by: Bob Beck <[email protected]>
Auto-Submit: Adam Langley <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
Test that DTLS retransmit can react to MTU changes
This works, but we never tested it. It will make DTLS 1.3 tests more
interesting, because ACKing older vs newer records will result in
different byte boundaries.
Bug: 42290594
Change-Id: Id7d5b98620373362ca9e500cda73cbb120cccc6c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72787
Reviewed-by: Nick Harper <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Redo DTLS retransmit tests
Previously, we tested DTLS retransmit by hooking *before* processing a
flight and bracketing one flight's worth of packets and then dropping
all of them, with minimal processing. We'd then repeat this a few times
and, once we were satisfied, actually process the next flight. This has
a few drawbacks:
1. We cannot interleave timeouts with chunks of the next flight.
Retransmit of the peer's flight, in both 1.2 and 1.3, interacts with
the peer receiving all or parts of the next flight, and in different
ways.
2. We cannot easily, in 1.3, ACK part of a flight and then wait for the
shim to retransmit the other part.
The new design instead hooks *after* we construct the reply to the
peer's flight under test. We buffer up all the data to be sent and then
call into a test-supplied callback that can control the exact order of
timeouts, etc. See the comment on DTLSController for more details.
As part of this, ChangeCipherSpec is now implicitly ordered with
ReorderHandshakeFragments, so there is no more need to carry
ReorderChangeCipherSpec separately.
Bug: 42290594
Change-Id: I754946dbb5591188aa2d32b694f213269836b266
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72708
Reviewed-by: Nick Harper <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
runner: don't assume BlockMod implements SetIV
It's an undocumented implementation detail, inherited from crypto/tls.
We are probably dropping it from the NewCBC* implementations (if it
doesn't break too many applications).
Change-Id: I2e27bc166bd4c75c449bde6980c68772d9a9191a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72947
Auto-Submit: Filippo Valsorda <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
Remove redundant tests.
Change-Id: I23c44ae5896eb9f96bbfba1af0374b2a9aaeb2e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72728
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>