| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Change-Id: I459e71c4ff12cdcd2783704409b64bbc0fe9b23d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66808
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>
|
|
Although we usually prefer not to use special -1 returns for errors, the
public API does this across the board. Making the internal function
different doesn't do much good.
Change-Id: I6bfe8c9d989da81affeb5cb652de8d3edcbf5efa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66649
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>
|
|
The CERT structure is never null.
Change-Id: I92436e1ad6156a9f79d30f4f5e989022e8fd0e9d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66371
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
|
|
If we move SSL_certs_clear to ssl_cert.cc, ssl_cert_clear_certs does not
need to be in the header. Moreover, its only other caller, ~CERT(), does
not need to call it. Now that everything outside of SSL_X509_METHOD is
managed with scopers, the destructor does it automatically. And
cert_free on SSL_X509_METHOD already automatically calls cert_clear, so
it's a no-op to do it again.
Change-Id: Ief9c704cc45440288783564ac4db4a27fbec1bfc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66370
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@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>
|
|
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>
|
|
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>
|
|
The standard macro-based pattern does not work in bindgen because of
https://github.com/rust-lang/rust-bindgen/issues/2544
Change-Id: Ic2b92e779ade2ed55a627bba9c76f7df5c0f6136
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61185
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@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>
|
|
It's unwise for organisations to try and define TLS profiles. As in this
case, they usually make security worse. However, since this is already
established and supported by Android, this change raises it to the level
of a supported policy.
Change-Id: Ic66d5eaa33d884e57fc6d8eb922d86882b621e9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58626
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
|
|
This relands
https://boringssl-review.googlesource.com/c/boringssl/+/54606, which was
temporarily reverted.
Update-Note: By default, clients will now require RSA server
certificates used in TLS 1.2 and earlier to include the keyEncipherment
or digitalSignature bit. keyEncipherment is required if using RSA key
exchange. digitalSignature is required if using ECDHE_RSA key exchange.
If unsure, TLS RSA server signatures should include both, but some
deployments may wish to include only one if separating keys, or simply
disabling RSA key exchange. The latter is useful to mitigate either the
Bleichenbacher attack (from 1998, most recently resurfaced in 2017 as
ROBOT), or to strengthen TLS 1.3 downgrade protections, which is
particularly important for enterprise environments using client
certificates (aka "mTLS") because, prior to TLS 1.3, the TLS client
certificate flow was insufficiently encrypted or authenticated. Without
reflecting an RSA key exchange disable into key usage, and then the
client checking it, an attacker can spoof a CertificateRequest as coming
from some server.
This aligns with standard security requirements for using X.509
certificates, specified in RFC 5280, section 4.2.1.3, and reiterated in
TLS as early as TLS 1.0, RFC 2246, section 7.4.2, published 24 years ago
on January 1999. Constraints on usage of keys are important to mitigate
cross-protocol attacks, a class of cryptographic attacks that is
well-studied in the literature.
We already checked this for each of ECDSA, TLS 1.3, and servers
verifying client certificates, so this just fills in the remaining hole.
As a result, this change is also important for avoiding some weird
behaviors when configuration changes transition a server in or out of
this hole. (We've seen connection failures get misattributed to TLS 1.3
when it was really a certificate misconfiguration.)
Chrome has also enforced this for some time with publicly-trusted
certificates. As a temporary measure for callers that need more time,
the SSL_set_enforce_rsa_key_usage API, added to BoringSSL in January
2019, still exists where we need to turn this off.
Fixed: 519
Change-Id: I91bf2cfb04c92aec7875e640f90ba6f837146dc1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58805
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
|
|
Bug: 586
Change-Id: I5bc8e6df3a5a14e6b218f41181d06406e835f9c1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58605
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@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>
|
|
This function reports when security-critical checks on the X.509 key
usage extension would have failed, but were skipped due to the temporary
exception in SSL_set_enforce_rsa_key_usage. This function is meant to
aid deployments as they work through enabling this.
Change-Id: Ice0359879c0a6cbe55bf0cb81a63685506883123
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55465
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
|
|
I had a rewrite of the decrepit ciphers (CAST and Blowfish) to use
CRYPTO_{load,store}_u32_be and drop the old macros, but this is probably
not worth the effort to review. Instead, just fix the type in the macro.
Bug: 516
Change-Id: I1cdecc16f6108a6235f90cf9c2198bc797c6716e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54985
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
|
|
This reverts commit 64393b57e8734b92a6ba784bcfc02b1aa01e5ff2. We'll
reland this change in January. Projects that rely on this revert should
use SSL_set_enforce_rsa_key_usage, available since 2019, to control the
security check without being reliant on the defaults.
Bug: 519
Change-Id: Icf53eae8c29f316c7df4ec1a7c16626ac3af8560
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55005
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
|
|
Update-Note: Clients will now require RSA server certificates used in
TLS 1.2 and earlier to include the keyEncipherment or digitalSignature
bit. keyEncipherment is required if using RSA key exchange.
digitalSignature is required if using ECDHE_RSA key exchange.
We already required this for each of ECDSA, TLS 1.3, and servers
verifying client certificates, so this just fills in the remaining hole.
Chrome has also enforced this for some time with publicly-trusted
certificates. For now, the SSL_set_enforce_rsa_key_usage API still
exists where we need to turn this off.
Fixed: 519
Change-Id: Ia440b00b60a224fa608702439aa120d633d81ddc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54606
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|
|
We spend a lot of effort implementing a big-endian sequence number
update, etc., when the sequence number is just a 64-bit counter. (Or
48-bit counter in DTLS because we currently retain the epoch
separately. We can probably tidy that a bit too, but I'll leave that
for later. Right now the DTLS record layer state is a bit entwined
with the TLS one.)
Just store it as uint64_t. This should also simplify
https://boringssl-review.googlesource.com/c/boringssl/+/54325 a little.
Change-Id: I95233f924a660bc523b21496fdc9211055b75073
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54505
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
|
|
Change-Id: I5e1d37106d7df8e8aaede295e8eb74c971553fd5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54365
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
|
|
Node calls these. OpenSSL renamed their APIs to align with the IETF
renaming NamedCurve to NamedGroup. (Ironically, with post-quantum
ciphers, that name turns out also to be wrong and it probably should
have been a reference to KEMs.)
To avoid churn for now, I haven't marked the old ones as deprecated, or
renamed any of the internal types yet. We can see about doing that
later.
Change-Id: I5765cea8398f3836611977805bf8ae7d6efc0a70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54306
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@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>
|
|
The C11 change has survived for three months now. Let's start freely
using static_assert. In C files, we need to include <assert.h> because
it is a macro. In C++ files, it is a keyword and we can just use it. (In
MSVC C, it is actually also a keyword as in C++, but close enough.)
I moved one assert from ssl3.h to ssl_lib.cc. We haven't yet required
C11 in our public headers, just our internal files.
Change-Id: Ic59978be43b699f2c997858179a9691606784ea5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53665
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
|
|
We still have our <= 0 return values because anything with BIOs tries to
preserve BIO_write's error returns. (Maybe we can stop doing this?
BIO_read's error return is a little subtle with EOF vs error, but
BIO_write's is uninteresting.) But the rest of the logic is size_t-clean
and hopefully a little clearer. We still have to support SSL_write's
rather goofy calling convention, however.
I haven't pushed Spans down into the low-level record construction logic
yet. We should probably do that, but there are enough offsets tossed
around there that they warrant their own CL.
Bug: 507
Change-Id: Ia0c702d1a2d3713e71b0bbfa8d65649d3b20da9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47544
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@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>
|
|
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>
|
|
These functions aid in meeting specific compliance goals and allows
configuration of things like TLS 1.3 cipher suites, which are otherwise
not configurable.
Change-Id: I668afc734a19ecd4b996eaa23be73ce259b13fa2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52625
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: 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>
|
|
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>
|
|
It's a bit of a mess, but BIO-like APIs typically return -1 on error and
0 for EOF.
Change-Id: Ibdcb70e1009ffebf6cc6df40804dc4a178c7199e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48845
Reviewed-by: Adam Langley <agl@google.com>
|
|
ssl_update_cache takes the cache lock to add to the session cache,
releases it, and then immediately takes and releases the lock to
increment handshakes_since_cache_flush. Then, in 1/255 connections, does
the same thing again to flush stale sessions.
Merge the first two into one lock. In doing so, move ssl_update_cache to
ssl_session.cc, so it can access a newly-extracted add_session_lock.
Also remove the mode parameter (the SSL knows if it's a client or
server), and move the established_session != session check to the
caller, which more directly knows whether there was a new session.
Also add some TSan coverage for this path in the tests. In an earlier
iteration of this patch, I managed to introduce a double-locking bug
because we weren't testing it at all. Confirmed this test catches both
double-locking and insufficient locking. (It doesn't seem able to catch
using a read lock instead of a write lock in SSL_CTX_flush_sessions,
however. I suspect the hash table is distributing the cells each thread
touches.)
Update-Note: This reshuffles some locks around the session cache.
(Hopefully for the better.)
Change-Id: I78dca53fda74e036b90110cca7fbcc306a5c8ebe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48133
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|
|
In renegotiation handshakes and, later, ECH ClientHelloOuter handshakes,
we don't want to add sessions to the session cache. We also don't want
to release a session as resumable until the handshake completes.
Ideally we'd only construct SSL_SESSION at the end of the handshake, but
existing APIs like SSL_get_session must work mid-handshake, so
SSL_SESSION is both a handle to immutable resumption state, and a
container for in-progress connection properties. We manage this with a
not_resumable flag that's only cleared after the handshake is done and
the SSL_SESSION finalized.
However, TLS 1.2 ticket renewal currently clears the flag too early and
breaks the invariant. This won't actually affect renegotiation or
ClientHelloOuter because those handshakes never resume. Still, we can
maintain the invariant storing the copy in hs->new_session. Note this
does sacrifice a different invariant: previously, ssl->session and
hs->new_session were never set at the same time.
This change also means ssl_update_cache does not need to special-case
ticket renewal.
Change-Id: I03230cd9c63e5bee6bd60cd05c0439e16533c6d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48132
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|
|
This CL fixes a couple of things. First, we never tested that SSL_write
refuses to write application data after a fatal alert, so add some tests
here. With those tests, we can revise some of this logic:
Next, this removes the write_shutdown check in SSL_write and instead
relies on the lower-level versions of the check in the write_app_data,
etc., hooks. This improves error-reporting on handshake errors:
We generally try to make SSL_do_handshake errors sticky, analogous to
handshakeErr in the Go implementation. SSL_write and SSL_read both
implicitly call SSL_do_handshake. Callers driving the two in parallel
will naturally call SSL_do_handshake twice. Since the error effectively
applies to both operations, we save and replay handshake errors
(hs->error).
Handshake errors typically come with sending alerts, which also sets
write_shutdown so we don't try to send more data over the channel.
Checking this early in SSL_write means we don't get a chance to replay
the handshake error. So this CL defers it, and the test ensures we still
ultimately get it right.
Finally, https://crbug.com/1078515 is a particular incarnation of this.
If the server enables 0-RTT and then reverts to TLS 1.2, clients need
to catch the error and retry. There, deferring the SSL_write check
isn't sufficient, because the can_early_write bit removes the write
path's dependency on the handshake, so we don't call into
SSL_do_handshake at all.
For now, I've made this error path clear can_early_write. I suspect
we want it to apply to all handshake errors, though it's weird because
the handshake error is effectively a read error in 0-RTT. We don't
currently replay record decryption failures at SSL_write, even though
those also send a fatal alert and thus break all subsequent writes.
Bug: chromium:1078515
Change-Id: Icdfae6a8f2e7c1b1c921068dca244795a670807f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48065
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|
|
Although not permitted by the TLS specification, systems sometimes
ossify TLS extension order, or byte offsets of various fields. To
keep the ecosystem healthy, add an API to reorder ClientHello
extensions.
Since ECH, HelloRetryRequest, and HelloVerifyRequest are sensitive to
extension order, I've implemented this by per-connection permutation of
the indices in the kExtensions structure. This ensures that all
ClientHellos within a connection are consistently ordered. As follow-up
work, permuting the other messages would also be nice, though any server
messages would need to be incorporated in handshake hints.
Change-Id: I18ce39b4df5ee376c654943f07ec26a50e0923a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|
|
Bug: 275
Change-Id: Ib5804ce3d0a5faff5cf26af544a4afaaf0ad2cc8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47909
Reviewed-by: Adam Langley <agl@google.com>
|
|
The remaining remnants of Channel ID all configure the private key ahead
of time. Unwind the callback machinery, which cuts down on async points
and the cases we need to test.
This also unwinds some odd interaction between the callback and
SSL_set_tls_channel_id_enabled: If a client uses
SSL_set_tls_channel_id_enabled but doesn't set a callback, the handshake
would still pause at SSL_ERROR_WANT_CHANNEL_ID_LOOKUP. This is now
removed, so SSL_set_tls_channel_id_enabled only affects the server and
SSL_CTX_set1_tls_channel_id only affects the client.
Update-Note: SSL_CTX_set_channel_id_cb is removed.
SSL_set_tls_channel_id_enabled no longer enables Channel ID as a client,
only as a server.
Change-Id: I89ded99ca65e1c61b1bc4e009ca0bdca0b807359
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47907
Reviewed-by: Adam Langley <agl@google.com>
|
|
Also now that it's finalized, flip the default for
SSL_set_quic_use_legacy_codepoint.
Update-Note: QUIC APIs now default to the standard code point rather
than the draft one. QUICHE has already been calling
SSL_set_quic_use_legacy_codepoint, so this should not affect them. Once
callers implementing the draft versions cycle out, we can then drop
SSL_set_quic_use_legacy_codepoint altogether. I've also bumped
BORINGSSL_API_VERSION in case we end up needing an ifdef.
Change-Id: Id2cab66215f4ad4c1e31503d329c0febfdb4603e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47864
Reviewed-by: David Schinazi <dschinazi@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|
|
Change-Id: Ib8aaa2b6bfafc88cf51d2ae0f085bb87275a4306
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47585
Reviewed-by: Adam Langley <agl@google.com>
|
|
We didn't end up deploying this. We also never implemented the final
RFC, so what we do have isn't useful for someone who wishes to deploy
it anyway.
Update-Note: Token binding APIs are removed.
Change-Id: Iecea7c3dcf9d3e2644a3b7afaf61511310b45d5f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47584
Reviewed-by: Adam Langley <agl@google.com>
|
|
This aligns with OpenSSL. In particular, we clear not_resumable as soon
as the SSL_SESSION is complete, but it may not have an ID or ticket.
(Due to APIs like SSL_get_session, SSL_SESSION needs to act both as a
resumption handle and a bundle of connection properties.)
Along the way, use the modified function in a few internal checks which,
with the ssl_update_cache change, removes the last dependency within the
library on the placeholder SHA256 IDs.
Change-Id: Ic225109ff31ec63ec08625e9f61a20cf0d9dd648
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47447
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@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>
|
