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>
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>
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>
Referencing a variable in a closure captures it by _address_. So
referencing a loop variable can go horribly wrong:
https://go.dev/play/p/f2ivPAIN_bG
This is accepted as essentially a bug by Go and will be fixed in a
future release (https://github.com/golang/go/wiki/LoopvarExperiment).
But, for now at least, work around it.
Our tests trim the ACVP inputs to only have a single test case per group
in many cases, which hides most of this issue from tests. When we run
run full ACVP sets, our modulewrapper is seemingly fast enough not to
notice there either. But I've updated one of the tests here by
duplicating a test case enough that it catches this a meaningful amount
of the time.
Change-Id: I8216c00f67636ab7dad927eae4b49ae45ae3cf31
Bug: 646
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62965
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@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>
Handshake hints work fine with TLS 1.2 resumption now. Also split
handshakes is really really dangerous, and I think hints has survived
long enough that we can just declare it the successor.
Change-Id: Ib5fe5e1b030034b853a96c3404608c56d7b7a7c7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62925
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
We have a corresponding check on the ServerHello, but not
HelloRetryRequest. See also https://github.com/rustls/rustls/pull/1374,
where rustls forgot to apply the compatibility logic to
HelloRetryRequest.
(From the perspective of a TLS-1.2-expecting observer, HelloRetryRequest
is the ServerHello, so encoding hacks need to apply to both.)
Change-Id: I9b711ea45c54770a76ecfbca8bc992a4eaef6fcd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62906
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This reverts 1e2f169663. Bazel 6.3 has
since been released, which includes a fix for
https://github.com/bazelbuild/bazel/issues/15073. Envoy and gRPC have
both since updated to this Bazel version. The policies in
https://opensource.google/documentation/policies/cplusplus-support#build_systems
also imply a minimum Bazel version of 6.3.2.
I'm thinking we let this bake for a little while, to catch any
unexpected issues, and then, if it sticks, we try to go ahead and
require C11 across the board.
Update-Note: If using Bazel with MSVC, and the build fails with
something like "Command line error D8016 : '/std:c++20' and '/std:c11'
command-line options are incompatible", you are likely running into the
above Bazel bug. Update to Bazel 6.3 or later.
Bug: 623, 624
Change-Id: I8baa99392ca47bc7580bc2930e7f4b16beced91e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62905
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@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>
Although it works without these (we just refer the unwinder to the red
zone), older versions of libunwind seem to have a bug that cause it to
flakily fail to restore rbx without this. I've attempted to bisect the
problem, but the issue is very flaky and I've failed to find the culprit
four times now, so just give up and work around it. Explicit restores
match what we do in other files.
Hopefully this will clear some issues tha fiat-crypto's CI are running
into.
Change-Id: I6a19679a37cad8e93e6dee554b6a9b3b9b4bbe4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62865
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
RBP pointed 8 bytes off of where it should be. I've left the RSP offsets
alone, though it does mean they're shifted by 8 from what they
previously were. Per Andres, the new version of CryptOpt will generate
an RBP-compatible prolog, but for now I've just fixed it up by hand.
(This part was already hand-written.)
Change-Id: I23720e76affff6fae46b8f85b0a509380ccc8bc0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62805
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@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>
Where the Trusty and Android baremetal cases are unambiguously mistakes
in their respective builds, nanolibc is a bit more interesting.
nanolibc sometimes build for a non-Linux target (which should not define
__linux__), but also sometimes build for Linux. Although technically
running in Linux userspace, this lacks all the libc APIs we'd normally
expect on Linux, so we treat it as a non-Linux target.
Change-Id: Id36f6bbc6e790d96e31193532717630a86f124b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62685
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: 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>
Newer glibc have an attribute((nonnull(1))) on fclose. Attributes aren't
part of the language, so decltype(fclose) lose the attribute. It seems
this causes std::unique_ptr<FILE, decltype(fclose)> to trip
-Wignored-attributes in GCC.
This is a bit aggressive of a warning, but work around this with a
custom deleter, which makes the unique_ptr object smaller anyway.
(Though the compiler can, I hope, dissolve all of this anyway.)
Fixed: 642
Change-Id: I9a0206a8c5675f856e80c5266c90be42d66a5606
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62465
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@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>
Implemented a generic Aead trait and struct against the EVP_AEAD
API's, which can be used to provide bindings to all of the AEAD's
provided by boringssl. Starting with AES_GCM_SIV, but will expand
to more AEAD's.
Change-Id: I7d4113f3d49ff40de3ccb76424f9a25d25797e82
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59965
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@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>
766C6164