Also generate a corpus to unblock the Chromium roll. The build tools
expect there to be a corresponding directory somewhere.
Bug: 275
Change-Id: I7a061ba6625ec57c10b0ae17e68b6b0159c539d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46826
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
'corpora' seems to be more common than 'corpuses' in Chromium code
search, including in libFuzzer's source itself.
Change-Id: I6489b57a4608f47274c4400aac135cbfb991953a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46825
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The gclient-managed files usually go in .gitignore. I think without it,
we have to redownload it all the time on the bots? Though this also
makes my git status cleaner.
Change-Id: Ic9bac6796bd3dcdea5039bcac39e853d9f12906b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46824
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See also 86a90dc749af91f8a7b8da6628c9ffca2bae3009 from upstream. This
differs from upstream's which treats {NULL, 2} as a valid way to spell
the empty list. (I think this is a mistake and have asked them about
it.)
Upstream's CL also, for them, newly makes the empty list disable ALPN,
when previously they'd disable it but misread it as a malloc failure.
For us, we'd already fixed the misreading due to our switch to
bssl::Array and bssl::Span, but the documentation was odd. This CL
preserves that behavior, but updates the documentation and writes a
test.
Update-Note: SSL_CTX_set_alpn_protos and SSL_set_alpn_protos will now
reject invalud inputs. Previously, they would accept them, but silently
send an invalid ALPN extension which the server would almost certainly
error on.
Change-Id: Id5830b2d8c3a5cee4712878fe92ee350c4914367
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46804
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This also fixes a minor bug (that doesn't matter because we don't
implement DTLS 1.3). init_message must be paired with finish_message to
correctly handle the DTLS header.
Change-Id: I4b65c82d4b691d5b77d9e20513983145098d6f8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46785
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We currently determine whether we need HelloRetryRequest at the same
time as resolving key share machinery. That is a little too late for
early data negotiation, so we end up accepting early data and then
clearing it later on in the function. This works but is easy to mess up,
given the preceding CL. There's also some ALPS logic that got this
wrong, but I believe it didn't result in any incorrect behavior.
Instead, this pulls secret computation out of the key_share helper
function, which now just finds the matching key share. We then check
early whether we need HRR, before deciding on early data.
Change-Id: I108865da08addfefed4a7db73c60e11cf4335093
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46765
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 servers should only skip early data if the client offered it.
Our HRR codepath didn't quite get this right. This CL is the minimal fix
for this issue, but I think we should rearrange this logic slightly
rather than deciding to do 0-RTT and then changing our mind. The next CL
will do that.
(This bug does not have any interoperability consequences. When
configured to skip early data, we're happy to vacuously skip over zero
early data records. We were just less strict than we should be.)
Change-Id: Ida42134b92b4df708b2bb959c536580bec454165
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46764
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: Ib2356f1a6e6ef8bfd5b5469eae9d1bc43dd40895
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46724
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Rather than computing kVarianceBlocks, which is hard to reason about,
use a sha1_final_with_secret_suffix abstraction. This lets us separate
reasoning in bytes about the minimum and maximum values of |data_size|
and the interaction with HMAC, separately from the core constant-time
SHA-1 update.
It's also faster. I'm guessing it's the more accurate block counts.
Before:
Did 866000 AES-128-CBC-SHA1 (16 bytes) open operations in 2000697us (6.9 MB/sec)
Did 616000 AES-128-CBC-SHA1 (256 bytes) open operations in 2001403us (78.8 MB/sec)
Did 432000 AES-128-CBC-SHA1 (1350 bytes) open operations in 2003898us (291.0 MB/sec)
Did 148000 AES-128-CBC-SHA1 (8192 bytes) open operations in 2006042us (604.4 MB/sec)
Did 83000 AES-128-CBC-SHA1 (16384 bytes) open operations in 2010885us (676.3 MB/sec)
After:
Did 2089000 AES-128-CBC-SHA1 (16 bytes) open operations in 2000049us (16.7 MB/sec) [+141.3%]
Did 851000 AES-128-CBC-SHA1 (256 bytes) open operations in 2000034us (108.9 MB/sec) [+38.2%]
Did 553000 AES-128-CBC-SHA1 (1350 bytes) open operations in 2002169us (372.9 MB/sec) [+28.1%]
Did 178000 AES-128-CBC-SHA1 (8192 bytes) open operations in 2008596us (726.0 MB/sec) [+20.1%]
Did 98000 AES-128-CBC-SHA1 (16384 bytes) open operations in 2001509us (802.2 MB/sec) [+18.6%]
Confirmed with valgrind tooling that this is still constant-time. In
doing so, I ran into a new nuisance with GCC. In loops where we run
constant_time_lt with a counter value, GCC sometimes offsets the loop
counter by the secret. It cancels it out before dereferencing memory,
etc., but valgrind does not know that x + uninit - uninit = x and gets
upset. I've worked around this with a barrier for now.
Change-Id: Ieff8d2cad1b56c07999002e67ce4e6d6aa59e0d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46686
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This CL adds an initial implementation of the ECH server, with pieces of
the client in BoGo as necessary for testing. In particular, the server
supports ClientHelloInner compression with ech_outer_extensions. When
ECH decryption fails, it can send retry_configs back to the client.
This server passes the "ech-accept" and "ech-reject" test cases in
tls-interop-runner[0] when tested against both the cloudflare-go and nss
clients. For reproducibility, I started with the main branch at commit
707604c262d8bcf3e944ed1d5a675077304732ce and updated the endpoint's
script to pass the server's ECHConfig and private key to the boringssl
tool.
Follow-up CLs will update HPKE to the latest draft and catch us up to
draft-10.
[0]: https://github.com/xvzcf/tls-interop-runner
Bug: 275
Change-Id: I49be35af46d1fd5dd9c62252f07d0bae179381ab
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45285
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We currently construct finishedHash fairly late, after we've resolved
HelloRetryRequest. As a result, we need to defer some of the transcript
operations across a large chunk of code.
This is a remnant of earlier iterations of TLS 1.3, when
HelloRetryRequest didn't tell us the cipher suite yet. Now the cipher
suite is known earlier and we can construct the finishedHash object
immediately. In doing so, move HRR handling inside doTLS13Handshake().
This keeps more of TLS 1.3 bits together and allows us to maintain the
HRR bits of the handshake closer to the rest of HRR processing. This
will be useful for ECH which complicates this part of the process with
an inner and outer ClientHello. Finally, this adds a missing check that
the HRR and SH cipher suites match.
Change-Id: Iec149eb5c648973325b190f8a0622c9196bf3a29
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46630
Reviewed-by: Adam Langley <agl@google.com>
This removes the now unnecessary virtual calls. Benchmark differences are
mostly positive but probably noise.
Before:
Did 839000 AES-128-CBC-SHA1 (16 bytes) open operations in 2000497us (6.7 MB/sec)
Did 623000 AES-128-CBC-SHA1 (256 bytes) open operations in 2000409us (79.7 MB/sec)
Did 434000 AES-128-CBC-SHA1 (1350 bytes) open operations in 2002909us (292.5 MB/sec)
Did 146000 AES-128-CBC-SHA1 (8192 bytes) open operations in 2000785us (597.8 MB/sec)
Did 82000 AES-128-CBC-SHA1 (16384 bytes) open operations in 2014268us (667.0 MB/sec)
After:
Did 866000 AES-128-CBC-SHA1 (16 bytes) open operations in 2000697us (6.9 MB/sec) [+3.2%]
Did 616000 AES-128-CBC-SHA1 (256 bytes) open operations in 2001403us (78.8 MB/sec) [-1.2%]
Did 432000 AES-128-CBC-SHA1 (1350 bytes) open operations in 2003898us (291.0 MB/sec) [-0.5%]
Did 148000 AES-128-CBC-SHA1 (8192 bytes) open operations in 2006042us (604.4 MB/sec) [+1.1%]
Did 83000 AES-128-CBC-SHA1 (16384 bytes) open operations in 2010885us (676.3 MB/sec) [+1.4%]
Change-Id: I735e99296ca9a1771518c622b8e7e6979a0d30bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46685
Reviewed-by: Adam Langley <agl@google.com>
The challenge field, at least per our implementation and OpenSSL, may be
either left-padded or truncated to form the ClientHello random. Test
both cases, as well as an exact match.
Change-Id: Icdedf899ef483225d8ed20580ad15818b5e52e91
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46631
Reviewed-by: Adam Langley <agl@google.com>
The client handshake currently defers creating the finishedHash and
writing things into the transcript, which is a little annoying for ECH.
In preparation for simplifying that, one nuisance is that we retain both
hello and helloBytes, across a long span of code. helloBytes is *almost*
the same as hello.marshal() except:
- When we send a V2ClientHello, helloBytes records that we serialized
the ClientHello completely differently.
- For the JDK11 workaround tests, helloBytes records that we swapped out
the ClientHello entirely.
- By the time we finally write helloBytes into the transcript, hello may
have been updated to the second ClientHello.
This CL resolves the first two issues. It replaces the v2ClientHelloMsg
with an option when serializing the clientHelloMsg, and it has the
ClientHello replacement function return a clientHelloMsg instead of a
[]byte. (This is a little weird because we're conflating parsed and
constructed ClientHellos, but ah well.)
A follow-up CL will remove the differed transcript bits and we'll
actually be able to drop helloBytes.
Change-Id: Ib82ac216604e2c4bf421277e57aa5fd3b4cef161
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46629
Reviewed-by: Adam Langley <agl@google.com>
Re-encoding a message does not necessarily give back the same value.
Bug: 275
Change-Id: I52cddd6152445b70579cbe03525898383bee211d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46644
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Dan McArdle <dmcardle@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Matching the Go standard library cipher.AEAD interface, EVP_AEAD, and
the C implementation, put the AAD parameter after plaintext/ciphertext.
Bug: 275
Change-Id: I46804ff0e55a75742016ff6311bbe6fd6d208355
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46665
Reviewed-by: Dan McArdle <dmcardle@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Also avoid unnecessarily stashing a copy of the serialized old
ClientHello.
Change-Id: I699299f0ce767ba059fbb08e8f2140793a649322
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46628
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
All the comments say the buffer is only needed in TLS 1.2, but this
doesn't match the code. The code uses the buffer in one place, for ECH,
to avoid copying a hash.Hash. Go does support this, albeit in a *very*
roundabout way.
This is ugly but means we can now properly drop the handshake buffer in
TLS 1.3.
Change-Id: I4a1559a64fcb98ccfbab54de99402fe6f62725a1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46627
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The 'client' and 'server' halves are remnants of SSL 3.0 and Go
(originally) lacking a way to clone hash.Hash. The Go limitation meant
that computing SSL 3.0's proto-HMAC construction mutated the running
hash on Finished, so crypto/tls just maintained two of them.
Without SSL 3.0, this is no longer needed. That, however, leaves us with
having both a crypto.Hash and a hash.Hash, and both can't be named
'hash'. I stepped around this by storing the cipher suite itself and
using cipherSuite.hash().
Change-Id: Ia38880ae446949baa2181d33136c748cf5374664
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We only need to implement enough of SSL 3.0 to test that the shim does
not.
Change-Id: I25cb48e407f1bc458bbdb3544b9df9fdfbc3d9c9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Per the comment in writeClientHash, we should writeClientHash before
writeRecord to get the sequence numbers right. Some of the client HRR
bits are still wrong, but I'll fix those as part of tidying up the HRR
path in a later commit.
(This doesn't actually matter because only DTLS uses sequence numbers,
and we don't support DTLS 1.3.)
Change-Id: I4cbc671f524d56c7f970b5ec0bceeb2641625d15
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46624
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is a remnant of when we had various pre-standard TLS 1.3 variants.
runner's logic is now built-in.
Change-Id: I72a2fcef9a94e82fa39fe4be9d60ddd329d212ce
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46604
Reviewed-by: Adam Langley <agl@google.com>
In early TLS 1.3 drafts, HelloRetryRequest was a dedicated message type.
Our HelloRetryRequest handling in runner is still based on this. Along
the way, remove the SendServerHelloAsHelloRetryRequest test, since
that's just a generic unexpected message type now.
Change-Id: Idd9c54d0ab66d962657af9a53849c3928f78ce5c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46585
Reviewed-by: Adam Langley <agl@google.com>
This avoids looking up and passing around the cipherSuite object
everywhere. We don't serialize ClientSessionState and, if we did, we can
simply do the lookup at parsing time.
Change-Id: Ice06e4da6b23ff32988597100e8aaa11b82f23ad
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46565
Reviewed-by: Adam Langley <agl@google.com>
This avoids duplicating some code in client and server. It should also
clean up some ECH test code, which needs to juggle a pair of transcripts
for a brief window.
Change-Id: I4db11119e34b56453f01b5890060b8d4129a25b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46564
Reviewed-by: Adam Langley <agl@google.com>
AES_128_GCM is more common than AES_GCM_128 and matches the
specification.
Bug: 275
Change-Id: If3446a38f7bfbe0250d9646e363db29b93e4d231
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46666
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Dan McArdle <dmcardle@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We don't support renegotiation on the server anymore. Even if we did, we
wouldn't want to rerun ALPN anyway, and we don't do resumption on
renegotiation.
Change-Id: I43438d084bfe5fbe9b011ae0f53349df1baf6c97
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46533
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
BIO_flush may return a negative value, so we shouldn't cast it directly
to bool.
Change-Id: Ibdf688d1a6b4b316069e3b99a8a8b18974ee17ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46534
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's only used from that file and, given the names defined by it,
probably isn't usable by other files anyway.
Change-Id: Ice205408962ade00c1dcb51406da3ef2fd7f0393
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46426
Reviewed-by: Adam Langley <agl@google.com>
We have loads of variations of these. Align them in one set. This avoids
the HOST_* macros defined by md32_common.h, so it'll be a little easier
to make it a more conventional header.
Change-Id: Id47fe7b51a8f961bd87839f8146d8a5aa8027aa6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46425
Reviewed-by: Adam Langley <agl@google.com>
It's a little confusing to have load_word_le but actually use size_t
instead of crypto_word_t.
NOTE: on some platforms, notably NaCl, crypto_word_t is larger than
size_t. (Do we still need to support this?) We don't have a good testing
story here, so I tested it by hacking up a 32-bit x86 build to think it
was OPENSSL_64_BIT.
Change-Id: Ia0ce469e86803f22655fe2d9659a6a5db766429f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46424
Reviewed-by: Adam Langley <agl@google.com>
Some of our calls handled it and others didn't.
Change-Id: I09f15d3db679954599bcf987d86357b6e12e9b9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46532
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The ssl_buffer.cc code handles this, but since outgoing handshake I/O
goes through a different path, it was missing these checks.
Change-Id: I4fed62b435b577645c405d0d995511a58d47a702
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46531
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The check for ssl_hs_read_change_cipher_spec didn't do anything. Replace
it with an assert and add some comments since the hs->wait handling is a
little tricky.
Change-Id: I8e62ce3cceca9bed4611cb9d3faf0bfec3d3bdd4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46530
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's strange to have Serialize/Deserialize methods not inverses of each
other. Split the operation up and move the common parts out of the
subclass.
Change-Id: Iadfa57de19faca411c64b64d2568a78d2eb982e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46529
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The delegated credentials bits got stuck in the middle of the handshake
bits.
Change-Id: I522d8a5a5f000de3e329934851ee74fc4ec613a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46528
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 works, so no need to exclude version negotiation. We also now
only test QUICTransportParams with QUIC, so there is no need to exclude
it manually. Checking the protocol works as well.
Change-Id: Ie9d33095231a1f9eb74145db5147a287e4fdc930
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46527
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is no longer needed.
Change-Id: Ie6dba524ecccd265f7f80a910b40c0fe1800356b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46526
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Do a better job with scopers for fds and posix_spawn_file_actions_t.
There's also no need to make a copy of handshaker_path with strdup.
The non-const parameter are because posix_spawn inherits execve's
C problem: unlike C++, C cannot cast from char *const * to
const char *const *, so POSIX APIs are not const-correct.
Finally, we freely use std::vector and friends in tests, so we don't
actually need to depend on bssl::Array.
Change-Id: I739dcb6b1a2d415d47ff9b2399eebec987aab0bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46524
Reviewed-by: Adam Langley <agl@google.com>
When doing Android FIPS validations one ends up with quite a lot of
different build configurations for ACVP and it's useful to be able to
check that a binary is what you think it is.
Change-Id: Ie5c81f164e6e6903c85ea832a93868f84921e74a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46484
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Omitting the extension means we'll never issue tickets, but if the
client were to offer a ticket anyway, RFC8446 4.2.9 says we MUST reject
the ClientHello. It's not clear on what alert to use, but
missing_extension is probably appropriate.
Thanks to Ben Kaduk for pointing this out.
Change-Id: Ie5c720eac9dd2e1a27ba8a13c59b707c109eaa4e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46464
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
MSAN doesn't like the counters starting at whatever value malloc
found to be free.
Change-Id: I0968e61e0025db35b82291fde5d1e193aef77c1e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46444
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This wasn't being used and wasn't even set correctly in split handshake
tests.
Change-Id: I03000db8dd3c227ea44e7bacaf3d1341259fae44
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46384
Reviewed-by: Adam Langley <agl@google.com>
It's now a year past the February 2020 deadline for removing it. Judging
from b/72831885, it looks like the root cause was addressed.
Change-Id: I8c8b358ef4f4146b41aab2a7163c000fa7306025
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46407
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley
Dan McArdle
Adam Langley