See also f8fc0e35e0b1813af15887d42e17b7d5537bb86c from upstream, though
our BN_divs have diverged slightly.
Change-Id: I49fa4f0a5c730d34e6f41f724f1afe3685470712
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48426
Reviewed-by: Adam Langley <agl@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>
X509*_get_*_by_NID return -1 if the extension was not found, but -2 if
the NID was invalid. Looking through callers, many check index != -1,
rather than index < 0. That means, in theory, they'll do the wrong thing
in some cases.
Realistically, this case is impossible: most callers pass in a constant.
Even in those that don't, NIDs are a local enum, not standard constants.
That means hitting this path is almost certainly a programmer error. No
need to complicate the calling convention for it.
Update-Note: The return value convention of some functions was
simplified. This is not expected to affect any callers.
Change-Id: If2f5a45c37caccdbfcc3296ff2db6db1183e3a95
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48368
Reviewed-by: Adam Langley <agl@google.com>
This function's behavior differs from all the other lastpos functions.
It does not appear to be used anywhere, so remove it. (lastpos = -1
returns the first match, lastpos = -2 additionally fails if there are
duplicates, lastpos = -3 additionally fails if the attribute is
multiply-valued.)
Update-Note: X509at_get0_data_by_OBJ is removed. We found no callers of
this function.
Change-Id: I8547bac6626623e43827e2490f04850eb148e317
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48367
Reviewed-by: Adam Langley <agl@google.com>
The comments say that this should work, but it didn't. OpenSSL doesn't
have any documentation about this but from looking at the code it works
there. (Along with things like magic sections called “ENV” to get
environment variables, sigh.)
Change-Id: I538fbad57e6af37eee739de6d2643f554bfc5c79
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48386
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
lh_strhash mapped nullptr to zero. ec8c67dfbc switched CONF's use to
OPENSSL_strhash, which crashes on nullptr. But CONF depends on the
nullptr handling.
Change-Id: I131c752aa089fb99b01c9e406b6994f3a6236976
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48385
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
EVP_MD_nid, in OpenSSL, is the same as EVP_MD_type. EVP_MD_type seems to
be the preferred spelling, so put EVP_MD_nid in the deprecated bucket.
Also add an EVP_MD_do_all alias to EVP_MD_do_all_sorted.
Change-Id: I4e7b800902459ac5cb9ef0df65d73da94afdf927
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48365
Reviewed-by: Adam Langley <agl@google.com>
This cleans up the story with
https://boringssl-review.googlesource.com/c/boringssl/+/46164. None of
our exported functions mutate ASN1_OBJECTS, with the exception of
ASN1_OBJECT_free, the object reuse mode of c2i_ASN1_OBJECT, and their
callers. Those functions check flags to correctly handle static
ASN1_OBJECTs.
For now, I've kept the struct definition in crypto/asn1 even though
ASN1_OBJECT is partially in crypto/obj. Since we prefer to cut
dependencies to crypto/asn1, we probably should rearrange this later.
I've also, for now, kept crypto/asn1/internal.h at C-style comments,
though our style story here is weird. (Maybe it's time to clang-format
crypto/asn1 and crypto/x509? Patches from upstream rarely directly apply
anyway, since we're a mix of 1.0.2 and 1.1.1 in crypto/x509.)
Update-Note: ASN1_OBJECT is now opaque. Callers should use accessors.
Change-Id: I655e6bd8afda98a2d1e676c3abeb873aa8de6691
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48326
Reviewed-by: Adam Langley <agl@google.com>
Additionally decorate ipv4_from_asc and ipv6_from_asc with their
array lengths.
Bug: 419
Change-Id: I2bce182ac260b071f076434deadab4096d29b2b1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48265
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@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>
These macros aren't consumed by anything anymore.
Change-Id: Id9616fa0962ae0dbf27bc884c6883dcad9755eb2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48229
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We already had a test, but move it to asn1_test.cc since it's part of
the ASN.1 library. Also, since it's easy, test it using public APIs
rather than stack-allocating an ASN1_STRING.
Change-Id: Ic77494e6c8f74584d159a600e334416197761475
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's BIT STRING representation has two modes, one where it
implicitly trims trailing zeros and the other where the number of unused
bits is explicitly set. This means logic in ASN1_item_verify, or
elsewhere in callers, that checks flags and ASN1_STRING_length is
inconsistent with i2c_ASN1_BIT_STRING.
Add ASN1_BIT_STRING_num_bytes for code that needs to deal with X.509
using BIT STRING for some fields instead of OCTET STRING. Switch
ASN1_item_verify to it. Some external code does this too, so export it
as public API.
This is mostly a theoretical issue. All parsed BIT STRINGS use explicit
byte strings, and there are no APIs (apart from not-yet-opaquified
structs) to specify the ASN1_STRING in X509, etc., structures. We
intentionally made X509_set1_signature_value, etc., internally construct
the ASN1_STRING. Still having an API is more consistent and helps nudge
callers towards rejecting excess bits when they want bytes.
It may also be worth a public API for consistently accessing the bit
count. I've left it alone for now because I've not seen callers that
need it, and it saves worrying about bytes-to-bits overflows.
This also fixes a bug in the original version of the truncating logic
when the entire string was all zeros, and const-corrects a few
parameters.
Change-Id: I9d29842a3d3264b0cde61ca8cfea07d02177dbc2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48225
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@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 is a bit short of a name to take, and no one seems to be using
it. (OpenSSL has renamed it, but not unexported it.)
Change-Id: I0de74d4d4812678ac3b1ec4b1b126a7748fe952b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48129
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Bug: chromium:1221591
Change-Id: Ie8335e53b107ba019a1bde62c12f846802e056c5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48165
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@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>
See also upstream's 9689a6aeed4ef7a2357cb95191b4313175440e4c.
X509_VERIFY_PARAM_ID made sense as a separate structure when
X509_VERIFY_PARAM was public, but now the struct is unexported.
Change-Id: I93bac64d33b76aa020fae07bba71b04f1505fdc4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48128
Reviewed-by: Adam Langley <agl@google.com>
This is a reland of 160a8891ae with go.mod
and go.sum fixed. This updates golang.org/x/crypto, adds the latest
golang.org/x/net as a direct dependency (it was previously an indirect
dependency via x/crypto), and cleans up stale entries from go.sum with
go mod tidy.
Original change's description:
> Add util/fetch_ech_config_list.go
>
> I wrote this tool to make it easier to test the ECH client against
> real-world servers with the bssl client tool. I found that manually
> extracting an ECHConfigList from a raw HTTPS record is unnecessarily
> painful.
>
> The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS
> records in the response, it attempts to extract an ECHConfigList from
> the "ech" SvcParam. It can write each extracted ECHConfigList to a file
> in a given directory. Once the ECH client implementation lands, the bssl
> client tool should have a new flag that that takes the path to an
> ECHConfigList file.
>
> I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I
> recently added the |UnknownResource| type to this library to enable
> callers (like us) to extract the bytes of otherwise-unsupported records
> (like HTTPS). I updated the dependency with `go get -u golang.org/x/net`.
>
> Although the bssl client tool knows how to resolve the address of its
> "-connect" parameter, it is difficult to query HTTPS records in a
> platform-agnostic way. If we decide the bssl client should directly
> query HTTPS rather than leaning on fetch_ech_config_list.go, we should
> look into libresolv. Specifically, the |res_query| function enables the
> caller to query arbitrary record types. This may open its own can of
> cross-platform worms; macOS and Linux typically ship with different
> implementations and it is not available on Windows. For more info, see
> `man 3 resolver`.
>
> Bug: 275
> Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b
> Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104
> Reviewed-by: David Benjamin <davidben@google.com>
Bug: 275
Change-Id: I9571e96c7a2ad7e239d86a353929a4e556d71287
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48106
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts commit 160a8891ae.
Reason for revert: This broke go.sum on CI for some reason. Will fix
and reland.
Original change's description:
> Add util/fetch_ech_config_list.go
>
> I wrote this tool to make it easier to test the ECH client against
> real-world servers with the bssl client tool. I found that manually
> extracting an ECHConfigList from a raw HTTPS record is unnecessarily
> painful.
>
> The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS
> records in the response, it attempts to extract an ECHConfigList from
> the "ech" SvcParam. It can write each extracted ECHConfigList to a file
> in a given directory. Once the ECH client implementation lands, the bssl
> client tool should have a new flag that that takes the path to an
> ECHConfigList file.
>
> I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I
> recently added the |UnknownResource| type to this library to enable
> callers (like us) to extract the bytes of otherwise-unsupported records
> (like HTTPS). I updated the dependency with `go get -u golang.org/x/net`.
>
> Although the bssl client tool knows how to resolve the address of its
> "-connect" parameter, it is difficult to query HTTPS records in a
> platform-agnostic way. If we decide the bssl client should directly
> query HTTPS rather than leaning on fetch_ech_config_list.go, we should
> look into libresolv. Specifically, the |res_query| function enables the
> caller to query arbitrary record types. This may open its own can of
> cross-platform worms; macOS and Linux typically ship with different
> implementations and it is not available on Windows. For more info, see
> `man 3 resolver`.
>
> Bug: 275
> Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b
> Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104
> Reviewed-by: David Benjamin <davidben@google.com>
TBR=davidben@google.com,dmcardle@google.com
Change-Id: Iec36265dfa3b7c59eb811ed708219bfebb07a710
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 275
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48105
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I wrote this tool to make it easier to test the ECH client against
real-world servers with the bssl client tool. I found that manually
extracting an ECHConfigList from a raw HTTPS record is unnecessarily
painful.
The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS
records in the response, it attempts to extract an ECHConfigList from
the "ech" SvcParam. It can write each extracted ECHConfigList to a file
in a given directory. Once the ECH client implementation lands, the bssl
client tool should have a new flag that that takes the path to an
ECHConfigList file.
I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I
recently added the |UnknownResource| type to this library to enable
callers (like us) to extract the bytes of otherwise-unsupported records
(like HTTPS). I updated the dependency with `go get -u golang.org/x/net`.
Although the bssl client tool knows how to resolve the address of its
"-connect" parameter, it is difficult to query HTTPS records in a
platform-agnostic way. If we decide the bssl client should directly
query HTTPS rather than leaning on fetch_ech_config_list.go, we should
look into libresolv. Specifically, the |res_query| function enables the
caller to query arbitrary record types. This may open its own can of
cross-platform worms; macOS and Linux typically ship with different
implementations and it is not available on Windows. For more info, see
`man 3 resolver`.
Bug: 275
Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104
Reviewed-by: David Benjamin <davidben@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>
This comment dates to SSLeay. It appears to be describing the
incremental trial division strategy where they would pick a starting
candidate, compute moduli by small primes, and then update by
incrementing the candidate and saved moduli instead of dividing from
scratch. We use a simpler rejection sampling strategy.
Change-Id: If2203d616f2b1f632bcd7033ceb60a83d1b75674
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48047
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>
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>
David Benjamin
Adam Langley
Giorgio Marinelli
Peter Kasting
Dan McArdle