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 1e2f1696636088626cb223aa5a10f64e07b62ffd. 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>
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>
Change-Id: I05a3b0cb7ffaee90ed85d2cf795feded8fbad1df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62305
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@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>
Change-Id: Ib65febca30ce312f2c8fd6d6dbc85f24987b50d8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62245
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I203a19b59c23def9bca6f01c2b6e8c885b0c9c3f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62205
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Use CSlice instead of a regular Rust slice when passing pointers to C
FFI.
Change-Id: Iccd827f4c6f005d860993e97fef5e9caf514885b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60525
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
I think there are a couple projects remaining that still use the old
lists here, but they're in repositories we don't spend as much time in,
and it should be straightforward for them to update when they get here.
Removing these should put us in a good place to check in pre-generated
asm lists. While I'm here, fix a few typos in TODOs I previously added.
Update-Note: If you're one of those projects and have trouble switching
to the new lists, let us know.
Bug: 542
Change-Id: I57559bafc85eceacc7a237e2f29db6eaf492a8cb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62186
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The android-cmake one should no longer be needed as of aosp/2673299, and
the JSON one as of https://github.com/grpc/grpc/pull/33700
Bug: 542
Change-Id: I3c7b752806d82a5a01b5ad9180771e88d2810b70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62185
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
