Bug: chromium:1322914
Change-Id: Ic5a1349013bcfb279e5fee9f9838c63558d663b7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63025
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@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>
For example, openssl/asm_base.h expands to include the line
.long ((1 << 0) | (1 << 1));
when BTI and PAC are enabled.
Change-Id: I07208e0430757721e97b88c706672375f8f58f1f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62525
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@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>
Rather than sample it from the UnwindCursor, we can just save it
immediately before starting the test.
Change-Id: Ica1eaa215755b0b772eaa08e03c5885aacec4f70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62866
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: 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>
Bug: chromium:1322914
Change-Id: I2efbb110747273188245530f9ab1964faba5201c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62825
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@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>
bug: chromium:1322914
Change-Id: I23b49ed6a9a739cddf17b0b4d9e26c74b7cb3de5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62785
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: 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>
Windows in chrome also does not like this
bug: chromium:1322914
Change-Id: I79c788e0b521964fdc07b530ec47d7fc3635e5a4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62765
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Windows gets mad otherwise
bug: chromium:1322914
Change-Id: I3f0409ff9b397cb6a888f8c81642737721912cb0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62706
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I215f9090e12314bcc3b0e15f5e83b751fea42003
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62726
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This way the Chromium certificate verifier can more easily use them.
Bug: chromium:1322914
Change-Id: I51dafc4e70d74da8543688b6457563d78e298150
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62745
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: I18ba860d28dd3fb55cc14904758d6a8dc95e3f89
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62725
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I368e98f7484bdafac8d8600a6b4d5d7013e08817
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62705
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: 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>
Change-Id: I295b0142b4448a5ee10ca9b092a2c3eaa1fffc86
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60405
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@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>
While cmake does not care and happily build anyway, Bazel gets
very upset when faced with this in it's generated files from this.
Crbug: 1322914
Change-Id: Ia564be8dfd81bd206b80996bc660113da04de314
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62505
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
This also removes fillins/check.h which is no longer needed.
Crbug: 1322914
Change-Id: If5e8355700472bf6703c80809ea276c4c07ddc52
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62485
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Bug: b:296302767
Change-Id: I247c02b6b8fbab38f254c9d74576d0b103d93b4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62425
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
we do end up needing this for google3.
crbug: 1322914
Change-Id: I3788170521fff6a7a8075c58d929558b97820a34
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62405
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: I43283162ef356f9e7fb959dbc1ec9e0e98ee83ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62385
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@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>
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>
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>
I think this dates to when CRYPTO_is_*_capable were inline functions in
public headers, so they couldn't access OPENSSL_armcap_P directly. Now
they can.
Change-Id: Ic06fffa7f5056401118b62d690dfe6b21bc30f86
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62345
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@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>
Change-Id: I235d81c6e6b013b25488355ccd5de254e7c172b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62306
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@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>