We have ssl_has_certificate and ssl_has_private_key calls scattered
throughout libssl, but they're never actually tested. The checks are
also a little subtle because of cert->chain's weird representation of
the leaf being missing but a chain configured.
In hindsight, possibly we should have made them separate fields, but
it's too late now. We'd have to get rid of SSL_CTX_get0_chain and
SSL_get0_chain. Normally we don't bother with these functions, under the
"you should know what you configured" theory, but one caller needed it
recently in
https://boringssl-review.googlesource.com/c/boringssl/+/66087
The tests also confirm that most of the ssl_has_private_key calls,
other than the one in ssl_has_certificate, are redundant. The
ssl_has_certificate calls are also in an odd place.
This will all get shuffled around with SSL_CREDENTIAL, so set up tests
first.
Bug: 249
Change-Id: If1bb7097a15649e593886c3c22e2cc829a853830
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66508
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This produces slightly nicer output, is less code, and helps us remember
to check both the library and reason code.
Change-Id: Ic49508accb0bc8a25cbb5b94cc7e4aeb1bd8cbd0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66507
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@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>
Avoid rewriting the FILE scoper, and deal with the Android problem in
one place. This header will also, in the next CL, be the home for a
temporary directory helper for hash_dir.
Change-Id: I4be69ef6c2ac3443b80ee8852bcce4078bf7f118
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66007
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
No one would ever do this, but it works today and we should keep it
working. As part of certificate selection, we'll be introducing an
SSL_CREDENTIAL object. In doing so, the existing APIs will mutate a
built-in "default" credential. If we made a shallow copy, it would break
things.
Bug: 249
Change-Id: I75b1486289659611184a42e87771a6cf7ddb5aa7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66372
Commit-Queue: David Benjamin <davidben@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>
OpenSSL added a similar helper function. It's very, very common for us
to malloc something an then zero it. This saves some effort. Also
replace some more malloc + memcpy pairs with memdup.
Change-Id: I1e765c8774a0d15742827c39a1f16df9748ef247
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63345
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: 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>
We've required C++14 for a while now. As we're mostly C with a little
C++, this is less helpful, but may as well avoid bare new where
possible.
Change-Id: Icf3386e3f3b6f2092bb0089ed874cc50985f1a40
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61429
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We added SSL_CIPHER_get_prf_nid to match the other SSL_CIPHER_get_*_nid
functions, but OpenSSL went with returning the EVP_MD instead.
rust-openssl uses this function, and all the callers of
SSL_CIPHER_get_prf_nid then call EVP_get_digestbynid anyway, so this
version is preferable.
Update-Note: This change is backwards-compatible, but we should update
the QUIC code to use this new function when OPENSSL_API_VERSION is
high enough. It has the benefit of not pulling in random other hash
functions like MD4.
Change-Id: Ied66a6f0adbd5d7d86257d9349c40a2830e3c7e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60606
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>
Right now we use NIDs to configure the group list, but group IDs (the
TLS codepoints) to return the negotiated group. The NIDs come from
OpenSSL, while the group ID was original our API. OpenSSL has since
added SSL_get_negotiated_group, but we don't implement it.
To add Kyber to QUIC, we'll need to add an API for configuring groups to
QUICHE. Carrying over our inconsistency into QUICHE's public API would
be unfortunate, so let's use this as the time to align things.
We could either align with OpenSSL and say NIDs are now the group
representation at the public API, or we could add a parallel group ID
API. (Or we could make a whole new SSL_NAMED_GROUP object to pattern
after SSL_CIPHER, which isn't wrong, but is even more new APIs.)
Aligning with OpenSSL would be fewer APIs, but NIDs aren't a great
representation. The numbers are ad-hoc and even diverge a bit between
OpenSSL and BoringSSL. The TLS codepoints are better to export out to
callers. Also QUICHE has exported the negotiated group using the
codepoints, so the natural solution would be to use codepoints on input
too.
Thus, this CL adds SSL_CTX_set1_group_ids and SSL_set1_group_ids. It
also rearranges the API docs slightly to put the group ID ones first,
and leaves a little note about the NID representation before introducing
those.
While I'm here, I've added SSL_get_negotiated_group. NGINX seems to use
it when available, so we may as well fill in that unnecessary
compatibility hole.
Bug: chromium:1442377
Change-Id: I47ca8ae52c274133f28da9893aed7fc70f942bf8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60208
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This adds "group" versions of our codepoint-based APIs. This is mostly
because it looks weird to switch terminology randomly in the
implementation. See I7a356793d36419fc668364c912ca7b4f5c6c23a2 for
additional context.
I've not bothered renaming the bssl_shim flags. That seems a waste of
time.
Change-Id: I566ad132f5a33d99efd8cb2bb8b76d9a6038c825
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60207
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We're this awkward mix of "group" and "curve" right now. On the spec
side, this is because they used to be "curves", but then RFC 7919
renamed to "group" in an attempt to generalize FFDH and ECDH. The
negotiated FFDH stuff never really went anywhere (the way it used cipher
suite values in TLS 1.2 made it unusable), but the name change stuck.
In our implementation and API, we originally called it "curve". In
preparation for TLS 1.3, we renamed the internals to "group" to match
the spec in
https://boringssl-review.googlesource.com/c/boringssl/+/7955, but the
public API was still "curve".
Then we exported a few more things in
https://boringssl-review.googlesource.com/c/boringssl/+/8565, but I left
it at "curve" to keep the public API self-consistent.
Then we added OpenSSL's new "group" APIs in
https://boringssl-review.googlesource.com/c/boringssl/+/54306, but
didn't go as far to deprecate the old ones yet.
Now I'd like to add new APIs to clear up the weird mix of TLS codepoints
and NIDs that appear in our APIs. But our naming is a mess, so either
choice of "group" or "curve" for the new API looks weird.
In hindsight, we probably should have left it at "curve". Both terms are
equally useless for the future post-quantum KEMs, but at least "curve"
is more unique of a name than "group". But at this point, I think we're
too far through the "group" rename to really backtrack:
- Chromium says "group" in its internals
- QUICHE says "group" in its internals and public API
- Our internals say "group"
- OpenSSL has switched to "group" and deprecated "curve", so new APIs
will be based on "group"
So align with all this and say "group". This CL handles set1_curves and
set1_curves_list APIs, which already have "group" replacements from
OpenSSL. A follow-up CL will handle our APIs. This is a soft deprecation
because I don't think updating things is particularly worth the churn,
but get the old names out of the way, so new code can have a simpler API
to target.
Also rewrite the documentation for that section accordingly. I don't
think we need to talk about how it's always enabled now. That's a
reference to some very, very old OpenSSL behavior where ECDH negotiation
needed to be separately enabled.
Change-Id: I7a356793d36419fc668364c912ca7b4f5c6c23a2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60206
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
3DES has long been obsolete. It uses a small block size, making it
vulnerable to attacks at sufficiently high volumes (see
https://sweet32.info/, CVE-2016-6329). On top of this, it is slow even
without constant-time protections, making it a DoS risk for server
operators.
Since the alias "3DES" has existed in OpenSSL for a long time, keep that
one working, to reduce the risk of breaking someone who specifically
wanted 3DES enabled.
Update-Note: This CL disables TLS_RSA_WITH_3DES_EDE_CBC_SHA by default.
Specifically, it will not be included unless explicitly listed in the
cipher config, as "TLS_RSA_WITH_3DES_EDE_CBC_SHA", its legacy OpenSSL
name "DES-CBC3-SHA", or the alias "3DES". To restore it, add one of the
above to your cipher config.
Bug: 599
Change-Id: Ib94a2f149b3bfa240ef1008b9f3729a9c10368fb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59425
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is intended to be run under TSan. ex_data currently works by taking
a global read lock every time an RSA, SSL, SSL_CTX, etc., is freed. We
should be able to fix that but, first, make sure we have test coverage
for the threading requirements.
Bug: 570
Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan
Change-Id: I0e12907e116481d88e45191a1f15f3a51833bf6d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59865
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Envoy needs to have the possible cipher, etc., strings predeclared to
reduce synchronization needs in the steady state. It currently does this
by (1) iterating over SSL_CTX_get_ciphers at SSL_CTX creation time and
(2) hard-coding a lists of known TLS 1.3 ciphers, TLS versions,
NamedGroups, etc.
(1) would work for some applications, but it breaks any applications
that configure ciphers on the SSL on a certificate callback, etc. If the
callback configures a cipher that wasn't configured on the SSL_CTX (e.g.
if the SSL_CTX were left at defaults), Envoy's logging breaks and we hit
an ENVOY_BUG assertion.
(2) breaks whenever BoringSSL adds a new feature. In principle, we could
update Envoy when updating BoringSSL, but this is an unresasonable
development overhead for just one of many BoringSSL consumers to impose.
Such costs are particularly high when considering needing to coordinate
updates to Envoy and BoringSSL across different repositories.
Add APIs to enumerate the possible strings these functions can return.
These string lists are a superset of those that any one application may
care about (e.g. we may have a deprecated cipher that Envoy no longer
needs, or an experimental cipher that's not yet ready for Envoy's
stability goals), but this is fine provided this is just used to
initialize the table. In particular, they are *not* intended to
enumerate supported features.
Bump BORINGSSL_API_VERSION to aid in patching these into Envoy.
Bug: b:280350955
Change-Id: I4d11db980eebed5620d3657778c09dbec004653c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59667
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This TLS 1.2 algorithm is substantially inferior to AES-GCM and should
never be used. It will not be available unless configured by name.
However, in can be used to provide backwards-compatibility with devices
that cannot be updated if so needed.
Change-Id: I1fd78efeb33aceca76ec2e7cb76b70f761ed1af8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59585
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>
The initial codepoint is called X25519Kyber786Draft00 in the draft, so
align with that name for this version. Also remove the placeholder bits
for the other combinations, which haven't gotten that far yet.
Update-Note: Update references to NID_X25519Kyber768 to
NID_X25519Kyber768Draft00. For now, the old name is available as an
alias.
Change-Id: I2e531947f41e589cec61607944dca844722f0947
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59605
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
HRSS itself remains in libcrypto because there are some direct users of
it. But this will let it be dropped by the linker in many cases.
Change-Id: I870eda30c9ed1d08693c770e9e7df45a2711b7df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58645
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Also unexport PEM_proc_type and PEM_dek_info. They're never called
externally, just private functions within one file. Also, while I'm
here, fix the include guard on asn1/internal.h.
Bug: 516
Change-Id: I6961a65f638e7b464a8c349663898a954d7826b4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58548
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This was replaced with the upstream-compatible SSL_CIPHER_standard_name
in https://boringssl-review.googlesource.com/17324. It looks like we've
since migrated everything off the old name, so let's just remove it.
Update-Note: SSL_CIPHER_get_rfc_name calls can be replaced with
SSL_CIPHER_standard_name, which is also more efficient as it avoids an
allocation and copy. If anyone's using this function and can't easily
migrate, let us know and we can put this back for a little longer.
Change-Id: I6bce40a8a146671429641a5dbff6f614006a9a1c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58665
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Caught by running malloc failure tests on unit tests.
Bug: 563
Change-Id: Ic0167ef346a282dc8b5a26a1cedafced7fef9ed0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56927
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
That test cert expires in 2099, which is a ways off but if this code is
somehow still around by then, let's save the future some pain. With this
fixed, our test all pass at least through the year 3000, so we're
hopefully clear of timebombs.
Change-Id: Ie9dcbc4f4db70c6bcc1ae9717c6e1ee89eb4195c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55625
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Bug: b:261632678, chromium:1396479
Change-Id: I82f7ce05ece8b5c145d4394dc0d4173e357ce176
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55585
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
It should not be possible to make BoringSSL request unknown signature
algorithms, or the special SSL_SIGN_RSA_PKCS1_MD5_SHA1 value, in the
ClientHello or CertificateRequest.
Update-Note: This CL makes unknown values fail
SSL_set_verify_algorithm_prefs, etc. SSL_SIGN_RSA_PKCS1_MD5_SHA1 is
silently dropped from the list, rather than an error because, although
documented as incorrect, this hole in the abstraction seems to be
confusing. I think there's some code in Chromium which accidentally puts
it in the signing prefs (wrong but harmless) and I often need to explain
to folks that it doesn't belowing in verify prefs (puts it in the
ClientHello). This makes us tolerate the value by ignoring it.
This makes the previous pkey_supports_algorithm change moot because we'd
never get that far with SSL_SIGN_RSA_PKCS1_MD5_SHA1, but I think the
check, but I think the check belongs in that function too.
The test also reveals that some of our tests have been accidentally
passing zero into the preference list all this time.
Change-Id: I76d4eb98682515c3b819e0ed8d44f2d708a98975
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55446
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
SSL_export_keying_material can only be used when the exporter secret is
available, e.g. during False Start (TLS 1.2) and on the server when
processing 0-RTT (TLS 1.3). These conditions were special cased, but
there is at least one more case in TLS 1.3 where the exporter secret is
available. This change switches the logic for TLS 1.3 to check whether
the exporter secret has been derived and makes
SSL_export_keying_material functional if it has, instead of checking if
the handshake is in one of some number of specified states.
Allowing the availability of the exporter in TLS 1.3 on the server after
processing the client's handshake flight and sending the server Finished
is equivalent to the already-allowed case of exposing the exporter in
TLS 1.2 False Start.
Bug: b:255591447
Change-Id: Ib216fd4a676524a777aae17569161c02dd2e40ca
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55025
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
We never ended up using these, or making them work with TLS 1.3 (which
has KeyUpdates and NewSessionTickets). It'd still be nice to have an
in-place API, but for now unwind these ones until we have time to give
it another go. Supporting TLS 1.3's post-handshake messages will
probably require a slightly more involved design.
(I suspect some of the seal_scatter bits in tls_record.cc can also be
simplified with these removed, but I've left them alone here.)
Update-Note: Removed some unused, experimental APIs.
Change-Id: Iad1245fa467cc6e599d20561f5db44d236219e06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54527
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Among many many problems with renegotiation is it makes every API
ambiguous. Do we return the pending handshake's properties, or the most
recently completed handshake? Neither answer is unambiguously correct:
On the one hand, OpenSSL's API makes renegotiation transparent, so the
pending handshake breaks invariants. E.g., currently,
SSL_get_current_cipher and other functions can return NULL mid
renegotiation. See https://crbug.com/1010748.
On the other hand, OpenSSL's API is callback-heavy. During a handshake
callback, the application most likely wants to check the pending
parameters. Most notably, cert verify callbacks calling
SSL_get_peer_certificate.
Historically, only the pending state was available to return anyway.
We've since changed this
(https://boringssl-review.googlesource.com/8612), but we kept the public
APIs as-is. I was particularly worried about cert verify callbacks.
As of https://boringssl-review.googlesource.com/c/boringssl/+/14028/ and
https://boringssl-review.googlesource.com/c/boringssl/+/19665/, cert
verify is moot. We implement the 3-SHAKE mitigation in library, so the
peer cert cannot change, and we don't reverify the certificate at all.
With that, I think we should switch to returning the established
parameters. Chromium is the main consumer that enables renegotiation,
and it would be better off with this behavior. (Maybe we should try to
forbid other properties, like the cipher suite, from changing on
renegotiation. Unchangeable properties make this issue moot.)
This CL would break if the handshake internally used SSL_get_session,
but this is no longer true as of
https://boringssl-review.googlesource.com/c/boringssl/+/41865.
Update-Note: Some APIs will now behave differently mid-renegotation. I
think this is the safer option, but it is possible code was relying on
the other one.
Fixed: chromium:1010748
Change-Id: I42157ccd9704cde3eebf947136d47cda6754c36e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54165
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Most SSL_ERROR_* values are tracked directly with rwstate. SSL_get_error
is just reading the extra return value out from the previous call.
However, SSL_ERROR_ZERO_RETURN infers close_notify from the SSL's
shutdown state and a zero return value (EOF).
This works, but if we implement SSL_read_ex and SSL_write_ex, a zero
return value is no longer as carefully correlated with EOF. Moreover,
it's already possible to get a non-EOF zero return post-close_notify if
BIO_write returns an (arguably incorrect) return value. Instead, track
SSL_ERROR_ZERO_RETURN in rwstate explicitly.
Since rwstate is exposed as SSL_want and SSL_ERROR_ZERO_RETURN was
previously never returned there, I've made it map SSL_ERROR_ZERO_RETURN
back to SSL_ERROR_NONE. I've also added a test for BIO_write returning
zero, though the real purpose is for a subsequent SSL_write_ex
implementation to retain all the other tests we've added in here.
Update-Note: This is intended to be safe, but if anything breaks around
EOFs, this change is a likely culprit.
Bug: 507
Change-Id: Ide0807665f2e02ee695c4976dc5e99fb10502cf0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53946
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
A later CL will tighten up SSL_ERROR_ZERO_RETURN handling. In
preparation for this, test that SSL_CTX_set_quiet_shutdown can trigger
SSL_ERROR_ZERO_RETURN.
Bug: 507
Change-Id: Ib50a02c514673ad4b73540934480d54b372d9505
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53945
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/53007
inadvertently changed the semantics of SSL_load_client_CA_file slightly.
The original implementation, by delaying allocating ret, would fail
rather than return an empty list.
Fix this and add a test. We don't have much support for testing
filesystem-related things yet, so I've just used /dev/null and gated it
to Linux + macOS for now. If we need it later, we can add temporary file
support to the test_support library.
Change-Id: If77dd493a433819a65378d76cf400cce48c0abaa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53785
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This works correctly, but part of implementing SSL_write_ex will, if not
done correctly, regress this. Specifically, if the read_shutdown check
in SSL_get_error were not conditioned on ret == 0, the last
SSL_get_error in the test would mistakenly classify the write error as
SSL_ERROR_ZERO_RETURN.
Add a regression test in advance.
Bug: 507
Change-Id: I8ddb4606e291977506ee81f4ed11427e5b1636d8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53626
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Writing application data goes through three steps:
1. Encrypt the data into the write buffer.
2. Flush the write buffer to the network.
3. Report to SSL_write's caller that the write succeeded.
In principle, steps 2 and 3 are done together, but it is possible that
BoringSSL needs to write something, but we are not in the middle of
servicing an SSL_write call. Then we must perform (2) but cannot perform
(3).
TLS 1.3 0-RTT on a client introduces a case like this. Suppose we write
some 0-RTT data, but it is blocked on the network. Meanwhile, the
application tries to read from the socket (protocols like HTTP/2 read
and write concurrently). We discover ServerHello..Finished and must then
respond with EndOfEarlyData..Finished. But to write, we must flush the
current write buffer.
To fix this, https://boringssl-review.googlesource.com/14164 split (2)
and (3) more explicitly. The write buffer may be flushed to the network
at any point, but the wpend_* book-keeping is separate. It represents
whether (3) is done. As part of that, we introduced a wpend_pending
boolean to track whether there was pending data.
This introduces an interesting corner case. We now keep NewSessionTicket
messages buffered until the next SSL_write. (KeyUpdate ACKs are
implemented similarly.) Suppose the caller calls SSL_write(nullptr, 0)
to flush the NewSessionTicket and this hits EWOULDBLOCK. We'll track a
zero-length pending write in wpend_*! A future attempt to write non-zero
data would then violate the moving buffer check. This is strange because
we don't build records for zero-length application writes in the first
place.
Instead, wpend_pending should have been wpend_tot > 0. Remove that and
rearrange the code to check that properly. Also remove wpend_ret as it
has the same data as wpend_tot.
Change-Id: I58c23842cd55e8a8dfbb1854b61278b108b5c7ea
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53546
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This is split out from
https://boringssl-review.googlesource.com/c/boringssl/+/47544 just to
get the bugfixes and tests out of the way of the refactor.
If we trip the SSL_R_BAD_LENGTH check in tls_write_app_data, wnum is set
to zero. But wnum should only be cleared on a successful write. It
tracks the number of input bytes that have been written to the transport
but not yet reported to the caller. Instead, move it to the success
return in that function. All the other error paths already set it to
something else.
Change-Id: Ib22f9cf04454ecdb0062077f183be5070ab7d791
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53545
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
CPython uses this function. Our implementation is slightly weird since
it leaks the clamping behavior, but probably not a big deal.
Update-Note: When this is merged into the internal repository, we can
simplify the CPython patches.
Change-Id: I291ddf852fb463bf02998fe04d0d0e8cb358dc55
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53485
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>
Our TLS 1.3 stack predates OpenSSL's. We chose TLS1_CK_* to align with
the existing names. OpenSSL made a new convention, TLS1_3_CK_*. Match
them.
This means that, in the likely event that TLS 1.4 uses the same
constants, they'll have weird names, just as several of our constants
still say SSL3_* but it doesn't particularly matter.
Change-Id: I97f29b224d0d282e946344e4b907f2df2be39ce1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53425
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
SSL_load_client_CA_file can just call
SSL_add_file_cert_subjects_to_stack.
SSL_add_file_cert_subjects_to_stack itself is rewritten to use scopers
and also give subquadratic running time. Sorting after every insertion
does not actually help. (It would have been faster to do a linear
search.) Instead, gather the names first, then sort and deduplicate.
Finally, add a SSL_add_bio_cert_subjects_to_stack. This is both to
simplify testing and because Envoy code copied from
SSL_add_file_cert_subjects_to_stack, complete with the quadratic
behavior. It is the only external project that depends on the
STACK_OF(T) comparison function. To simplify making that const-correct,
just export the function they needed anyway.
Bug: 498
Change-Id: I00d13c949a535c0d60873fe4ba2e5604bb585cca
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53007
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CPython and wpa_supplicant are using this nowadays. To avoid needing to
tweak the ticket nonce derivation, I've just internally capped the
number of tickets at 16, which should be plenty.
Change-Id: Ie84c15b81a2abe8ec729992e515e0bd4cc351037
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52465
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The check finds implicit conversions of integer literals to bools:
bool b1 = 1;
bool b2 = static_cast<bool>(1);
and transforms them to:
bool b1 = true;
bool b2 = true;
Bug: chromium:1290142
Change-Id: I15579e28f544d07b331a230b70a8278e0651150d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51085
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
GCC has a warning that complains about even more type mismatches in
printf. Some of these are a bit messy and will be fixed in separate CLs.
This covers the easy ones.
The .*s stuff is unfortunate, but printf has no size_t-clean string
printer. ALPN protocol lengths are bound by uint8_t, so it doesn't
really matter.
The IPv6 printing one is obnoxious and arguably a false positive. It's
really a C language flaw: all types smaller than int get converted to
int when you do arithmetic. So something like this first doesn't
overflow the shift because it computes over int, but then the result
overall is stored as an int.
uint8_t a, b;
(a << 8) | b
On the one hand, this fixes a "missing" cast to uint16_t before the
shift. At the same time, the incorrect final type means passing it to
%x, which expects unsigned int. The compiler has forgotten this value
actually fits in uint16_t and flags a warning. Mitigate this by storing
in a uint16_t first.
The story doesn't quite end here. Arguments passed to variadic functions
go through integer promotion[0], so the argument is still passed to
snprintf as an int! But then va_arg allows for a signedness mismatch[1],
provided the value is representable in both types. The combination means
that %x, though actually paired with unsigned, also accept uint8_t and
uint16_t, because those are guaranteed to promote to an int that meets
[1]. GCC recognizes [1] applies here.
(There's also PRI16x, but that's a bit tedious to use and, in glibc, is
defined as plain "x" anyway.)
[0] https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions
[1] https://en.cppreference.com/w/c/variadic/va_arg
Bug: 450
Change-Id: Ic1d41356755a18ab922956dd2e07b560470341f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50765
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Always enable X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS and never enable
X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS.
Update-Note: BoringSSL will no longer accept wildcard patterns like
*www.example.com or www*.example.com. (It already did not accept
ww*w.example.com.) X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS will also be
ignored and can no longer be used to allow foo.bar.example.com to match
*.example.com.
Fixes: 462
Change-Id: I004e087bf70f4c3f249235cd864d9e19cc9a5102
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50705
Reviewed-by: Adam Langley <agl@google.com>
This was added in OpenSSL 1.1.x. It is slightly different from
SSL_pending in that it also reports buffered transport data.
Change-Id: I81e217aad1ceb6f4c31c36634a546e12b6dc8dfc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50445
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
There is a long outdated comment that TLS 1.3 is disabled by default,
which is no longer true. While I'm here, run through all TLS and DTLS
versions, now that we have that table.
Change-Id: I7b813111ad3be295cc5a7e0eb0c7088e40df2a35
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49905
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
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>
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>
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>
David Benjamin
Gabriel Redner
Victor Tan
Victor Tan
Adam Langley
Adam Langley
Bob Beck
Nick Harper
Anton Bikineev
Adam Langley