Someone requested that bssl::ScopedEVP_HPKE_KEY be movable.
Change-Id: I48058567c776b5fe9a746072ccb7ddd723ef2b68
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62265
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
When applications use Diffie-Hellman incorrectly, and use
attacker-supplied domain parameters, rather than known-valid ones (as
required by SP 800-56A, 5.5.2), algorithms that aren't designed with
attacker-supplied parameters in mind become attack surfaces.
CVE-2023-3446 and CVE-2023-3817 in OpenSSL cover problems with the
DH_check function given large p and large q. This CL adds some fast
validity checks to the DH parameters before running any operation. This
differs from upstream in a few ways:
- Upstream only addressed issues with DH_check. We also check in
DH_generate_key and DH_check_pub_key.
- For a more consistent invariant, reuse the existing DH modulus limit.
Ideally we'd enforce these invariants on DH creation, but this is not
possible due to OpenSSL's API. We additionally check some other
cheap invariants.
This does not impact TLS, or any applications that used Diffie-Hellman
correctly, with trusted, well-known domain parameters.
Ultimately, that this comes up at all is a flaw in how DH was specified.
This is analogous to the issues with ECC with arbitrary groups and DSA,
which led to https://github.com/openssl/openssl/issues/20268
CVE-2022-0778, CVE-2020-0601, and likely others. Cryptographic
primitives should be limited to a small set of named, well-known domain
parameters.
Update-Note: Egregiously large or invalid DH p, q, or g values will be
more consistently rejected in DH operations. This does not impact TLS.
Applications should switch to modern primitives such as X25519 or ECDH
with P-256.
Change-Id: I666fe0b9f8b71632f6cf8064c8ea0251e5c286bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62226
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
If the DH object already has a private key, DH_generate_key is actually
a function to compute the corresponding public key. This is very weird,
but as we don't really care about DH, just document and test it.
Change-Id: Idbddfd06839450a198fdf8a34bf2f53b0250c400
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62225
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
With b/291102972 resolved, we can try this again.
Bug: 629, b:291102972
Change-Id: Ic04d1855f185ead6ae2e151dcc56493afce40b4f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62105
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Pretty much all of std::span and base::span are constexpr. der::Input
similarly has constexpr bits. So we can use bssl::Span in der::Input,
align bssl::Span in constexpr-ness.
Also fix const-ness of first() and last().
Bug: chromium:770501
Change-Id: Ic0031cd955d8ac0af9c3cb928411f23a34820347
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61945
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>
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>
See if that helps discourage their use slightly. (Though pretty much all
the uses are problematic things like Rust and prebuilts, so they
probably won't notice.)
Change-Id: I012b68922d7b36e778488e7455908e81b3de15f7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61905
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This is a temporary workaround for b/291994116
Bug: b:291994116
Change-Id: I5b0372e07cf3bdf4b6a5b4b37dabea828ad026a8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61885
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This corresponds to the libcrypto_baremetal build target in Android,
which is an embedded-style platform that uses a subset of the bionic
libc. It will also, eventually, use getentropy for its PRNG.
As part of this, generalize the OPENSSL_TRUSTY exclusion for file BIOs
to OPENSSL_NO_FILESYSTEM. Upstream OpenSSL uses OPENSSL_NO_STDIO, but
that excludes all of FILE entirely. We already require FILE in quite a
few places (urandom.c, self_test.c) for writing to stderr, and FILE is
part of C standard library. So, let's tentatively say that we require
you have FILE and stderr.
Instead, OPENSSL_NO_FILESYSTEM is saying you don't have fopen. You're
still required to have the three std{in,out,err} FILEs, and given a
FILE, you need to allow the standard operations on it. (Possibly in
forms that always fail.)
To keep us honest, whenever a function is excluded, I've dropped it from
the header too, and followed callers up the chain. I have not attempted
to make the tests work when these are excluded. Later CLs in this series
will do the same for NO_SOCK and NO_POSIX_IO. This was a little tedious,
but not too bad.
(I assume we'll end up changing our minds on this a lot. For now, let's
try this.)
I haven't yet restored OPENSSL_RAND_TRUSTY or removed the OPENSSL_TRUSTY
ifdef on file.c. Having a separate CL makes it a bit easier to revert if
something goes wrong.
This depends on
https://android-review.googlesource.com/c/platform/bionic/+/2659335,
which fixes the header bionic uses for getentropy.
Bug: 629, b:291102972
Change-Id: Idd839cd3fa4253128de54bd1be7da261dbcdeb7c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61726
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
BoringSSL cannot support any build configurations where a platform
claims to be something but is not. Different embedded targets need their
own define.
Bug: b:291102972
Change-Id: I91db6ddd823b0d8b7b52580f7a6c498e55da6b79
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61605
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
GCC does not have __has_feature, so writing #if __has_feature(foo),
without a guard, will cause GCC to error. This is tripping the gRPC
update.
Prior to https://boringssl-review.googlesource.com/c/boringssl/+/60765,
this worked because the preamble to every assembly file would define the
missing __has_feature macro as part of detecting MSan. Now we pick up
the logic in <openssl/base.h>, which tries not to stomp over symbols we
don't own. This had the side effect of removing the __has_feature
polyfill.
Though "public", <openssl/asm_base.h> is not really a public header, so
we could put the __has_feature polyfill in there. But we already have a
pattern for detecting sanitizers in <openssl/target.h>, so just switch
to that one.
Change-Id: I747b4513f1b2f189d2df629149f22fd0fa490257
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61565
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Due to b/290792019 and b/290785937, we need them to actually exist at
the original symbols. For all of Rust's language-level safety benefits,
the ecosystem seems determined to undo it with patterns that are even
less safe than C.
This is not great and the bugs need to be fixed, but do this for now to
unblock the Android update.
Change-Id: Ia883336879779f652e7320cecdd5ca843996f6a3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61525
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Also remove unnecessary EC_GROUP_free calls. EC_GROUP_free is only
necessary in codepaths where arbitrary groups are possible.
Bug: 20
Change-Id: I3dfb7f07b890ab002ba8a302724d8bc671590cfe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60932
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Add a rand_extra file for trusty, bump the BORINGSSL_API_VERION
and mark both trusty and windows as non-forking so we do not
require fork detection support.
Update-Note:
Prior to API version 24, Trusty maintained their own CRYPTO_sysrand
implementations outside of the BoringSSL tree. With this change
they are not expected to provide CRYPTO_sysrand, it is maintained
inside the BoringSSL tree.
Change-Id: Iabcef024ff85bd767e2869a6ff27a64236322325
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61465
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This replaces our dynamically creating singleton EC_GROUPs from curve
data with static EC_GROUP instances.
They're just shy of being fully static because delocate still forces us
go to through CRYPTO_once to initialize structures with pointers.
(Though, without delocate, the loader would need similar initialization
via a runtime relocation.)
This means we can now have functions like EC_group_p256(), analogous to
EVP_sha256(). These are infallible functions that return const EC_GROUP
pointers. Although there is an initial 2KiB hit to binary size (now we
precompute a few more Montgomery values), I'm hoping it'll eventually
help binaries that only use a few of the curves to drop the others. Also
it removes some locks used to initialize the static curve objects, as
well as removing an annoying error condition.
Bug: 20
Change-Id: Id051c5439f2b2fe2b09bf10964d656503ee27d9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60931
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This function hasn't done anything since 2015, when
https://boringssl-review.googlesource.com/5792 made it a no-op.
Also move RAND_get_system_entropy_for_custom_prng under obscure
functions. It's usually not what you want.
Change-Id: Ica57552792acff3c5991e6db09c15112db09d133
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61505
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
They end up in uint64_t frequently right now just because the CBS APIs
use uint64_t, but we don't actually accept that large of an iteration
count.
Also use uint32_t instead of unsigned. This type should be sized based
on how large of an iteration count we think is reasonable, not something
platform-dependent.
Bug: 516
Change-Id: Ie5ff379af6bc65c5e4d25f4d10774bd819f08a50
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61426
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: Ie64ed383a1f6b63a0624c8a6f64d92a33dabf56e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61485
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This allows operating systems to insist on IBT
enforcement as an exploit mitigation mechanism without
needing to make an exception for anything using a
bundled boringssl, such as chrome, mono, and qtwebengine.
Change-Id: Iac28dd3d2af177b89ffde10ae97bce23739feb94
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Trying to migrate Chromium to the "link all the asm files together"
strategy broke the aarch64 Android build because some of the ifdef'd out
assembly files were missing the .note.gnu.property section for BTI. If
we add support for IBT, that'll be another one.
To fix this, introduce <openssl/asm_base.h>, which must be included at
the start of every assembly file (before the target ifdefs). This does a
couple things:
- It emits BTI and noexecstack markers into every assembly file, even
those that ifdef themselves out.
- It resolves the MSan -> OPENSSL_NO_ASM logic, so we only need to do it
once.
- It defines the same OPENSSL_X86_64, etc., defines we set elsewhere, so
we can ensure they're consistent.
This required carving files up a bit. <openssl/base.h> has a lot of
things, such that trying to guard everything in it on __ASSEMBLER__
would be tedious. Instead, I moved the target defines to a new
<openssl/target.h>. Then <openssl/asm_base.h> is the new header that
pulls in all those things.
Bug: 542
Change-Id: I1682b4d929adea72908655fa1bb15765a6b3473b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60765
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
nanolibc is an embedded platform with no threads. To start unforking
that build, generalize some of the OPENSSL_TRUSTY defines. OpenSSL has
OPENSSL_NO_SOCK if you don't have sockets and OPENSSL_NO_POSIX_IO if you
don't have file descriptors. Those names are fine enough, so I've
borrowed them here too.
There's more to be done here, but this will clear out some of it.
Change-Id: Iaba1fafdebb46ebb8f68b7956535dd0ccaaa832f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60890
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
While EVP_PKEY_RSA, EVP_PKEY_DSA, and EVP_PKEY_EC have publicly-exposed
internaly representations, other EVP_PKEY types to not. EVP_PKEY_assign
should not allow callers to manipulate those representations.
As part of this, teach EVP_PKEY_assign_RSA, etc. to find their method
tables directly, rather than indirecting through an integer. This makes
those EVP APIs static-linker-friendly.
Bug: 618, 497
Change-Id: Ic45a7514e9a3adc505759f2327129f13faf03a65
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60645
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We no longer need to define CRYPTO_MUTEX in public headers. This
simplifies a pile of things. First, we can now use pthread_rwlock_t
without any fuss, rather than trying to guess the size on glibc.
As a result, CRYPTO_MUTEX and CRYPTO_STATIC_MUTEX can be merged into one
type. We can almost do this to CRYPTO_refcount_t too. BIO is the one
straggler remaining.
Fixed: 325
Change-Id: Ie93c9f553c0f02ce594b959c041b00fc15ba51d2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60611
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Three years of updating calling code are finally complete!
Update-Note: Accessing the RSA struct directly is no longer supported.
Use accessors instead.
Bug: 316, 325
Change-Id: I27b4c9899cb96f5807075b8fe351eaf72a9a9d44
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60610
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
rust-openssl uses this function when targetting OpenSSL 1.1.x.
Change-Id: Ifeb1b65be9976358f9ee636ed23c1a931e03b275
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60609
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@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>
These bits need more work (and possibly some removal) as they're very,
very far from thread-safe, but rust-openssl relies on them being
const-correct when targetting OpenSSL 1.1.x.
Change-Id: I60531c7e90dbdbcb79c09fc440bd7c6b474172df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60607
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
At the public API, the comparison functions are a double pointer to
match qsort. qsort comparators are passed pointers to the list elements,
and our list elements are T*. qsort needs this indirection because it
can sort values of arbitrary size. However, our type-erased versions
have no such constraints. Since we know our all elements are pointers,
we can skip the indirection.
Change-Id: Ibb102b51a5aaf0a68a7318bf14ec8f4f9c7a3daf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60506
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(https://github.com/sfackler/rust-openssl/issues/1944), but to fix that,
we'll need to align with the OpenSSL names.
To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.
Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
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>
Update-Note: Accessing the DSA struct directly is no longer supported.
Use accessors instead.
Bug: 325
Change-Id: I73dc20f2e275d48648ca84d2db7806fe155c567d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60425
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The assembly functions from OpenSSL vary in how the counter overflow
works. The aarch64 implementation uses a mix of 32-bit and 64-bit
counters. This is because, when packing a block into 64-bit
general-purpose registers, it is easier to implement a 64-bit counter
than a 32-bit one. Whereas, on 32-bit general-purpose registers, or when
using vector registers with 32-bit lanes, it is easier to implement a
32-bit counter.
Counters will never overflow with the AEAD, which sets the length limit
so it never happens. (Failing to do so will reuse a key/nonce/counter
triple.) RFC 8439 is silent on what happens on overflow, so at best one
can say it is implicitly undefined behavior.
This came about because pyca/cryptography reportedly exposed a ChaCha20
API which encouraged callers to randomize the starting counter. Wrapping
with a randomized starting counter isn't inherently wrong, though it is
pointless and goes against how the spec recommends using the initial
counter value.
Nonetheless, we would prefer our functions behave consistently across
platforms, rather than silently give ill-defined output given some
inputs. So, normalize the behavior to the wrapping version in
CRYPTO_chacha_20 by dividing up into multiple ChaCha20_ctr32 calls as
needed.
Fixed: 614
Change-Id: I191461f25753b9f6b59064c6c08cd4299085e172
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60387
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Andres Erbsen noticed we didn't actually have tests to catch when the
format macros were wrong.
In doing so, remove BN_DEC_FMT2. It was unused and only makes sense in
the context of the bignum-to-decimal conversion, where we no longer use
it anyway. None of these macros are exported in OpenSSL at all, so it
should be safe to remove it. (We possibly can remove the others too. I
see one use outside the library, and that use would probably be better
written differently anyway.)
Change-Id: I4ffc7f9f7dfb399ac060af3caff0778000010303
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60325
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
NGINX requires this constant if SSL_get_negotiated_group is defined.
OpenSSL returns this to indicate a named group constant it doesn't
understand, which doesn't make sense out of SSL_get_negotiated_group
because the library wouldn't negotiate a group it doesn't know about.
Nonetheless, define it for compatibility.
Fixed: 615
Change-Id: I05a6d607912bb8507be9ac6ff5fe1699b4715f6b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60326
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@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>
Update-Note: SSL_CIPHER_get_value was our original name for the
function. OpenSSL later called it SSL_CIPHER_get_protocol_id. I believe
all external callers have since been updated to use the new function.
(If I missed a few stragglers, replace with SSL_CIPHER_get_protocol_id
to fix.)
Change-Id: I956fb49bf2d13a898eed73177493d2c8d50778ad
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60205
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
CONF supports a variable expansion feature, which allows a config file
to easily grow exponentially.
2d05568a7b7bc62affbd13ea97a81b5829b99794 (upstream's
6a6213556a80ab0a9eb926a1d6023b8bf44f2afd) capped the expansion to 65536
bytes, but this still allows a small input to produce a fairly large
output. (Once we have one large value, it only takes five characters
make a new key that references it, which is an expansion factor of
around 13,000.) This, combined with the string-based extensions
machinery's many quadratic behaviors (see
b92fcfdc17f3ad794c220a86f4ae6695d0a0fb61), means fuzzers hit timeouts.
We have very few uses of these APIs left, and none of them use this
feature. Just remove it. While none of these super-linear behaviors are
DoS bugs per se (they should never be passed untrusted input), there's
no sense in carrying an unused feature that only frustrates the fuzzers.
Update-Note: NCONF_load and NCONF_load_bio no longer support the $foo
variable expansion syntax. If you are using these functions in your
project, remove them.
Bug: oss-fuzz:59049
Change-Id: I85832ae1d05373ee420f4fb68197786736ca8272
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60105
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I8792f118d281bc7a407dfbabe1c8b8e63f9eed9f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60085
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Although we've switched STACK_OF(T) to use size_t, OpenSSL used int
pervasively. Much of crypto/x509 and third-party callers use int
indices. As much of that is in the public API now, ensure that
STACK_OF(T) can never exceed INT_MAX elements.
Bug: 516
Change-Id: I26b8fe590655f8c3e449b749b5d0222e28c413f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60065
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
refcount.c is now a single, generic file that calls into C11-atomic-like
APIs. Behind the scenes, this selects one of C11 atomics, Windows
interlocked APIs, or unsynchronized reads/writes (in the no-threads
build).
This frees us up to use atomics elsewhere in the library. For now, this
only binds sequentially consistent atomics, but we can add other memory
orders if needed. In particular, I believe up_ref only needs relaxed
atomics. Some of the later change I think only need acquire and release,
but I'm not positive.
Bug: 570
Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan
Change-Id: Ifcd7357611bb7a8cd14b82c23ad080d1a2df1386
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59848
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Saves some duplicated logic.
Change-Id: I202fa92a88101f9ad735648bc414ab05752641da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59685
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Trying to fix all the places where these formats go quadratic isn't a
good use of time. We've already documented that they're not safe for use
with untrusted inputs. Even without such DoS issues, they cannot be
safely used anyway. (E.g. RUSTSEC-2023-0023.)
Just cap the fuzzer input. It'd be nice if we could avoid this more
systematically in the function, but they're not structured to make this
easy to do, and anyone concerned about DoS in this function has worse
problems.
Bug: chromium:1444420, oss-fuzz:56048, 611
Change-Id: I53eeb346f59278ec2db3aac4a92573b927ed8003
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59785
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
