This applies the OpenSSL "copyright consolidation" process from the
following upstream changes:
* e0a651945c
* 3fb2cf1ad1
* ac3d0e1377
* c2f312f5c2
* 596d6b7e1c
* e18cf66aaf
* 846e33c729
* 440e5d805f
* 21dcbebc6e
* 6286757141
* 4f22f40507
* d2e9e32018
* 2039c421b0
* b1322259d9
* aa6bb1352b
* b6cff313cb
* 9e20068958
* 6aa36e8e5a
* 44c8a5e2b9
This was mostly automated, but partially manual. The automated portion
can be reproduced by checking OpenSSL to commit
44c8a5e2b9af8909844cc002c53049311634b314, and running the following:
git grep -l -E 'Copyright remains Eric Young|Copyright.*The OpenSSL Project\.|Written by.*for the OpenSSL Project' crypto/ decrepit/ include/ ssl/ | grep -v objects.go > files.txt
cat files.txt | xargs -n1 perl -i ./util/copyright.pl
From there, some years were fixed up manually according to
go/openssl-copyright-consolidation-comparison (internal-only).
Three files required additional manual fixing:
- crypto/ecdh_extra/ecdh_extra.cc
- crypto/fipsmodule/ecdh/ecdh.cc.inc
- include/openssl/ecdh.h
These files have an OpenSSL header, but *after* a different header, so
the script does not correctly detect the now redundant OpenSSL header.
They were manually modified to remove it. This matches what seems to
have been done to crypto/ec/ecdh_ossl.c in OpenSSL's
4f22f40507fea3f272637eb8e00cadf1f34b10d9.
Bug: 364634028
Change-Id: I79a559a409ebe2476f2cb8a48a488ac5dd77c90a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74710
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It turns out to be useful for tests to be able to read this value back.
Change-Id: Icf21144c230dc59f7548b7f75749509c8b646b4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74508
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: 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>
I believe our implementation is interoperable at this point.
Bug: 42290594
Change-Id: Id802b626a3028a3f2d4e89dfd3fcb69b51572b7d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73650
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
This CL addresses a number of issues:
1. If an ACK or retransmit hit SSL_ERROR_WANT_WRITE, we do not provide a
way for the caller to retry it.
2. If the retransmit timer fires too many times, we do not track the
fatal error. Instead, we return an error out of the
DTLSv1_handle_timeout and leave the timeout unhandled. If the caller
does not tear down the connection in response to
DTLSv1_handle_timeout failing, they may register a timeout for 0s,
wake up again and loop forever.
This happened in https://crbug.com/42224241. I filed
https://crbug.com/42290595 to track fixing this API wart.
3. When the DTLS open_app_data hook needs to retransmit or send an ACK,
we write immediately in that callback, but the eventual aim was to
provide an in-place encrypt/decrypt API, so those hooks should not
perform I/O.
DTLS 1.3 adds a lot more cases where we'll want to do this:
- We might read a message and then want to ACK on a timer
- We might read a message and then want to ACK immediately
- We might read a past retransmit and then want to retransmit
the next flight
- We might read an ACK, clear the current flight, and then want to
send a queued up KeyUpdate that was waiting on it.
To address all these, this CL rearranges things slightly. Now the DTLS
record layer keeps track of whether it owes the peer an ACK and a
flight. If so, the flush() operation will try to write those out. If it
fails, it keeps track of how far it got. If it succeeds, it clears those
flags.
Various operations can set those flags, notably the timeouts. This means
the actual "handle timeout" part of DTLSv1_handle_timeout is
infallible. Handling a timeout means cashing the timeout expiry in for
setting the flag.
From there, we drive flush() to completion where we need to. For now,
I've added it to the handshake and SSL_read(), though I'm not sure if
I've gotten exactly the right spots. (Should we also flush on
SSL_write?) We could also go further and have flush() drive
handle_timeout(), and then handle_timeout() can be deprecated
altogether. The model is just "you must retry this operation after the
timeout, in addition to the SSL_ERROR_WANT_WHATEVER you got". That's
quite attractive, but for now I haven't quite gone that far.
Another complication is that flush() currently behaves very differently
between TLS and DTLS. In DTLS, I had to add an explicit finish_flight()
to queue up the flight for sending. If you leave things in there, it's
no big deal because everything in DTLS is unordered. In TLS, everything
is ordered, so once we've encrypted records, we have to flush them
regardless. To that end, in TLS we generally like to flush things
implicitly on write, because that's when the caller is expecting I/O
anyway, and we can concatenate the, say, encrypted KeyUpdate with the
encrypted application data. Not sure what's best to do there. For now
I've just gated flush() calls on SSL_is_dtls().
I'm sure we'll find this also isn't quite right and rearrange it again
later, but hopefully this works a bit better than what we had before.
Bug: 42290594, 376718254
Fixed: 42290595
Change-Id: I4d9b5c753530b514a1bc5e4e69ddb25dfc8c1d06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73527
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Adds some symmetry with SSL_is_dtls and all the quic_method != nullptr
and quic_method == nullptr checks were getting really really hard to
follow.
Change-Id: I529e5856bd6026e7fb530d435fcfd9a6dc6158a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73427
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
This removes noise from the SPAKE2+ CL.
Change-Id: I6038bf02e80295f58ad955db47e4687547de8dbe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73807
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Back when we were deploying TLS 1.3, we were tracking how often some
buggy middlebox interfered with the new TLS version and prevented sites
from upgrading.
One middlebox's failure mode was that they dropped ServerHello and
passed the remaining records through. This error code was caught by
Chromium to histogram. TLS 1.3 has long since been deployed, and we've
dropped that histogramming code, so remove this special case.
See https://crbug.com/41325349 for some of the history here.
Change-Id: I70c1ada1ed3d9ba73dfe75213a0233e407599c98
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73548
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
The requirement to support BIO_flush is not obvious and should be
written down.
Change-Id: Ib046898c9a49a6ffe51ee3c32982db3854095db5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73467
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We'll need to carry a couple of timers with DTLS 1.3. Abstract this into
a DTLSTimer.
Bug: 42290594
Change-Id: I4d57dfae9c5984cb10f5db251642af5aaec9a495
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72951
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
While DTLS 1.2 recommended 1 second, it's 2024 and RTTs are generally
much lower. I believe most of our important uses already reconfigure
this, but let's default to something better.
Update-Note: The default DTLS timer is now slightly lower.
Bug: 42290594
Change-Id: Iec3f01395ac0c3c03cdfd951cc14acddb40ce72f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72868
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: Nick Harper <nharper@chromium.org>
TLS 1.2 and TLS 1.3 both support some mechanism for the server to change
its mind at the last minute and not send a ticket. In TLS 1.2, you leave
the ticket field empty. In TLS 1.3, you just don't send NewSessionTicket
in the first place.
We have two ticket encryption callbacks, the old "ticket key callback"
and the "ticket AEAD callback".
The ticket key callback has no way to trigger this behavior, but it has
an unused zero return that could be repurposed for this, in principle.
Back in 2016, OpenSSL did so in
https://github.com/openssl/openssl/pull/1098
The ticket AEAD callback could, in principle, support this by
successfully returning the empty string. This goes against the current
documented API contract, but it accidentally worked in TLS 1.2. In TLS
1.3, it caused our servers to send a syntax error.
The QUIC folks want this behavior and used to accidentally trigger the
syntax error, and have switched to returning some garbage placeholder
string. Between that, the new OpenSSL behavior, and it accidentally
working in TLS 1.2 anyway, let's just go ahead and support this.
To aid in version skew messes, this also bumps BORINGSSL_API_VERSION.
Change-Id: Id4fef7b9aa552dc8e927f89a5d746bdd2247e1c6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73028
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We never actually tested it in runner, though I think we did write
ssl_test.cc tests. Adding some tests now to prepare for adding and
testing some new behavior shortly.
Also fix the documentation for SSL_CTX_set_tlsext_ticket_key_cb
returning zero in the encrypt case.
Change-Id: Ic7b8d51b2433d56cc524f62565946b906d515257
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73027
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: We used to say you only need to drive the timer when
SSL_do_handshake asked for SSL_ERROR_WANT_READ. Change to just say you
are always obligated to drive the timer. Callers will need to adapt to
this before enabling DTLS 1.3.
Bug: 42290594
Change-Id: Iaf05eb608d3f4925cfafe243ae590e29e6526ff7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72848
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Add tests to ssl_test.cc to check basic combinations
of credential matching with the CA extension, and
with credentials both marked to match against
issuers, and a default credential that will match
anything.
Change-Id: Ic41a2dd0ecbbdc4c1be05e440d4673c087b52775
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71613
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Default reads as if it is somehow treated special, as a default when
fancier heuristics do not match, such as trust anchors. Such a concept
does not make sense because servers will have *multiple* fallbacks for
trust anchor matching, e.g. an ECDSA one and an RSA one. Nonetheless,
this was a bit confusing, so call it the "legacy credential" to make it
clearer this is just about whether you use a different API.
Change-Id: Ieb8cf454d1b2e0bf08a2c78cbea644d4f2f12202
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71727
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This is implemented by looking at the saved current read and write
secrets. That state is used by KeyUpdate and this logic.
As part of tidying up the epoch state for DTLS 1.3, I ran into that
state because DTLS does not have a single current read/write secret. But
it also isn't ideal for QUIC. For QUIC, the problem is that QUIC drives
KeyUpdates outside of TLS, but that means we'll just hold on to the
initial traffic secrets in memory, which can derive all the rotated
ones.
So let's for now, just limit this API to TLS. We can decide later
whether to also allow it for DTLS (after very carefully defining what
the "current" epoch means). I don't think we'd ever allow it for QUIC
given how QUIC is intended to work.
(This change doesn't actually fix any of the internal storage, just
breaks the API that would leak it. Changing the internal storage will be
in later CLs.)
Bug: 42290608
Change-Id: I5d4b170a5a80a7cc0657a957ae20135d742891d2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71647
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
libssl's error-handling is one of the most difficult things to get right
with this API. Leave some more notes, in case the reader does not know
what "error queue" means.
Change-Id: I91464ccdc12bf9e05ac9ed61930bc733244a9b36
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70929
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
ML-KEM is now finalized, so uses of Kyber should migrate to ML-KEM. This
adds the new codepoint for TLS, X25519MLKEM768 from
draft-kwiatkowski-tls-ecdhe-mlkem-01.
One detail to call out: where the Kyber hybrid put X25519 first, this
one places MLKEM768 first. Section 3 of the draft discusses why.
Bug: 40910498
Change-Id: I18862cd5d25d6ab6c4b38514e8333684dc5e3778
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70547
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This policy implements a part of RFC 9151.
Change-Id: I2c0a0e922b9287c8e29bdce4633fdee2a9f07fb8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69887
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: Ib3ae75c55da8f4e71af201627810e01cfb348687
Bug: 715
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68027
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
This commit solves
https://bugs.chromium.org/p/boringssl/issues/detail?id=714. To
summarize, there are cases where servers will advertise ECH on hostnames
that may, in practice, be unable to actually negotiate e.g. TLS 1.3. To
gracefully handle this case, this commit adds a new return value for the
select_cert_cb that signals to the server that ECH must be disabled. To
accomplish this, we slightly rewind the state machine to instead
handshake with ClientHelloOuter, and clear ech_keys on the handshake
state such that the server hello does not include any retry_configs in
EncryptedExtensions. Clients will take this as a signal that ECH is
disabled on the hostname, and that they should instead handshake without
ECH.
Bug: 42290593
Change-Id: I1806ba052ffbc3e5c46161a1596d125cc5e5a8fc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69087
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Now that we don't depend on external CRYPTO_library_init calls or the
static initializer to initialize CPU capabilities, we can drop a ton of
code.
This makes CRYPTO_library_init, and all its wrappers, into no-ops and
drops the (non-FIPS) static initializer. I've added an internal
OPENSSL_init_cpuid function for the places where the library actually
needs to initialize the CPU vector.
Note this slightly changes the default, previously
static-initializer-full build: previously, CRYPTO_library_init was a
no-op and we relied on the static initializer. Now we uniformly use
CRYPTO_once. This should be an atomic read in the steady state and
essentially free. We can restore the static initializer by default if
this ends up being a problem, but having only one mode is more
straightforward. This also avoids problems if an application calls into
BoringSSL during its own static initializer. Static initializers are not
coherently ordered.
Update-Note: The BORINGSSL_NO_STATIC_INITIALIZER build option and
CRYPTO_library_init are now unnecessary. Once updating past this
revision, those options can now be cleaned up from downstream projects.
Fixed: 40644931
Change-Id: Idc2e6ea7a73d6352e0360fd886c46d88dba3568c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69508
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
See draft-ietf-tls13-pkcs1-00. The code point is disabled by default
and must be configured in SSL_set_verify_algorithm_prefs and
SSL_set_signing_algorithm_prefs. It is also only defined for TLS 1.3
client certificates and otherwise ignored.
This required reworking the tests a bit since this is the first
signature algorithm that's disabled by default, and the first algorithm
that behaves differently between client and server.
Bug: 347047841
Change-Id: If4f653a456799ed9f0173159da291a9b6b6556fb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69368
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
SSL_select_next_proto has some fairly complex preconditions:
- The peer and supported list must be valid protocol lists
- The supported list must not be empty. The peer list may be empty due
to one of NPN's edge cases.
In the context of how this function is meant to be used, these are
reasonable preconditions. The caller should not serialize its own list
wrong, and it makes no sense to try to negotiate a protocol when you
don't support any protocols. In particular, it complicates NPN's weird
"opportunistic" protocol.
However, the preconditions are unchecked and a bit subtle. Violating
them will result in memory errors. Bad syntax on the protocol lists is
mostly not a concern (you should encode your own list correctly and the
library checks the peer's list), but the second rule is somewhat of a
mess in practice:
Despite having the same precondition in reality, OpenSSL did not
document this. Their documentation implies things which are impossible
without this precondition, but they forgot to actually write down the
precondition. There's an added complexity that OpenSSL never updated the
parameter names to match the role reversal between ALPN and NPN.
There are thus a few cases where a buggy caller may pass an empty
"supported" list.
- An NPN client called SSL_select_next_proto despite not actually
supporting any NPN protocols.
- An NPN client called SSL_select_next_proto, flipped the parameters,
and the server advertised no protocols.
- An ALPN server called SSL_select_next_proto, passed its own list in as
the second parameter, despite not actually supporting any ALPN
protocols.
In these scenarios, the "opportunistic" protocol returned in the
OPENSSL_NPN_NO_OVERLAP case will be out of bounds. If the caller
discards it, this does not matter. If the caller returns it through the
NPN or ALPN selection callback, they have a problem. ALPN servers are
expected to discard it, though some may be buggy. NPN clients may
implement either behavior.
Older versions of some callers have exhibited variations on the above
mistakes, so empirically folks don't always get it right. OpenSSL's
wrong documentation also does not help matters. Instead, have
SSL_select_next_proto just check these preconditions. That is not a
performance-sensitive function and these preconditions are easy to
check. While I'm here, rewrite it with CBS so it is much more
straightforwardly correct.
What to return when the preconditions fail is tricky, but we need to
output *some* protocol, so we output the empty protocol. This, per the
previous test and doc fixes, is actually fine in NPN, so one of the
above buggy callers is not retroactively made OK. But it is not fine in
ALPN, so we still need to document that callers need to avoid this
state. To that end, revamp the documentation a bit.
Thanks to Joe Birr-Pixton for reporting this!
Fixed: 735
Change-Id: I4378a082385e7334e6abaa6705e6b15d6843f6c5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69028
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
NPN is a little odd, owing to being a three-step process. The client
offers NPN, then the server accepts NPN and offers a list of protocols,
then the client picks a protocol.
The server is permitted to accept NPN but then offer zero supported
protocols. This worked, but was not tested or clearly documented.
In the last step, the client *must* pick a protocol, but it is permitted
to pick the empty string. The semantics of this are not explicitly
stated in the draft, but one can infer it means we aren't picking a
protocol. This also worked but was not tested or clearly documented.
Change-Id: I26d7089f4902834ff68a97467fc826e957d5fdf8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69027
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We encode the secrets in hex. When we do so, we should not leak them
based on memory access patterns. Of course, the caller is presumably
going to leak them anyway, because this is the SSLKEYLOGFILE callback.
But it's plausible that the caller might have registered the callback
unconditionally and then, in the callback, decide whether to discard the
data. In that case, we should not introduce a side channel.
Change-Id: If6358a3081c658038232b4610603967cb38659b4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67829
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We actually don't test this at all right now.
Change-Id: Iaac8850da3c012cbd21d0f38b026e7ff14db3650
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67828
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Make google3 common typos stop complaining in the future.
Change-Id: Ib5156335afa691427dbe618c8b29797665cef35f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66947
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Field-by-field setters make the worst APIs. This fixes the following:
- Calling SSL_CTX_set_chain_and_key twice should override the old one
(Regression from SSL_CREDENTIAL.)
- Various APIs forgot to clear the old chain before appending new ones.
(Regression from SSL_CREDENTIAL.)
- Switching between a custom private key and a concrete one should not
leave the old one lying around. (I think this was always broken.)
Add tests for all of these cases.
Change-Id: Ief7b3aecf2ada3b123d79d4eddf464c65d5f7d0d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66907
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This adds a notion of "credentials" to BoringSSL's API, to support
certificate selection by key type (typically ECDSA vs RSA), though the
aim is for it to be generalizable to other certificate types and other
kinds of selection criteria, such as Trust Expressions, or Merkle Tree
Certificates. Since we already had some nascent delegated credentials
I've reworked that feature with SSL_CREDENTIALs as well.
The model is that you create an SSL_CREDENTIAL object containing all the
configuration for what you are authenticating as. An X.509
SSL_CREDENTIAL has a certificate chain, private key, optionally an OCSP
response and SCT list. Delegated credentials are similar. In the future,
we might use this for raw public keys, other certificate types, etc.
Once you set those up, you configure those on the SSL or SSL_CTX in
preference order, and BoringSSL will internally pick the first one that
is usable.
The current implementation ends up redundantly selecting the signature
algorithm a couple of times. This works but is a little goofy. A
follow-up change will remove this redundancy. The protocol between the
runner and shim for tests is also a little weird, but it was the easiest
way I could think of for injecting that. Long-term, I think we should
just replace that protocol with a JSON structure. (See
https://crbug.com/boringssl/704.)
As split handshakes are in the process of being replaced with handshake
hints, this won't work with split handshakes. It works with handshake
hints without any extra work.
Update-Note: The delegated credentials API has been revamped.
Previously, it worked by configuring an optional delegated credential
and key with your normal certificate chain. This has the side effect of
forcing your DC issuer and your fallback certificate to be the same. The
SSL_CREDENTIAL API lifts this restriction.
A delegated credential is now just a different kind of credential. It
may use the same certificate chain as an X.509 credential or be
completely separate. All the SSL_CREDENTIAL APIs take CRYPTO_BUFFERs,
so, if common, the buffers may be shared to reduce memory.
The SSL_delegated_credential_used API is also removed, in favor of the
more general SSL_get0_selected_credential API. Callers can use ex_data
or pointer equality to identify the credential.
Bug: 249
Change-Id: Ibc290df3b7b95f148df12625e41cf55c50566602
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66690
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
In TLS 1.2 and below, the supported_curves list simultaneously contrains
ECDH and ECDSA. Since BoringSSL, previously, did not handle ECDSA
certificate selection in the library, we ignored the latter and left it
to the callers. If configured with an ECDSA certificate that didn't
match the peer's curve list, we proceeded anyway, and left it to the
client to reject the connection.
This contradicts RFC 8422, which says:
The server constructs an appropriate certificate chain and conveys it
to the client in the Certificate message. If the client has used a
Supported Elliptic Curves Extension, the public key in the server's
certificate MUST respect the client's choice of elliptic curves. A
server that cannot satisfy this requirement MUST NOT choose an ECC
cipher suite in its ServerHello message.)
As with the previous client certificate change, once we move certificate
selection into the library, we'll need to evaluate this ourselves. A
natural implementation of it will, as a side effect, cause us to enforce
this match, even when only a single certificate is configured. This CL
lands that behavior change ahead of time and, in case there are
compatibility impats, leaves a flag, SSL_set_check_ecdsa_curve, to
restore the old behavior. If the change goes through fine, we can retire
the flag after a few months.
If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, but these all result in some slightly suboptimal behavior,
so I think we should treat them as contingency plans.
To help debugging, I gave this a dedicated error, though doing so is a
little tricky because of the PSK fallback. (See the
CheckECDSACurve-PSK-TLS12 test.)
Update-Note: A TLS 1.2 (or below) server, using an ECDSA certificate,
connecting to a client which doesn't advertise its ECDSA curve will now
fail the connection slightly earlier, rather than sending the
certificate and waiting for the client to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_WRONG_CURVE. If the client was buggy and did not correctly
advertise its own capabilities, this may cause a connection to fail
despite previously succeeding. We have included a temporary API,
SSL_set_check_ecdsa_curve, to disable this behavior in the event this
has any impact, but please contact the BoringSSL team if you need it,
as it will interfere with improvements down the line.
TLS 1.3 is not impacted by this change, neither are clients, or RSA
certificiates. Additionally, if your server was already looking at the
curve list before configuring an ECDSA certificate in TLS 1.2, this
will also have no impact.
Bug: 249
Change-Id: I2f1d4e2627641319556847cbbbcdddf347bbc8a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66688
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
TLS <= 1.2 servers indicate supported client certificate key types with
a certificate_types field in the CertificateRequest. Historically, we've
just ignored this field, because we've always outsourced certificate
selection to the caller anyway. This meant that, if you configured an
RSA client certificate in response to a server that requested only ECDSA
certificates, we would happily send the certificate and leave it to the
server to decide if it was happy.
Strictly speaking, this was in violation of RFC 5246:
- The end-entity certificate provided by the client MUST contain a
key that is compatible with certificate_types. [...]
Although prior TLS versions didn't say anything useful about this either
way.
Once we move certificate selection into the library, we'll want to start
evaluating supported algorithms ourselves. A natural implementation of
it will, as a side effect, cause us to enforce this match, even when
only a single certificate is configured. Since this is unlikely to have
any real compatibility impact (every TLS server I've seen just hardcodes
this list), let's just try turning it on. On the off chance it does
break someone, I've left a flag, SSL_set_check_client_certificate_type,
for folks to turn this check off. The flag will most likely be
unnecessary, in which case we can retire it after a few months.
If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, or lean on the sigalgs list (doesn't work for 1.0/1.1), but
these all result in some slightly suboptimal behavior, so I think we
should treat them as contingency plans.
Update-Note: A TLS 1.2 (or below) client, using client certificates,
connecting to a TLS server which doesn't support its certificate type
will now fail the connection slightly earlier, rather than sending the
certificate and waiting for the server to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_UNKNOWN_CERTIFICATE_TYPE. If the server was buggy and did not
correctly advertise its own capabilities (very very unlikely), this may
cause a connection to fail despite previously succeeding. We have
included a temporary API, SSL_set_check_client_certificate_type, to
disable this behavior in the unlikely event this has any impact, but
please contact the BoringSSL team if you need it, as it will interfere
with improvements down the line.
This change does not affect servers requesting client certificates, only
clients sending them.
Bug: 249
Change-Id: I159bc444c4ee79fbe5c476d4253b48d58d2538be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66687
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This includes the somewhat odd "defaults" API, which I've currently left
kind of handwavy. We should eventually decide what to do with this, be
it remove it, decide /etc/ssl is a fine default, or do something else
entirely. But I'll leave that to future us.
(If nothing else, we really should make it return an error on Windows
and macOS. It's really just Linux where /etc/ssl is a plausible platform
API.)
Bug: 426
Change-Id: Iacd2bb903f452ffe236a7a0b97e3072b5dcd8516
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66388
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
It is not actually possible to configure an inconsistent certificate and
private key pair (short of mutating the objects after you've configured
them). The functions that configure certificates and private keys will
refuse to get CERT into an inconsistent state.
SSL_CTX_check_private_key is really just checking that you have a
certificate and private key at all. Some callers (notably pyOpenSSL's
tests) are written as if SSL_CTX_check_private_key does something more,
but that's only because they also configure certificate and private key
in the wrong order. If you configure the key first, configuring the
certificate silently drops the mismatched private key because OpenSSL
thinks you're overwriting an identity. SSL_CTX_check_private_key is
really just detecting this case.
Add tests for all this behavior, document that certificates should be
configured first, and then deprecate SSL_CTX_check_private_key because,
in the correct order, this function is superfluous.
This will get shuffled around with SSL_CREDENTIAL, so add some tests
first.
Bug: 249
Change-Id: I3fcc0f51add1826d581583b43ff003c0dea979dd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66447
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Although the comments say draft-03, we're currently on draft-06.
dcd6e447eb forgot to update all the
comments.
The final RFC is identical to draft-06, except
expected_cert_verify_algorithm was renamed to dc_cert_verify_algorithm,
so this is just changing comment and renaming something.
While I'm here, write the codepoint in decimal instead of hex, to match
the document and how the other IANA codepoints are written out.
Change-Id: I6d1f362a21eecafeef5bba5879f4158e31c8def4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66367
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
It's less bad than I originally wrote because trust properties only
matter if configured on the X509_STORE. Add a test for this.
This is good because lots of functions trigger d2i_X509_AUX, so I think
we have to assume attackers can specify these values. Nonetheless, this
is surprising, so document which functions trigger this.
Change-Id: I73ce44acfa2a373ef3f3ef09c3e46cea98124f33
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65791
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Re-ran clang-format on those regions, now that clang-format knows about
STACK_OF(T).
Change-Id: Ied350343b8aaef0dc25dbe3f215c1fae25af90c4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66147
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The vast majority of implementations of this callback empirically did
not understand what this API was for, and actually wanted
SSL_CTX_set_cert_verify_callback.
Change-Id: If7961db612932ac9e76ed8482a59442685ece4b5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65007
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Our documentation comments already include examples of code blocks
and lists, they just don't get rendered right. We also have things that
were trying to be lists but aren't. Go ahead and add support for it, and
fix the handful of list-like things that didn't get rendered as lists.
I took inspiration from CommonMark (https://spec.commonmark.org/0.30/)
to resolve questions such as whether blank lines are needed between
lists, etc., but this does not support any kind of nesting and is still
far from a CommonMark parser. Aligning with CommonMark leaves the door
open to pulling in a real Markdown parser if we start to need too many
features. I've also borrowed the "block" terminology from CommonMark.
One ambiguity of note: whether lists may interrupt paragraphs (i.e.
without a blank line in between) is a little thorny. If we say no, this
doesn't work:
Callers should heed the following warnings:
1) Don't use the function
2) Seriously, don't use this function
3) This function is a bad idea
But if we say yes, this renders wrong:
This function parses an X.509 certificate (see RFC
5280) into an X509 object.
We have examples of both in existing comments, though we could easily
add a blank line in the former or rewrap the latter. CommonMark has a
discussion on this in https://spec.commonmark.org/0.30/#lists
CommonMark says yes, but with a hack that only lists starting with 1 can
interrupt paragraphs. Since we're unlikely to cite RFC 1, I've matched
for now, but we may want to revisit this if it gets to be a pain. I
could imagine this becoming a problem:
This function, on success, does some stuff and returns
1. Otherwise, it returns 0.
But that looks a little weird and we usually spell out "one" and "zero".
I printed all the lists we detected in existing comments, and this has
not happened so far.
I've also required fewer spaces than CommonMark to trigger a code block.
CommonMark uses four, but four spaces plus a leading "//" and a " " is
quite a lot. For now I'm not stripping the spaces after the comment
marker at comment extraction time and then requiring three spaces, so
two spaces relative to normal text. This is mostly to match what we've
currently been doing, but we can always change it and our comments
later.
Change-Id: Ic61a8e93491ed96aba755aec2a5f32914bdc42ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64930
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL 1.1.0 included d9b8b89bec4480de3a10bdaf9425db371c19145b, which a
cleanup change to X509_verify_cert. This cleanup changed the semanitcs
of the depth limit. Previously, the depth limit omitted the leaf but
included the trust anchor. Now it omits both.
We forked a little before 1.0.2, so we still had the old behavior. Now
that the new behavior is well-established, switch to new one. Bump
BORINGSSL_API_VERSION so callers can detect one or the other as needed.
Document the new semantics. Also fix up some older docs where I implied
-1 was unlimited depth. Negative numbers were actually enforced as
negative numbers (which means only explicitly-trusted self-signed certs
worked).
Update-Note: The new semantics increase the limit by 1 compared to the
old ones. Thus this change should only accept more chains than
previously and be relatively safe. It also makes us more
OpenSSL-compatible. Envoy will need a tweak because they unit test the
boundary condition for the depth limit.
Bug: 426
Fixed: 459
Change-Id: Ifaa108b8135ea3d875f2ac1f2a3b2cd8a22aa323
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64707
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It is a little silly to have documented obscure verification internals
and not verification itself yet, but these were pretty easy.
X509_check_purpose and X509_check_trust should also go here, but I'll
do those when I've tackled more of X509_PURPOSE and X509_TRUST.
I haven't moved X509_CHECK_FLAG_* because those should go with
X509_VERIFY_PARAM_set_set_hostflags.
X509_check_akid seems to have no callers, so I unexported that one.
Bug: 426
Change-Id: I5af1824346db27fd52773ae27e943df89c1d5a87
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64630
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This change adds ALPS codepoint supports on split handshake.
serialize_features sends list of codepoints, handshaker will serialize
the codepoint when calling SSL_serialize_handback, and send the chosen
codepoint to the server, server can apply the codepoint when
SSL_apply_handback.
Change-Id: Id7bc985c4b9847b7c337595f1bc23b2af93d96e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63265
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Old version Chrome with the existing ALPS codepoint can potentially cause network error due to an arithmetic overflow bug in Chrome ALPS decoder (We already fixed the issues starting from M100 in Chrome).
This CL add a new codepoint for ALPS extension in a way that can be enabled on individual connections., To support multiple versions of Chrome, we need to support both codepoints in BoringSSL.
For details: https://docs.google.com/document/d/16pysbV_ym_qAau_DBYnrw2A4h5ve2212wfcoYASt52U
Change-Id: Iea7822e757d23009648febc8eaff1c91b0f06e18
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61125
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Handshake hints work fine with TLS 1.2 resumption now. Also split
handshakes is really really dangerous, and I think hints has survived
long enough that we can just declare it the successor.
Change-Id: Ib5fe5e1b030034b853a96c3404608c56d7b7a7c7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62925
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Like OPENSSL_NO_FILESYSTEM, keep us honest: if the symbol is missing,
don't declare it in the headers. This ensures folks aren't relying on
dead code elimination and then later break when they build in a context
where it doesn't happen.
Bug: 629
Change-Id: I3e56c3879e970aa8d0d6e0e5f1ad046d0f420ef0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61730
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Detecting errors (i.e. fs-less platforms using fs-only APIs) at compile
time is generally preferable to doing so at runtime, so
https://boringssl-review.googlesource.com/c/boringssl/+/61726 opted to
remove the APIs altogether on applicable targets.
However, Trusty uses rust-openssl somewhere and rust-openssl binds a
bunch of filesystem-dependent APIs unconditionally. To keep that
working, switch to a stub fopen when OPENSSL_NO_FILESYSTEM is set. We
effectively model a platform where the filesystem "exists", but is
empty. Upstream OpenSSL similarly has OPENSSL_NO_STDIO still define the
file BIO (unlike the socket BIO, which is excluded), but in a stub form.
As part of this, I've gone ahead and resolved one of the Trusty TODOs.
It does produce a duplicate symbol with [1], but things seem to link
fine in treehugger. In case it does break, I've bumped
BORINGSSL_API_VERSION, so we can go in and condition it if needed.
[1] https://android.googlesource.com/trusty/lib/+/refs/heads/main/lib/openssl-stubs/bio.c
Bug: 629
Change-Id: I4f20d872a7cde863d21c78090f270b77b03545fa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61925
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
David Benjamin
Adam Langley
Bob Beck
Adam Langley
Nick Harper
Rushil Mehra
Gabriel Redner
Victor Tan
Victor Tan