Instead of having a pair of bespoke build definitions use the standard
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION toggle. We actually originated
the idea of a fuzzing-specific build toggle, and then libFuzzer
standardized a toggle when we talked to them about what we were doing.
The problem is our fuzzer mode toggle substantially changed the TLS
stack behavior, such that downstream code would likely go haywire. So we
couldn't easily fold into the standard one, and all of BoringSSL's
downstream fuzzer builds were messy.
Instead, make a few changes:
1. Switch BORINGSSL_UNSAFE_DETERMINISTIC_MODE to
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. That flag is not expected
to cause downstream issues as it just makes the PRNG deterministic.
2. Replace BORINGSSL_UNSAFE_FUZZER_MODE with a runtime toggle that is
only available when building with
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.
3. Instead of the no_fuzzer_mode fuzzers being special corpora for the
client and server fuzzers, they're now just separate fuzzerrs and
follow the usual naming conventions between fuzzers and their
corpora.
Update-Note: Downstream fuzzer builds can now be simplified. If the
fuzzing infrastructure already builds with
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION, the separate boringssl_fuzz
(or whatever) target can be removed.
Bug: 42290128
Change-Id: Ia1e479777f366908951e15067c96c9767c229f0a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/77749
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Applications sometimes need to create SSL_CLIENT_HELLO objects for
testing. We already have this function internally. Just expose it as
public API.
The only non-trivial change here is I've moved the error queue bits from
the caller of ssl_client_hello_init to inside SSL_parse_client_hello.
This matches our conventions a bit better (usually functions are
responsible for originating the error), and keeps ssl.h's public APIs
consistently using the error queue here.
Bug: 395069491
Change-Id: I16fb35772a61e98d2621f781e475aa5611b13582
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76128
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
We use the standard Apache 2.0 file header, described in "APPENDIX: How
to apply the Apache License to your work."
This was primarily automated by running:
git ls-tree -r --name-only HEAD | xargs go run ./util/relicense.go
See go/boringssl-relicensing-triage for the results of triaging the
output of the tool.
As part of this, switch from taking fiat-crypto under MIT license to
Apache 2.0. (It is licensed under MIT OR Apache-2.0 OR BSD-1-Clause.)
The copyright_summary tool can also be used to confirm we didn't
accidentally drop any copyright lines:
# Run before the CL
git grep -l Copyright | xargs go run ./util/copyright_summary.go -out /tmp/old.json
# Run after the CL
git grep -l Copyright | xargs go run ./util/copyright_summary.go -compare /tmp/old.json
Bug: 364634028
Change-Id: I17c50e761e9d077a1f92e25969e50ed35e320c59
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75852
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Following the guidance in
https://opensource.google/documentation/reference/releasing/authors,
start maintaining an AUTHORS file.
Update all existing Google copyright lines to 'The BoringSSL Authors'
per the document. This CL also changes the styling to match the new
guidance: removed the '(c)' and the comma.
All other existing copyright lines are left unmodified. Going forward,
our preference will be that new contributions to BoringSSL use 'The
BoringSSL Authors', optionally adding to the AUTHORS file if the
contributor desires.
To avoid being presumptuous, this CL does *not* proactively list every
past contributor in the BoringSSL half of the AUTHORS file. Past
contributors are welcome to send us a patch to be added, or request that
we add you. (Listed or not, the commit log continues to be a more
accurate record, and any existing non-Google copyright lines were left
unmodified.)
The OpenSSL half of the AUTHORS file is seeded with the contents of the
current OpenSSL AUTHORS file, as of writing. The current contents in the
latest revision of the 1.1.1 branch
(b372b1f76450acdfed1e2301a39810146e28b02c) and master
(d992e8729ee38b082482dc010e090bb20d1c7bd5) are identical, just formatted
in text vs Markdown.
Note when reviewing: CONTRIBUTING.md and AUTHORS contain non-mechanical
changes.
Bug: 364634028
Change-Id: I319d0ee63ec021ad85e248e8e3304b9cf9566681
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74149
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
I would be surprised if this were ever used by anyone, but we should
either make it work, or add code to turn the feature off when
configured. All the things that prevented it from working we just bugs,
so it was easier to just fix this and enable all the tests.
The main nuisance is how our in-memory representation reflects the old
DTLS 1.2 header, conflicting ECH's many uses of the transcript. (We
really should switch the in-memory representation.) Then some bits of
ClientHello processing needed to not lose track of the cookie.
(This test already covers HelloVerifyRequest interactions because we
run through HelloVerifyRequest in DTLS 1.2.)
Bug: 42290594
Change-Id: Ic09b78ddc0c5524ffc6c5b965eafab881eff0a0f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73729
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
C++ is fun and has two notions of "default" initialization, `new T` and
`new T()`. These are default initialization and value initialization,
respectively.
They are identical except that POD types are uninit when
default-initialized and zero when value-initialized. InplaceVector
picked the safer option by default and called the other one
FooMaybeUninit. Array is older and uses the less safe one (it's almost
always the one we want; we usually allocate an array to immediately fill
it).
While MaybeUninit does capture what you do with it, it is slightly
ambiguous, as seen in Array's internal implementation: uninitialized
could also mean we haven't gotten around to initialize it at all. I.e.
we need to use a function like std::uninitialized_value_construct_n
instead of normal functions in <algorithm>.
C++20 has std::make_unique and std::make_unique_for_overwrite to capture
the two. This seems as fine a naming convention as any, so switch to it.
Along the way, make the internal bssl::Array default to the safer one.
This lets us remove a couple of memset(0)'s.
Change-Id: I32cede231da051a854e6251e10b87f8e4dd06ee6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72268
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
This is still a bit more tedious than I'd like, but we've got three of
these and I'm about to add a fourth. Add something like Chromium's base
class. But where Chromium integrates the base class directly with
scoped_refptr (giving a place for a static_assert that you did the
subclassing right), we don't quite have that since we need to integrate
with the external C API.
Instead, use the "passkey" pattern and have RefCounted<T>'s protected
constructor take a struct that only T can construct. The passkey ensures
that only T can construct RefCounted<T>, and the protectedness ensures
that T subclassed RefCounted<T>. (I think the latter already comes from
the static_cast in DecRef, but may as well.)
Change-Id: Icf4cbc7d4168010ee46dfa3a7b0a2e7c20aaf383
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66369
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Remove all the other ERR_R_MALLOC_FAILURES from the
codebase.
Also changes cbb to push to the error stack, to correctly
report cbb failures instead of now only reporting
malloc failures. Previously it turned all cbb failures
into a malloc failure
Bug: 564
Change-Id: Ic13208bf9d9aaa470e83b2f15782fc94946bbc7b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57046
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
and isxdigit.
All of these can be affected by locale, and although we weren't using
them directly (except for isxdigit) we instead had manual versions inline
everywhere.
While I am here add OPENSSL_fromxdigit and deduplicate a bunch of code
in hex decoders pulling out a hex value.
Change-Id: Ie75a4fba0f043208c50b0bb14174516462c89673
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56648
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
In testing out the ECH bits on the Chromium side, it is much harder to
tell what's going on without some indication that we sent a
ClientHelloInner. This CL routes it into the callback. A corresponding
CL in Chromium will add it to NetLog.
Bug: 275
Change-Id: I945ab2679614583e875a0ba90d6cf1481ed315d9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51205
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The ECH extension is not covered in the AAD and so should not be
referenced in ech_outer_extensions. We end up rejecting this anyway when
checking for valid ClientHelloInners, but better to reject this
explicitly, as the spec suggests.
As part of this, use the more specific error in the various tests, so we
can distinguish the two cases. (DECODE_ERROR is coming from an extra,
probably unnecessary, error in ssl_decode_client_hello_inner's caller.)
Bug: 275
Change-Id: Ibeff55e5e1b7646ce9c68c5847cd1b40a47e6480
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51185
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@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>
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>
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>
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>
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>
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>
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 misread (or maybe it changed?) the draft padding scheme. The current
text does not round the whole payload to a multiple of 32, just the
server name as a fallback. Switch the GREASE size selection to match.
Although, we may want to change the draft here. See also
https://github.com/tlswg/draft-ietf-tls-esni/issues/433
While I'm here, update some references from draft-09 to draft-10. Also
make the comment less verbose.
Bug: 275
Change-Id: I3c9f34159890bc3b7d71f6877f34b895bc7f9b17
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47644
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This introduces an EVP_HPKE_KEM, to capture the KEM choice, and
EVP_HPKE_KEY, to capture the key import (and thus avoids asking
receivers to pass in the full keypair). It is a bit more wordy now, but
we'll be in a better place when some non-TLS user inevitably asks for a
P-256 version.
Bug: 410
Change-Id: Icb9cc8b028e6d1f86e6d8adb31ebf1f975181675
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47329
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This replaces the ID-based API with one that is more static linker
friendly. For ECH, it doesn't make a difference because we currently
pull in all the options we've implemented. But this means other HPKE
uses need not pull in everything ECH needs and vice versa.
Along the way, fix an inconsistency: we prefixed all the AEAD constants
with "AEAD", but not the others. Since the rest of the name already
determines everything, go with the shorter version.
Bug: 410
Change-Id: I56e46c13b43c97e15eeb45204cde7019dd21e250
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47327
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@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>
David Benjamin