This silences a pile of -Wformat-signedness warnings. We still need
casts in a few places where the API gives int but really wanted
uint16_t. There I cast to unsigned instead of uint16_t for the sake of
not losing information.
With that, we should be -Wformat-signedness-clean on GCC, so enable the
warning.
Bug: 450
Change-Id: I3ab10348bb47d398b8b9b39acf360284a8ab04d7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50771
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Whether the order makes sense is another matter, but keep them aligned
so future flags have an easier time with it.
Change-Id: I3c3912039b593a55af86078b2e9768c76ee2ee14
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50770
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The command-line parser is slightly showing its age: first, it is hard
to add new integral types, such as uint16_t, which is getting in the way
of fixing some of the -Wformat-signedness errors. Second, the parameter
extraction logic and skipping logic is duplicated in every type.
While I'm here, use a binary search to look up the flag, since we have
rather a lot of them. With more C++ template tricks, we could avoid the
std::function, but that seemed more trouble than was worth it,
especially since, prior to C++17, it's a little hard to convince
template argument deduction to infer one of the parameters.
Change-Id: I208f89d46371b31fc8b44487725296bcd9d7c8e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50769
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
GCC has a warning that complains about even more type mismatches in
printf. Some of these are a bit messy and will be fixed in separate CLs.
This covers the easy ones.
The .*s stuff is unfortunate, but printf has no size_t-clean string
printer. ALPN protocol lengths are bound by uint8_t, so it doesn't
really matter.
The IPv6 printing one is obnoxious and arguably a false positive. It's
really a C language flaw: all types smaller than int get converted to
int when you do arithmetic. So something like this first doesn't
overflow the shift because it computes over int, but then the result
overall is stored as an int.
uint8_t a, b;
(a << 8) | b
On the one hand, this fixes a "missing" cast to uint16_t before the
shift. At the same time, the incorrect final type means passing it to
%x, which expects unsigned int. The compiler has forgotten this value
actually fits in uint16_t and flags a warning. Mitigate this by storing
in a uint16_t first.
The story doesn't quite end here. Arguments passed to variadic functions
go through integer promotion[0], so the argument is still passed to
snprintf as an int! But then va_arg allows for a signedness mismatch[1],
provided the value is representable in both types. The combination means
that %x, though actually paired with unsigned, also accept uint8_t and
uint16_t, because those are guaranteed to promote to an int that meets
[1]. GCC recognizes [1] applies here.
(There's also PRI16x, but that's a bit tedious to use and, in glibc, is
defined as plain "x" anyway.)
[0] https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions
[1] https://en.cppreference.com/w/c/variadic/va_arg
Bug: 450
Change-Id: Ic1d41356755a18ab922956dd2e07b560470341f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50765
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Always enable X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS and never enable
X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS.
Update-Note: BoringSSL will no longer accept wildcard patterns like
*www.example.com or www*.example.com. (It already did not accept
ww*w.example.com.) X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS will also be
ignored and can no longer be used to allow foo.bar.example.com to match
*.example.com.
Fixes: 462
Change-Id: I004e087bf70f4c3f249235cd864d9e19cc9a5102
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50705
Reviewed-by: Adam Langley <agl@google.com>
This was added in OpenSSL 1.1.x. It is slightly different from
SSL_pending in that it also reports buffered transport data.
Change-Id: I81e217aad1ceb6f4c31c36634a546e12b6dc8dfc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50445
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
HPKE draft-12 has no changes from draft-08 except that the test vectors
were refreshed and some fields in the JSON file renamed. Also fix the
test vector reference to point to copy from the spec rather than the
(identical) copy from the reference implementation.
Change-Id: Icd4fd467672cc8701fcd2b262ac90c5adc05ac39
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50465
Reviewed-by: Adam Langley <agl@google.com>
After fixing up some issue with the BORINGSSL_IMPLEMENTATION define in
Chromium builds (which used to work fine but, with the test that
references ASN1_ITEM_rptr(BASIC_CONSTRAINTS), is a bit more strict),
I'm running into this warning.
../../third_party/boringssl/src/ssl/internal.h(3695,15): error:
'SSL_CTX_free' redeclared without 'dllimport' attribute: previous
'dllimport' ignored [-Werror,-Winconsistent-dllimport]
friend void SSL_CTX_free(SSL_CTX *);
^
Searching for friend.*EXPORT in Chromium shows they match exports in
friend declarations, so I gather this is just how it works.
Change-Id: I704686854c77406378882477a8bab3f1521e29e4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50145
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This function is currently a no-op, but could be made to do something in
the future to ease the transition of deployments that extract keys from
the handshake and drive the record protocol themselves.
Change-Id: Ib1399e42442dad78173a6462980945559a88a2c7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49886
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
There is a long outdated comment that TLS 1.3 is disabled by default,
which is no longer true. While I'm here, run through all TLS and DTLS
versions, now that we have that table.
Change-Id: I7b813111ad3be295cc5a7e0eb0c7088e40df2a35
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49905
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The two headers already circularly import each other, and even have to
inspect each others' header guards to manage this. Keeping them
separate does not reduce include sizes. Fold them together so their
header guards are more conventional.
Bug: 426
Change-Id: Iaf96f5b2c8adb899d9c4a5b5094ed36fcb16de16
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49770
Reviewed-by: Adam Langley <agl@google.com>
Replace the hardcoded ECH config, which wasn't updated for draft-13,
with a call to SSL_marshal_ech_config.
Bug: 275, oss-fuzz:38054
Change-Id: I10c12b22015c9c0cb90dd6185eb375153a2531f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49445
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Later CLs will clean up the ClientHello construction a bit (draft-12
avoids computing ClientHelloOuter twice). I suspect the transcript
handling on the client can also be simpler, but I'll see what's
convenient after I've changed how ClientHelloOuter is constructed.
Changes of note between draft-10 and draft-13:
- There is now an ECH confirmation signal in both HRR and SH. We don't
actually make much use of this in our client right now, but it
resolves a bunch of weird issues around HRR, including edge cases if
HRR applies to one ClientHello but not the other.
- The confirmation signal no longer depends on key_share and PSK, so we
don't have to work around a weird ordering issue.
- ech_is_inner is now folded into the main encrypted_client_hello code
point. This works better with some stuff around HRR.
- Padding is moved from the padding extension, computed with
ClientHelloInner, to something we fill in afterwards. This makes it
easier to pad up the whole thing to a multiple of 32. I've accordingly
updated to the latest recommended padding construction, and updated
the GREASE logic to match.
- ech_outer_extensions is much easier to process because the order is
required to be consistent. We were doing that anyway, and now a simple
linear scan works.
- ClientHelloOuterAAD now uses an all zero placeholder payload of the
same length. This lets us simplify the server code, but, for now, I've
kept the client code the same. I'll follow this up with a CL to avoid
computing ClientHelloOuter twice.
- ClientHelloOuterAAD is allowed to contain a placeholder PSK. I haven't
filled that in and will do it in a follow-up CL.
Bug: 275
Change-Id: I7464345125c53968b2fe692f9268e392120fc2eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48912
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This unexports X509, X509_CINF, X509_NAME_ENTRY, X509_NAME, X509_OBJECT,
X509_LOOKUP_METHOD, X509_STORE, X509_LOOKUP, and X509_STORE_CTX.
Note this means X509_STORE_CTX can no longer be stack-allocated.
Update-Note: Patch cl/390055173 into the roll that includes this. This
unexports most of the X.509 structs, aligning with OpenSSL. Use the
accessor APIs instead.
Bug: 425
Change-Id: I53e915bfae3b8dc4b67642279d0e54dc606f2297
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48985
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
tls13_init_key_schedule calls InitHash internally, but we also call
InitHash earlier at various times. On the client, we do it early to
handle HelloRetryRequest and 0-RTT. ECH draft-12 will also need to do it
early. Apparently we do it early on the server too.
Probably tls13_init_key_schedule doesn't need to call InitHash, but for
now, it is an easy check in SSLTranscript.
Change-Id: I5473047c1f29bdeb60901e4e6e80979e592bd6e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48911
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
std::initializer_list appears to work by instantiating a T[N] at the
call site (which is what we were doing anyway), so I don't believe there
is a runtime dependency.
This also adds a way for individual entries to turn themselves off,
which means we don't need to manually check for some unsolicited
extensions.
Change-Id: I40f79b6a0e9c005fc621f4a798fe201bfbf08411
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48910
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We do this enough that it's worth extracting a common parser. And this
gives a struct we can pass around. Note this moves the server extensions
block parsing out of ssl_scan_serverhello_tlsext.
I've also consolidated a few error conditions to tighten the code up a
bit: the TLS 1.2 code distinguishes unknown from unadvertised cipher,
while the TLS 1.3 code didn't. And seeing the wrong legacy version
number in TLS 1.3 is really just a syntax error since it's not the
version field anymore. (RFC8446 specifies the value.)
Change-Id: Ia2f44ff9a3899b5a594569f1b258f2b487930496
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48908
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We fill in placeholder values of all zeros fairly often in TLS now,
as workarounds for messages being constructed in the wrong order.
draft-12 of ECH adds even more of these. Add a helper so we don't need
to interrupt an || chain with a memset.
Change-Id: Id4f9d988ee67598645a01637cc9515b475c1aec2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48909
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The session ID field cannot exceed 32 bytes, and we size various buffers
based on this. Test that our parsers correctly handle this.
Also fix the -wait-for-debugger flag. I broke it recently by removing
the statusShimStarted message.
Change-Id: I29bb177f29a79bb4904fb5ba3cedfb0b6b856061
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48907
Reviewed-by: Adam Langley <agl@google.com>
The cipher suite, like the version, is determined by the first server
message, independent of whether it's ServerHello or HelloRetryRequest.
We can simplify this by just processing it before we branch on which it
was.
Change-Id: I747f515e9e5b05a42cbed6e7844808d0fc79a30b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48906
Reviewed-by: Adam Langley <agl@google.com>
absl::Span, base::span, and std::span have first() and last() methods
which give prefixes and suffixes. first() just saves 5 characters, but
last() is nicer to write than subspan() for suffixes.
Unlike subspan(), they also do not have clipping behavior, so we're
guaranteed the length is correct. The clipping behavior comes from
absl::Span::subspan() and is not present in std::span or base::span.
I've left it in, in case we switch to absl::Span in the future, but I
imagine absl::Span will need to migrate this at some point.
Change-Id: I042dd6c566b6d753ec6de9d84e8c09ac7c270267
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48905
Reviewed-by: Adam Langley <agl@google.com>
It's a bit of a mess, but BIO-like APIs typically return -1 on error and
0 for EOF.
Change-Id: Ibdcb70e1009ffebf6cc6df40804dc4a178c7199e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48845
Reviewed-by: Adam Langley <agl@google.com>
The stack consumption of the HRSS functions is causing issues in
stack-constrained environments. Therefore allocate many variables on the
heap. This means that several HRSS_ functions now allocate, and thus can
fail, where they couldn't before. Callers that ignore the return value
and don't have crash-on-failure mallocs will still be safe, although
things will fail to decrypt later on.
Somehow, this actually makes key generation _faster_ on my machine. (I
don't know. Better alignment? Fewer L1 collisions?) The other operations
are slightly slower, as expected.
Before:
Did 17390 HRSS generate operations in 3054088us (5694.0 ops/sec)
Did 225000 HRSS encap operations in 3000512us (74987.2 ops/sec)
Did 87000 HRSS decap operations in 3014525us (28860.3 ops/sec)
After:
Did 21300 HRSS generate operations in 3026637us (7037.5 ops/sec)
Did 221000 HRSS encap operations in 3008911us (73448.5 ops/sec)
Did 84000 HRSS decap operations in 3007622us (27929.0 ops/sec)
Change-Id: I2312df8909af7d8d250c7c483c65038123f21ad9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48345
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Found by OSS-Fuzz. This comes up if you enable client certificates and
the draft ECH implementation on the server.
Bug: 275, oss-fuzz:35815
Change-Id: I0b4fcc994f7238f8a3cf1f1934672bac0cee0cfb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48425
Reviewed-by: Adam Langley <agl@google.com>
If a client offers ECH, but the server rejects it, the client completes
the handshake with ClientHelloOuter in order to authenticate retry keys.
Implement this flow. This is largely allowing the existing handshake to
proceed, but with some changes:
- Certificate verification uses the other name. This CL routes this up to
the built-in verifier and adds SSL_get0_ech_name_override for the
callback.
- We need to disable False Start to pick up server Finished in TLS 1.2.
- Client certificates, notably in TLS 1.3 where they're encrypted,
should only be revealed to the true server. Fortunately, not sending
client certs is always an option, so do that.
Channel ID has a similar issue. I've just omitted the extension in
ClientHelloOuter because it's deprecated and is unlikely to be used
with ECH at this point. ALPS may be worth some pondering but, the way
it's currently used, is not sensitive.
(Possibly we should change the draft to terminate the handshake before
even sending that flight...)
- The session is never offered in ClientHelloOuter, but our internal
book-keeping doesn't quite notice.
I had to replace ech_accept with a tri-state ech_status to correctly
handle an edge case in SSL_get0_ech_name_override: when ECH + 0-RTT +
reverify_on_resume are all enabled, the first certificate verification
is for the 0-RTT session and should be against the true name, yet we
have selected_ech_config && !ech_accept. A tri-state tracks when ECH is
actually rejected. I've maintained this on the server as well, though
the server never actually cares.
Bug: 275
Change-Id: Ie55966ca3dc4ffcc8c381479f0fe9bcacd34d0f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48135
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
base64.StdEncoding.EncodeToString is very long to write out.
Change-Id: Ie987d483513e4192a31c8562b9cf25e99f8a838b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48134
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
ssl_update_cache takes the cache lock to add to the session cache,
releases it, and then immediately takes and releases the lock to
increment handshakes_since_cache_flush. Then, in 1/255 connections, does
the same thing again to flush stale sessions.
Merge the first two into one lock. In doing so, move ssl_update_cache to
ssl_session.cc, so it can access a newly-extracted add_session_lock.
Also remove the mode parameter (the SSL knows if it's a client or
server), and move the established_session != session check to the
caller, which more directly knows whether there was a new session.
Also add some TSan coverage for this path in the tests. In an earlier
iteration of this patch, I managed to introduce a double-locking bug
because we weren't testing it at all. Confirmed this test catches both
double-locking and insufficient locking. (It doesn't seem able to catch
using a read lock instead of a write lock in SSL_CTX_flush_sessions,
however. I suspect the hash table is distributing the cells each thread
touches.)
Update-Note: This reshuffles some locks around the session cache.
(Hopefully for the better.)
Change-Id: I78dca53fda74e036b90110cca7fbcc306a5c8ebe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48133
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
In renegotiation handshakes and, later, ECH ClientHelloOuter handshakes,
we don't want to add sessions to the session cache. We also don't want
to release a session as resumable until the handshake completes.
Ideally we'd only construct SSL_SESSION at the end of the handshake, but
existing APIs like SSL_get_session must work mid-handshake, so
SSL_SESSION is both a handle to immutable resumption state, and a
container for in-progress connection properties. We manage this with a
not_resumable flag that's only cleared after the handshake is done and
the SSL_SESSION finalized.
However, TLS 1.2 ticket renewal currently clears the flag too early and
breaks the invariant. This won't actually affect renegotiation or
ClientHelloOuter because those handshakes never resume. Still, we can
maintain the invariant storing the copy in hs->new_session. Note this
does sacrifice a different invariant: previously, ssl->session and
hs->new_session were never set at the same time.
This change also means ssl_update_cache does not need to special-case
ticket renewal.
Change-Id: I03230cd9c63e5bee6bd60cd05c0439e16533c6d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48132
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Some tests run three connections, resuming a renewed ticket.
Particularly the way TLS 1.2 ticket renewal works, the client logic
could accidentally report the old session up to the application. Our
runner tests would not currently notice (though one of the tests in
ssl_tests does).
Make runner tests also check this by cycling ticket keys between
connection attempts. This also makes newSessionsOnResume apply even if
the test did not specify a resumeConfig.
Change-Id: I95375c01adf6ad62de65ecf8aed3c286a0571875
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48131
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This addresses some feedback in
https://boringssl-review.googlesource.com/c/boringssl/+/48131/1/ssl/test/runner/runner.go#1555,
pulled into a separate CL for clarity:
First, take the listener, waitChan, exec.Cmd trio and wrap them into a
shimProcess type. shimProcess is now responsible for the -port flag, so
it can manage the TCPListener internally.
Next, take the core test loop and moves it into a doExchanges()
function, so that it can use a more usual early return pattern for
errors, rather than thread err == nil through all the control flow. With
shimProcess pulled out, doExchanges() can just take a *shimProcess.
Finally, unacted-on err variable has gotten very far from where it's
actually used. Rename it to localErr, to align with our
expectedLocalError machinery.
Change-Id: I63697a5d79040ad77fa06c125253ec5031aeaf5c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48186
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The one place where LHASH_OF(T) appears in public APIs is
X509V3_EXT_conf_nid. This is only ever called with conf = NULL, but
cryptography.io needs to utter the type name as part of bindings. Thus
this CL keeps DECLARE_LHASH_OF and LHASH_OF macros public and the others
private.
Update-Note: BoringSSL no longer provides a general-purpose hash table
to callers. Use the language's standard library, or another
implementation.
Change-Id: Ibfc65c4b4bf35abf5b1919658d0c52e4004e6629
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48205
Reviewed-by: Adam Langley <agl@google.com>
This is a bit more self-explanatory, especially now that TLS 1.0 is the
minimum version we implement anyway.
Change-Id: Ic65e9f90bb5cd747328bd9e30b976d1e124c7764
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48130
Reviewed-by: Adam Langley <agl@google.com>
This was added in draft-11, which I'll update to more broadly in a
follow-up CL. This is an easily separable component: we don't want to
allow the DNS to arbitrarily insert strings in the ClientHello, so
invalid public names are rejected.
Unfortunately, we have a bit of a mess because DNS syntax does not
exclude IPv4 literals, yet everyone sticks DNS and IP literals in the
same string. The RFC3986 rules are alright, but don't match reality.
Reality is (probably?) the WHATWG rules, which are a mess.
The load-bearing bit of the spec is that, at certificate verification,
you should reject whatever strings your application refuses to represent
as a DNS name. I'll have Chromium call into its URL parser.
https://www.ietf.org/archive/id/draft-ietf-tls-esni-11.html#section-6.1.4.3-3
But there's still a bit at the validation step where clients "SHOULD"
run the IPv4 parser. In case downstream logic forgets, I've gone ahead
and implemented the WHATWG IPv4 parser.
https://www.ietf.org/archive/id/draft-ietf-tls-esni-11.html#section-4-6.6.1
Bug: 275
Change-Id: I15aa1ac0391df9c3859c44b8a259296e1907b7d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48085
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This CL fixes a couple of things. First, we never tested that SSL_write
refuses to write application data after a fatal alert, so add some tests
here. With those tests, we can revise some of this logic:
Next, this removes the write_shutdown check in SSL_write and instead
relies on the lower-level versions of the check in the write_app_data,
etc., hooks. This improves error-reporting on handshake errors:
We generally try to make SSL_do_handshake errors sticky, analogous to
handshakeErr in the Go implementation. SSL_write and SSL_read both
implicitly call SSL_do_handshake. Callers driving the two in parallel
will naturally call SSL_do_handshake twice. Since the error effectively
applies to both operations, we save and replay handshake errors
(hs->error).
Handshake errors typically come with sending alerts, which also sets
write_shutdown so we don't try to send more data over the channel.
Checking this early in SSL_write means we don't get a chance to replay
the handshake error. So this CL defers it, and the test ensures we still
ultimately get it right.
Finally, https://crbug.com/1078515 is a particular incarnation of this.
If the server enables 0-RTT and then reverts to TLS 1.2, clients need
to catch the error and retry. There, deferring the SSL_write check
isn't sufficient, because the can_early_write bit removes the write
path's dependency on the handshake, so we don't call into
SSL_do_handshake at all.
For now, I've made this error path clear can_early_write. I suspect
we want it to apply to all handshake errors, though it's weird because
the handshake error is effectively a read error in 0-RTT. We don't
currently replay record decryption failures at SSL_write, even though
those also send a fatal alert and thus break all subsequent writes.
Bug: chromium:1078515
Change-Id: Icdfae6a8f2e7c1b1c921068dca244795a670807f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48065
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Although not permitted by the TLS specification, systems sometimes
ossify TLS extension order, or byte offsets of various fields. To
keep the ecosystem healthy, add an API to reorder ClientHello
extensions.
Since ECH, HelloRetryRequest, and HelloVerifyRequest are sensitive to
extension order, I've implemented this by per-connection permutation of
the indices in the kExtensions structure. This ensures that all
ClientHellos within a connection are consistently ordered. As follow-up
work, permuting the other messages would also be nice, though any server
messages would need to be incorporated in handshake hints.
Change-Id: I18ce39b4df5ee376c654943f07ec26a50e0923a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This would have caught an issue with some tests I was working on. It
also catches an issue with some per-message tests, so fix those.
Change-Id: I6b3ad8e0db0b1a6ccac4b346dcc652b16b73e006
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48046
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Based on an initial implementation by Dan McArdle at
https://boringssl-review.googlesource.com/c/boringssl/+/46784
This CL contains most of a client implementation for
draft-ietf-tls-esni-10. The pieces missing so far, which will be done in
follow-up CLs are:
1. While the ClientHelloInner is padded, the server Certificate message
is not. I'll add that once we resolve the spec discussions on how to
do that. (We were originally going to use TLS record-level padding,
but that doesn't work well with QUIC.)
2. The client should check the public name is a valid DNS name before
copying it into ClientHelloOuter.server_name.
3. The ClientHelloOuter handshake flow is not yet implemented. This CL
can detect when the server selects ClientHelloOuter, but for now the
handshake immediately fails. A follow-up CL will remove that logic
and instead add the APIs and extra checks needed.
Otherwise, this should be complete, including padding and compression.
The main interesting point design-wise is that we run through
ClientHello construction multiple times. We need to construct
ClientHelloInner and ClientHelloOuter. Then each of those has slight
variants: EncodedClientHelloInner is the compressed form, and
ClientHelloOuterAAD just has the ECH extension erased to avoid a
circular dependency.
I've computed ClientHelloInner and EncodedClientHelloInner concurrently
because the compression scheme requires shifting the extensions around
to be contiguous. However, I've computed ClientHelloOuterAAD and
ClientHelloOuter by running through the logic twice. This probably can
be done better, but the next draft revises the construction anyway, so
I'm thinking I'll rework it then. (In the next draft, we use a
placeholder payload of the same length, so we can construct the
ClientHello once and fill in the payload.)
Additionally, now that we have a client available in ssl_test, this adds
a threading test to confirm that SSL_CTX_set1_ech_keys is properly
synchronized. (Confirmed that, if I drop the lock in
SSL_CTX_set1_ech_keys, TSan notices.)
Change-Id: Icaff68b595035bdcc73c468ff638e67c84239ef4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48004
Reviewed-by: Adam Langley <agl@google.com>
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>
David Benjamin
Adam Langley