We'll probably need to make this more complex later, but this should be
a start. I had hoped this would also simplify tests, MakeECHConfig() was
still needed to generate weird inputs for tests. I've instead tidied
that up a bit with a params structure. Now the only hard-coded ECHConfig
in tests is to check the output of the new API.
Bug: 275
Change-Id: I640a224fb4b7a7d20e8a2cd7a1e75d1e3fe69936
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48003
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Previously we would extract the KEM ID from the ECHConfig and then parse
the private key using the corresponding KEM type. This CL makes it take
a pre-pared EVP_HPKE_KEY and checks it matches. This does require the
caller pass the key type through externally, which is probably prudent?
(On the other hand we are still inferring config from the rest of the
ECHConfig... maybe we can add an API to extract the EVP_HPKE_KEM from a
serialized ECHConfig if it becomes a problem. I could see runner or tool
wanting that out of convenience.)
The immediate motivation is to add APIs to programmatically construct
ECHConfigs. I'm thinking we can pass a const EVP_HPKE_KEY * to specify
the key, at which point it's weird for SSL_ECH_KEYS_add to look
different.
Bug: 275
Change-Id: I2d424323885103d3fe0a99a9012c160baa8653bd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48002
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We didn't correctly handle tests where the versions figure into
resumeConfig and got by because the test didn't actually check the
version. Run it more accurately, and check more fields. Also add a
skipVersionNameCheck option for when the heuristic doesn't work.
(Some of the tests specify a TLS maximum version by passing in all the
-no-tls1, etc., flags for the other versions. Moreover, some of them
will set no flags for a maximum of TLS 1.3. Suppress the check on those
tests.)
Change-Id: I72c069b1baed09e29bf502036957fe0e90edbe60
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48000
Reviewed-by: Adam Langley <agl@google.com>
While the previous CL fixed a bug in computing this padding, we don't
actually need to pad the second (cleartext) ClientHello anyway. This
padding is to work around bugs in old F5 and WebSphere servers, which do
not speak TLS 1.3. Save a few bytes.
Change-Id: I9b5d9bb1c0d880f1b1c9182667a9d3d82588c04c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47999
Reviewed-by: Adam Langley <agl@google.com>
If we're dropping the PSK extension due to an HRR with mismatched hash
(looking back at that, we could easily have avoided that nuisance...
I've filed [0] on rfc8446bis), we don't predict the length correctly.
The consequence is we don't pad the second ClientHello to the desired
range. Fix this and add an assert.
[0] https://github.com/tlswg/tls13-spec/issues/1227
Change-Id: I47d880b6bdafa95840f7513b6b7718851f22554d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47998
Reviewed-by: Adam Langley <agl@google.com>
Computing the binders on ClientHelloInner is a little interesting. While
I'm in the area, tidy this up a bit. The exploded parameters may as well
be an SSL_SESSION, and hash_transcript_and_truncated_client_hello can
just get folded in.
Change-Id: I9d3a7e0ae9f391d6b9a23b51b5d7198e15569b11
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47997
Reviewed-by: Adam Langley <agl@google.com>
This makes calls to ssl_add_clienthello_tlsext a hair easier. Also we
only apply the [256, 511) compatibility hack to TLS, so we can just use
a constant.
Bug: 275
Change-Id: Ia2b5192aeef0cd8848ecfa1ea3b89a0a7382ff1a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47996
Reviewed-by: Adam Langley <agl@google.com>
ssl_add_clienthello_tlsext is about to get kinda messy with ECH. Move
the padding and GREASE extensions into a few helpers.
Bug: 275
Change-Id: I3bb702fb79dce4be68490c4a8fd889121ecdae58
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47995
Reviewed-by: Adam Langley <agl@google.com>
For TLS 1.3, since the bulk of extensions move to EncryptedExtensions,
we made the extension callbacks apply to EncryptedExtensions and pulled
the few ServerHello extensions out of the callback system. This means
the ServerHello naming is a little confusing.
We probably should rename these callbacks, though parse_server is a bit
ambiguous as to whether this is "parse the extension from the server" or
"parse as a server". For now, add a comment.
Change-Id: If1aa0846426de2cc8dcb2253695a8dd3285f7b76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47994
Reviewed-by: Adam Langley <agl@google.com>
We'll need to maintain two transcripts on the ECH client and then, once
we know which of ClientHelloOuter or ClientHelloInner is used, overwrite
the default transcript with the alternate one.
Rather than indirect through a pointer, move support is easy enough.
Then this can just be hs->transcript = std::move(hs->inner_transcript).
Bug: 275
Change-Id: Id4b0a0a48b956cd65ce8fc3dacfd16eebe2eb778
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47993
Reviewed-by: Adam Langley <agl@google.com>
May not be strictly necessary, but similarly easier to reason about when
we need to interweave multiple ClientHellos.
Bug: 275
Change-Id: I9f85787860f3e8ce1653331ce52343d5bf5def23
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47992
Reviewed-by: Adam Langley <agl@google.com>
This is less effective than it seems because both
((const SSL_HANDSHAKE*)hs)->ssl and ((const SSL*)ssl)->s3 are both
non-const pointers. I considered moving hs->ssl to hs->ssl_ and adding
const-overloaded accessors, but I'd need to repeat the same with
ssl->s3, at which point this seemed not worth it yet. Maybe later if we
rewrite everything to more idiomatic C++.
Bug: 275
Change-Id: I9912a3df205916fdf2191a3687013d717528038d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47991
Reviewed-by: Adam Langley <agl@google.com>
This is kinda annoying and, like the grease_seed, demonstrates a
shortcoming with the idea of making add_clienthello completely const.
Strictly speaking, we only need idempotence. But I think this is okay:
const is much easier to enforce than idempotence, and we'll likely need
to do this anyway:
- While not in the current draft, I expect the draft's eventual HRR
resolution to retain the ECH extension, GREASE or not, on ECH reject.
Things are somewhat violating RFC8446 HRR rules right now. That means
we'll need to stash the ECH payload regardless.
- ECH binds all the other extensions in the outer ClientHello, so
computing the payload will need to move outside the callback system
anyway.
In some sense, all this is shifting our ClientHello output from the
"immediate mode" end of the spectrum (as we usually use) to the
"retained mode" end, which I suppose makes sense as this message becomes
more intricate.
Bug: 275
Change-Id: I9eb8cd1cde2ce264345b6ed3ee526d4eab81e911
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47990
Reviewed-by: Adam Langley <agl@google.com>
This lets ssl_get_grease_value be const. The lazy thing isn't a
deal-breaker (we only need idempotence, and a non-thread-safe const also
works fine), but just initializing it earlier seems simpler.
Bug: 275
Change-Id: Iad228ea4a9146ede9a3849f3339f7ec9e698e6eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47988
Reviewed-by: Adam Langley <agl@google.com>
This is now never used. Instead, we rely on each renegotiation creating
a new handshake structure with fresh state. This simplifies things for
ECH.
(We probably could make an init hook work with ECH's two-ClientHello
scheme by either maintaining separate state per ClientHello or calling
init once for both ClientHellos. But the few uses of init were
removable, so this is easier.)
Bug: 275
Change-Id: Ie5e132fe072e5ea8db21ca16aa53fcd0895d8e48
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47987
Reviewed-by: Adam Langley <agl@google.com>
Like the early_data CL, this does shift a bit of logic that was
previously hidden away in the callbacks. For key_share, this is probably
a good move independent of ECH. The logic around HRR, etc., was a little
messy.
Bug: 275
Change-Id: Iafbcebdf66ce1f7957d798a98ee6b996fff24639
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47986
Reviewed-by: Adam Langley <agl@google.com>
Also add ECH GREASE state into the mix. Clearing this isn't critical,
especially now that we have an SSL_HANDSHAKE structure, but it's easy
enough.
Bug: 275
Change-Id: If1aa8d5c0c8fdb5af710852778ce452c507a2524
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47985
Reviewed-by: Adam Langley <agl@google.com>
ECH requires that we construct two ClientHellos. That means our
add_clienthello callbacks will need to be called multiple times and
should be const. (They already are called multiple times for
HelloRetryRequest, but we currently thread that through the callbacks a
bit. With ECH, I think we need to make them pure serialization.)
Bug: 275
Change-Id: I11f8195fd2ec4b8639f0a2af01a24d4974445580
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47984
Reviewed-by: Adam Langley <agl@google.com>
While decompression is deterministic, compression is not. New revisions
of the compression algorithm may start using different (hopefully
smaller!) compressions. So this doesn't cause hint mismatches, add a
certificate compression hint. If the shim's Certificate message matches
the handshaker, we'll reuse the already compressed message.
This also adds what appears to be a missing test for when the server
cannot find compression algorithms in common.
Change-Id: Idbedaceb20208463d8f61581ee27971c17fcd126
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48005
Reviewed-by: Adam Langley <agl@google.com>
This implements draft-ietf-tls-esni-10.
This will be used to test the client implementation. While I'm here,
I've switched the setup logic in the server tests to use the new
ServerECHConfig type. I'll probably need to patch in various features
later for testing, but this should be a usable starting point.
Based on an initial implementation by Dan McArdle in
https://boringssl-review.googlesource.com/c/boringssl/+/46786
Bug: 275
Change-Id: I69523cda70c3da2ae505bcab837fd358195fb9e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47967
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Noticed this while I was in the area. We currently use an extremely lax
parse that even tolerates syntax errors. Instead use a strict parse that
ensures our client only sends what we expect.
Change-Id: Ifb0e1e1698489ff217db0c7a0317caa885e20759
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47966
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Having the nil vs. non-nil []byte for the sake of a couple tests with
invalid payloads is tedious. Use separate fields instead.
Bug: 275
Change-Id: I557d914d60ce94d68796c05162ff3dd2ab7684db
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47965
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
An ECHConfig is like a certificate in that knowing the fields isn't
sufficient. The exact byte representation is significant. (The ECHConfig
is bound into the encryption.) But the ECHConfig type only has fields,
so runner can only represent ECHConfigs that are the output of our
serialization function.
This matters less as a client testing a server because the server can
only parse ECHConfigs with fields we support. But as a server testing a
client, we need to see how the client reacts to extra extensions, etc.
Just using []byte to represent ECHConfigs is inconvenient, so instead
pattern this after x509.Certificate: you can parse one from a byte
string (not currently included since we don't need it yet), or you can
construct a new one from a template with the fields you want.
Bug: 275
Change-Id: I6602d0780b1cef12b6c4b442999bdff7b3d7dd70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47964
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The test was not actually using a repeated config ID.
Bug: 275
Change-Id: I69519fde196247abb07dceba1da1bac22188f13f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47912
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The first thing any deployment will want to monitor is whether ECH was
actually used. Also it's useful if the command-line tool can output
this. (The alert is how the client signals it discarded the connection
due to ECH reject.)
This also disables ECH with the handoff mechanism for now. (The
immediate cause being that ech_accept isn't serialized.) We'll probably
need to make some decisions around the ordering here, since ECH affects
where the true ClientHello is available.
Bug: 275
Change-Id: Ie4559733290e653a514fcd94431090bf86bc3172
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47911
Reviewed-by: Adam Langley <agl@google.com>
The ECH server extension is defined for TLS 1.3 EncryptedExtensions, not
TLS 1.2 ServerHello.
Bug: 275
Change-Id: Ie6e76c238075d70e6a0694ec0192df07da3457d1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47910
Reviewed-by: Adam Langley <agl@google.com>
Make it a little clearer they shouldn't be updating sequence numbers,
enqueuing the message, etc. That's left to add_message. (ECH clients
need to construct a pair of parallel ClientHellos.)
Change-Id: I554a8f200d464727bc219b66931b3d0bae266f76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47908
Reviewed-by: Adam Langley <agl@google.com>
The remaining remnants of Channel ID all configure the private key ahead
of time. Unwind the callback machinery, which cuts down on async points
and the cases we need to test.
This also unwinds some odd interaction between the callback and
SSL_set_tls_channel_id_enabled: If a client uses
SSL_set_tls_channel_id_enabled but doesn't set a callback, the handshake
would still pause at SSL_ERROR_WANT_CHANNEL_ID_LOOKUP. This is now
removed, so SSL_set_tls_channel_id_enabled only affects the server and
SSL_CTX_set1_tls_channel_id only affects the client.
Update-Note: SSL_CTX_set_channel_id_cb is removed.
SSL_set_tls_channel_id_enabled no longer enables Channel ID as a client,
only as a server.
Change-Id: I89ded99ca65e1c61b1bc4e009ca0bdca0b807359
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47907
Reviewed-by: Adam Langley <agl@google.com>
The channel_id_valid bit is both used for whether channel_id is filled
in (SSL_get_tls_channel_id), and whether this particular handshake will
eventually negotiate Channel ID.
The former means that, if SSL_get_tls_channel_id is called on the
client, we'll return all zeros. Apparently we never fill in channel_id
on the client at all. The latter means the state needs to be reset on
renegotiation because we do not currently forbid renegotiation with
Channel ID (we probably should...), which is the last use of the init
callback for extensions.
Instead, split this into a bit for the handshake and a bit for the
connection. Note this means we actually do not expose or even retain
whether Channel ID was used on the client.
This requires a tweak to the handoff logic, but it should be compatible.
The serialized ssl->s3->channel_id was always a no-op: the handback
happens before the ChannelID message, except in RSA key exchange. But we
forbid Channel ID in RSA key exchange anyway.
Update-Note: SSL_get_tls_channel_id will no longer return all zeros
during the handshake or on the client. I did not find any callers
relying on this.
Change-Id: Icd4b78dd3f311d1c7dfc1cae7d2b86dc7e327a99
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47906
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing to worry about the interaction with renegotiation
which, in turn, means we can drop the init callback. (If we did support
DTLS renegotiation, we'd probably want to forbid the parameter from
changing anyway. Changing your SRTP parameters partway through will
likely confuse the RTP half of the application anyway.)
Change-Id: Ifef1e9479d9df296b69b0d296f6bef57b13da68e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47905
Reviewed-by: Adam Langley <agl@google.com>
Because file names are not enclosed in quotation marks in the open call.
https://bugs.chromium.org/p/boringssl/issues/detail?id=415
```
cmake --build "C:\Projects\ Extern\Visual C++ 2015\x64 Debug\Build\BoringSSL\."
[9/439] Generating rdrand-x86_64.asm
FAILED: crypto/fipsmodule/rdrand-x86_64.asm
cmd.exe /C "cd /D "C:\Projects\ Extern\Visual C++ 2015\x64 Debug\Build\BoringSSL\crypto\fipsmodule" && "C:\Program Files\CMake\bin\cmake.exe" -E make_directory . && C:\Perl64\bin\perl.exe "C:/Projects/ Extern/Source/BoringSSL/crypto/fipsmodule/rand/asm/rdrand-x86_64.pl" nasm rdrand-x86_64.asm"
Can't open perl script "C:/Projects/": No such file or directory
error closing STDOUT at C:/Projects/ Extern/Source/BoringSSL/crypto/fipsmodule/rand/asm/rdrand-x86_64.pl line 87.
ninja: build stopped: subcommand failed.
```
Bug: 415
Change-Id: I83c4a460689b9adeb439425ad390322ae8b2002a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47884
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Also now that it's finalized, flip the default for
SSL_set_quic_use_legacy_codepoint.
Update-Note: QUIC APIs now default to the standard code point rather
than the draft one. QUICHE has already been calling
SSL_set_quic_use_legacy_codepoint, so this should not affect them. Once
callers implementing the draft versions cycle out, we can then drop
SSL_set_quic_use_legacy_codepoint altogether. I've also bumped
BORINGSSL_API_VERSION in case we end up needing an ifdef.
Change-Id: Id2cab66215f4ad4c1e31503d329c0febfdb4603e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47864
Reviewed-by: David Schinazi <dschinazi@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Node.js uses EVP_PKEY_get0, which is present in OpenSSL but which
BoringSSL currently does not export. This CL adds an implementation
for it, which Electron is currently floating as a patch.
See
6a7eb32c5b
from Node.
Change-Id: I2474cacbd22882355a8037e2033739f7496b21f2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47824
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Similar to
https://boringssl-review.googlesource.com/c/boringssl/+/46405,
SHA256_Final and SHA224_Final hit array size warnings in the new GCC.
The array sizes are, strictly speaking, purely decoration, but this is a
good warning so we should be clean with it on.
That same change is difficult to apply to md32_common.h because
md32_common.h generates the functions for us. md32_common.h is already
strange in that it is multiply-included and changes behavior based on
macros defined by the caller.
Instead, replace it with inline functions, which are a bit more
conventional and typesafe. This allows each hash function to define the
function prototype. Use this to add an unsized helper for SHA-256.
Bug: 402
Change-Id: I61bc30fb58c54dd40a55c9b1ebf3fb9adde5e038
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47807
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Peter Foley <pefoley@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The macro isn't doing any work here.
Change-Id: Id97dfa4b027407c5e4b3e7eb1586c3c2a2d977d8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47806
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This adds a check to EVP_get_cipherbyname which ensures that name
is not null when passed to OPENSSL_strcasecmp, which cannot handle
null values.
OpenSSL already ensures this in their implementation of
EVP_get_cipherbyname by using OBJ_NAME_get, so this improves parity.
Change-Id: Icea45a5da2a7a461d2a65fbfbc84653c4f124dab
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47844
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
When decrypting a ticket we would copy the client's session ID into the
session and then copy the session's ID into the ServerHello (if
resuming). That seems icky. Instead install the same placeholder on the
server as we do on the client.
Change-Id: Icb50a3be2f05e6428f1b286c8c09015f7bb4af16
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47784
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also shortens ECH variable names in runner.go.
Bug: 275
Change-Id: Iaef520ae09eb94f714fbdaa4383d1456add6f113
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47744
Commit-Queue: Dan McArdle <dmcardle@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Now skipping over HPKE decryption in |ssl_client_hello_decrypt| when
fuzzer mode is enabled. To improve code coverage, this fuzzer-only logic
also also has the ability to simulate a failed decryption.
As a result of mostly skipping the decryption, we now have to exclude
"*-ECH-Server-Decline*" tests from running in fuzzer mode. These tests
rely on the now-broken assumption that decryption will fail when the
client used an ECHConfig unknown to the server.
Bug: 275
Change-Id: I759a79c8596897cdd3d3a37e05f2973d47346ef9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47624
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We'll return 0 and get confused. (Negotiating early data and not using
it is plausible if, say, the client preconnects but gets a ServerHello
before any request binds the socket.)
Change-Id: I94d458e18c58223f73c9340cac06e5ec5f8c84a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47684
Reviewed-by: Adam Langley <agl@google.com>
If we ever forget to fill it in the randoms, they'll end up all zero.
Particularly at the ClientHello, that logic is getting increasingly far
away from ClientHello serialization, so add a test to make sure we
notice.
(This will flakily fail with probability 2^-256, which is reasonably
unlikely.)
Change-Id: I81f32fd96dbccf377cb92198a222b557ab66976b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47665
Reviewed-by: Adam Langley <agl@google.com>
This is part of a very deep dependency chain. I'm sniffing at making all
the add_clienthello callbacks const. Between HelloVerifyRequest,
HelloRetryRequest, and soon ECH, we're creating lots of ClientHellos per
connection. That's probably easiest to manage if constructing a
ClientHello had no side effects.
Update-Note: The change to the return type isn't quite compatible, but I
only found one caller of this function, which has since been fixed. (If
we need to return a non-const value for compatibility, we can do that
and document that the caller should not mutate the output.)
Change-Id: I21f18f7438920a5b03d874fa548f054af3a42c4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47664
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Florin Crișan
Shelley Vohr
Peter Foley
Adam Langley
Dan McArdle