BoringSSL can currently be built in C11 or pre-C11 mode in MSVC. They're
broadly the same, but do use completely different implementations of
alignas and alignof. Now that every build configuration I'm aware of has
been moved to the C11 mode, we don't even test the pre-C11 mode anymore.
Start requiring it.
Update-Note: If building with MSVC, BoringSSL now requires building with
/std:c11 or later. (On non-MSVC compilers, we have required C11 for a
while now.)
Fixed: 624
Change-Id: Ie9f66eee0bebac8143c23a7229c6854afaefea6e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63065
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/62585 made the
compiler emit multiple CRYPTO_library_init calls in functions which
dispatch between a tower of alternatives. Ideally, the compiler would
know that at most one call suffices.
There doesn't seem to be such an attribute, but we can get the same
effect with pure or const attributes. We tie init with returning the
capability vector. On Intel, because the vector is so large, we have to
go with a weaker version. Somewhat annoyingly, the getter must be
out-of-line, because otherwise the compiler inlines first and loses the
attribute.
I went with pure because we allow our unit tests to mutate
OPENSSL_armcap_P, which means the Arm one is, strictly speaking, pure,
not const. This slightly reduces optimization potential, but should
still allow deduping in most places. Confirmed that aes_init_key
now only calls a helper function once.
See discussion in
https://boringssl-review.googlesource.com/c/boringssl/+/62585/comment/26083b88_b3db2b75/
Bug: 35
Change-Id: I9bc464f0e5a0ed9601017a5037028f906693a137
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62985
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
All the C accesses have been sufficiently abstracted that this is pretty
easy to handle automatically.
We still have accesses from assembly, so we're not quite
initializationless yet. But this does get us most of the way there. I'm
thinking what's next is:
- Make a list of asm symbols that touch armcap or ia32cap
- For each, figure out the place(s) in the calling code where we need to
init manually and/or pull the dispatch up into C
One interesting subtlety with how this CL does it: although this CL
means you can freely call, say, CRYPTO_is_SSSE3_capable without
CRYPTO_library_init, you cannot *quite* assume that CRYPTO_library_init
has been called after you call CRYPTO_is_SSSE3_capable. It is possible
that the build defined __SSSE3__, in which case CRYPTO_is_SSSE3_capable
does nothing. This does complicate resolving the asm cases above.
Bug: 35
Change-Id: Ie52c74e4a59a7019c3af0526dbb35950604ada66
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62585
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Some tests in the Chromium verifier use gmock. Since upstream googletest
considers them a single project now, just import both of them.
Update-Note: As part of this, I've made gtest_main.cc now initializes
gmock alongside gtest. This means BoringSSL's test targets now depend on
both. Downstream builds where googletest and googlemock are separate
build targets may need to add a dependency. (Though every downstream
test I looked at treats them as one target, so this is most likely a
no-op.)
Bug: chromium:1322914
Change-Id: Ief38c675bc13a4639dee061d058580967ab99d41
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62945
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Previously, EVP_CIPHER_CTX_iv_length always returned the cipher's fixed IV length. Now, after modification with EVP_CTRL_AEAD_SET_IVLEN, it returns the correct value.
Fixed: 626
Change-Id: Id98c929439850b3e83a80111f35aabebc6e5d47a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62907
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
unw_context_t is, at least on x86_64, the same type as ucontext_t.
Passing that into unw_init_local doesn't work, but there's a
unw_init_local2 in libunwind 1.3.0 or later, which has a flag for this
case.
This avoids needing to unwind past the signal handler stack frames,
which is both simpler and faster. (Shaved around 10 seconds off running
all the unwind tests on my machine.)
Change-Id: I09c130e76682d63e51b7b9de9ff5b91415e26f32
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62867
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Frustratingly, simply writing the standard (void)write(...) does not
work because GCC is broken and intentionally leaves the warning enabled
there. This does not comply with the now standard semantics for
nodiscard.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
Instead, what seems to work is to assign it to a variable and then
(void) the variable.
Fixed: 644
Change-Id: Ic418b4185aeae1a9ca424c45a05af063e8d50255
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62666
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These are OpenSSL names for BN_bn2le_padded and BN_le2bn. We can just
replace BN_le2bn with BN_lebin2bn. BN_bn2lebinpad is not size_t-clean,
so handle it as a separate function like we did BN_bn2binpad.
Change-Id: I6999ca06140a0c8c25942362dc79d1821971d679
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62665
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
While I'm here, align on the version that compares the lengths
explicitly, rather than subtract. The subtraction trick does actually
work, because the lengths can't be negative and we're two's complement
(so 0 - INT_MAX fits in int). But just comparing avoids needing to think
about it.
Change-Id: Ide6e3539a27e187bb1a405600c367bb8dd82197e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62545
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
As with large p, q, or g in the preceding CL, an application that uses
Diffie-Hellman incorrectly could be given a large private key length.
Computing the public key and shared secret will then be very slow.
This matters for a (p, g)-only group, where q is unknown. One way or
another, we should bound or clamp dh->priv_length. The two relevant
specifications I could find are SP 800-56A Rev3 and PKCS#3. SP 800-56A
wants a value for q, so we'd need to fabricate one. I believe X9.42's
Diffie-Hellman formulation similarly expects an explicit q. PKCS#3 is
(p, g)-only and seems to match what OpenSSL does. In PKCS#3:
- DH groups have an optional l, such that 2^(l-1) <= p
- For keygen, if l was provided, pick 2^(l-1) <= x < 2^l. Otherwise,
pick 0 < x < p-1
Our current q-less keygen behavior matches this, with
l = num_bits(p) - 1. Interestingly, the first constraint allows
l = num_bits(p), but doing so allows x >= p - 1! This is a bit odd and
will wrap around the multiplicative group.
OpenSSL 3.0 (but not 1.1.1) bounds l in their q-less path, and cites
PKCS#3's 2^(l-1) <= p in a comment. But their actual check doesn't match
and excludes num_bits(p). The two problems cancel each other out.
PKCS#3 is quite old and does not even discuss subgroups or safe primes,
only this uninspiring text:
> Some additional conditions on the choice of prime, base, and
> private-value length may well be taken into account in order to deter
> discrete logarithm computation. These security conditions fall outside
> the scope of this standard.
I'm thus not inclined to give the document much weight in 2023. SP
800-56A Rev3 is not ideal either. First, we must fabricate a value for
q. (p-1)/2 is most natural, though it assumes g indeed generates a
prime-order subgroup and not the whole multiplicative group.
The second annoyance with SP 800-56A Rev3 is its insistance on uniformly
selecting between [1, 2^N-1] instead of [0, 2^N-1] or [1, 2^N]. For all
plausible values of N, this difference will not matter, yet NIST
specifies rejection sampling, defeating the point of a power-of-two
bound.
None of these really matters. p should be large enough to make the
differences between all these schemes negligible. Since we want to
follow NIST for the (p, q, g) path, using the same scheme for the (p, g)
path seems the most reasonable. Thus this CL recasts that path from
PKCS#3 to SP 800-56A, except dh->priv_length is clamped before invoking
the algorithm, to avoid worrying about whether someone expects
DH_set_length(BN_num_bits(p)) to work.
Change-Id: I270f235a6f04c69f8abf59edeaf39d837e2c79f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62228
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
sha512-armv8.pl and sha512-x86_64.pl implement both SHA-256 and SHA-512
and select which to emit by looking for "512" in the output path.
This can result in a false positive if the output path happens to
contain "512" in it. When the build uses relative paths, it's fine, but
this seems needlessly fragile. If we're generate into a temporary file,
there's a small but non-negligible probability that the path has a
"512" in it.
Instead, give those scripts three arguments: flavor hash output, so the
selection is independent of the output file name.
Bug: 542
Change-Id: Idf256abed1c07003034d3eb4544552125e3289e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62325
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Section 5.6.1.1.4 of SP 800-56A Rev 3 and Appendix B.1.2 of FIPS 186-4
select the private key out of the range [1, q-1]. We used [2, q-1]. This
distinction is unimportant. 0, 1, 2, 3, 4, etc. all make equally bad
private keys. The defense against each of these is their negligible
probability, not rejection sampling.
Nonetheless, we may as well align with *some* specification, and NIST's
formulation works fine.
Change-Id: I33352061f3fbdbec5b14b576d15be98464a57536
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: 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>
cpu_arm_openbsd.c is the same as OPENSSL_STATIC_ARMCAP.
cpu_aarch64_freebsd.c is the same as cpu_aarch64_sysreg.c. (The FreeBSD
one was using the macros in their headers, but those macros expand to
the same inline assembly.)
Also send ANDROID_BAREMETAL + 32-bit Arm to OPENSSL_STATIC_ARMCAP. This
way we can remove OPENSSL_STATIC_ARMCAP from the Android baremetal build
without having to chase down constraining it to aarch64. See
b/291106677#comment6
Update-Note: This is a slight change to the OpenBSD build. Previously,
we assumed OpenBSD on 32-bit Arm implies NEON. Now, we pick it up from
the __ARM_NEON define from ACLE, i.e. whether the compiler has been told
that NEON is available. (This comes from -march or other options.) Doing
that is desirable anyway: if NEON is in your baseline, you should tell
your compiler so it can vectorize loops.
Change-Id: Icd43a2b56bb6e3f04f0fed996ae750fba65e3312
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62066
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Provide an implementation of OPENSSL_cpuid_setup() which identifies the
available CPU features required by the library directly from the system
registers for baremetal AArch64 builds without static initializer
(OPENSSL_NO_STATIC_INITIALIZER) that don't configure static capabilities
(OPENSSL_STATIC_ARMCAP). This assumes that the client code is NOT
running at exception level EL0 (userspace) and is enabled for
ANDROID_BAREMETAL.
Bug: b:265125189
Change-Id: Ifee6fbd24ece823a4661dd984f89473e1e1e3eda
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58586
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@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>
This warning was being tripped because lib_buf and reason_buf made GCC,
incorrectly, believe that the strings could get that long, and then
attempted to sum up the snprintf to 120, obtained by inlining some
things.
Those buffers were larger than they needed to be, so bringing it down is
sufficient to silence things. That said, the buffer bounds are supplied
by the caller and it is expected that truncation can occur, so the
warning is just incorrect. The warning can also be silenced by checking
the snprintf return value. As we're already trying to detect truncation,
we may as well do it with the return value and skip the extra strlen
call.
Either of the two changes is sufficient to suppress the warning, but
both seem worthwhile, so I've done them both.
Change-Id: Ia1b1de67bba55da6f0d07e3682165a1820ce2c9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61805
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
As part of this, factor out some of the socket bits. I tried to write
the sockaddr mess in a way that's strict-aliasing-clean, at least as far
as code we own goes. But the API is really not designed for it, and who
knows what effective type the underlying libc functions expect.
(Fortunately it's mostly syscalls, which definitely escape the
abstract machine.)
Change-Id: I12621f6c40f074ff7423dd46ddceca120ba63db9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61728
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These functions are just wrappers over BIO_ctrls, shared between the fd
and socket BIOs. Though we don't currently support one, it is
conceivable that there would be a platform with socket BIOs but not fd
BIOs. In that case, the BIO_get_fd function would still be useful to
implement SSL_get_rfd. (And someone could conceivably implement it in
another BIO.)
Bug: 629
Change-Id: I7ac3561f76af86af32d70b1c6265c4caeaecb129
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61727
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>
glibc and musl do not make getentropy available from <unistd.h> unless
_BSD_SOURCE (or _GNU_SOURCE) is defined. _BSD_SOURCE, in glibc, triggers
a deprecation warning to use _DEFAULT_SOURCE instead.
It seems _DEFAULT_SOURCE might be fairly broadly defined, but some
Emscripten-based toolchain (which uses musl) didn't end up defining
it for some reason. Just do it explicitly in the source file so it
always works.
Change-Id: I4532d4adb9f8ed55c43763ca2dd426b5fa1b4f5c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61625
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: 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>
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>
David Benjamin