Merge BoringSSL through 168777fbc49296eeb54b5da8fc7dfcae1a4d3e27 #2396
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://boringssl-review.googlesource.com/c/boringssl/+/74268 broke the SSSE3 GHASH fallback because it no longer maintained alignment of Htable. We had been relying on the memcpy to copy Htable into something aligned. Maintaining the alignment requirement without the memcpy is kind of a nuisance because it now leaks into EVP_AEAD_CTX. Since we don't have a good way to make caller-allocatable structs aligned, it would mean allocating 15 extra bytes and then finding the right position. Benchmarks shows that the alignment makes no difference on a Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz. Of course, this is artificial because that CPU would never run this code anyway. I recall adding the alignment requirement because it gave a bit of a perf boost on the old Mac Mini 2010 I was testing against, which actually is a CPU that would run it. I was able to dig it up, but apparently I no longer have a keyboard that's compatible with it. (That machine is also long EOL and cannot even run Chrome's minimum macOS version. Although its CPU may be representative for older Windows.) Regardless, I don't think it makes sense to expend this complexity for this. (See internal Chrome UMA Net.QuicSession.PreferAesGcm on Windows for the percentage of Windows that would be running this code. Though they should also be using ChaCha20-Poly1305 anyway.) Bug: 42290477 Change-Id: I4ef8c636bfc18200869f011ea50cc5d4988244ba Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74327 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
One more step towards making GCM128 into a coherent AES-GCM abstraction a pulling EVP_CIPHER and EVP_AEAD out of BCM. Bug: 42290602 Change-Id: I2efc6a57be194715b2b72d5eb60e4873de14f88b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74269 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The HKDFExtract and HKDFExpandLabel commands used for TLS 1.3 KDF ACVP testing are also parameterized by hash function, like the HKDF command. This commit updates their entries in the ACVP.md command table to reflect this. Change-Id: I3cfecfeacb94019a0731e77f3f0212d32145831f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74307 Reviewed-by: Bob Beck <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Bob Beck <[email protected]>
This is needed to cover the new VAES+AVX2 AES-GCM code. Change-Id: I3587113e9c761fd42cd317181e549428b0555050 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74367 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]>
We did this for Dilithium, but the tests themselves got lost for ML-DSA. This is just a matter of adding some declassifies so that the tests can survive running through with secret data marked secret. As for what to mark secret, I opted to mark: - Any internal secret output from the PRNG since that aligns better with crbug.com/42290551. Though it probably introduces a bunch of false positives on the TLS side because we haven't actually run all that through this yet. - Any *uniformly* secret inputs in test vectors. That is, seeds, but *not* the serialized long-form private keys (which ought to become seeds anyway). This is because a portion of those serializations include public keys and it's tricky to declassify that before the public key parser gets confused. To simplify comparisons, I added a Declassified() helper in test_util.h. I considered just making == on Bytes automatically declassify, but then we won't notice when we forget to, e.g. declassify ciphertext, so making the test do it explicitly seemed worthwhile? Change-Id: I2c53e25ca843ef876f2a89c15131a5b2b425603f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74387 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
…_prf The comment that a barrier-less reduce_once was safe turned out to not *quite* be true. In Clang configurations without auto-vectorization (notably -O1), Clang would emit a branch instead of a CMOV. Unfortunately, adding a barrier to reduce_once has significant performance costs. The problem seems to be that auto-vectorization breaks. I suspect it is primarily because the value barrier forces the value into a general-purpose register, while vectorized code puts it straight into a SIMD register. Though knowing the comparison is a comparison seems to also help a bit. Based on what we've understood of Clang's select transforms thus far, it would make sense that ML-KEM might not need the barrier. The main culprit is turning multiple selects with the same condition into a branch, and that does not happen in ML-KEM. Yet we observe a problem. Based on valgrind instrumentation, the problem seems to be limited to scalar_centered_binomial_distribution_eta_2_with_prf, likely because the value has such a limited range of values. For some reason, this causes many recent versions of Clang to emit a branch. I think this may actually be a misoptimization. Indeed the very latest trunk build of Clang on godbolt does not have this problem. Somewhere between 8cb44859cc31929521c09fc6a8add66d53db44de and 8daf4f16fa08b5d876e98108721dd1743a360326, LLVM seems to have fixed this issue. We can avoid this by computing it differently. We currently write reduce_once(kPrime + a + b - (c + d)), where a through d are 0 or 1. Instead, we can write a + b - (c + d), let the underflow happen, and then conditionally add kPrime based on the sign bit of the result. This seems to avoid mishaps, for now. If this breaks down again, we may need to get better value barriers, or to stop relying on auto-vectorization and vectorize ourselves. Change-Id: I917456348d63628880467d21138a57297532bc9a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74447 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Change-Id: I934bf60900ffe1f318e5929072844b26f4c35e44 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73667 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]>
It looks like seeds will be the official private key format for ML-DSA and ML-KEM. Thus parsing the weird private key format will only be needed for processing NIST's test vectors. Change-Id: Id6273214ba98b73aaf96640ec25ea289801b9bd7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73848 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]>
The referenced bug notes some ODR warnings in an unspecified
configuration. (Which I can't get AppleClang or GCC 14 to reproduce, at
least.) This is likely due to us switching to C++. The ODR rules in C++
are a little different, but also perhaps the detection is only kicking
in for C++.
Either way, structs in .cc files are generally not intended to leave
that compilation unit. (Unless it's an opaque struct listed in base.h.)
There's no such thing as `static struct` so this change wraps many
structs in .cc files in `namespace {`. There are also lots of structs in
test file. Those are less concerning, but test files should mostly
entirely be in a namespace so this change does that where possible for
test files containing structs.
Bug: 384186552
Change-Id: I6ccf715fbcdc3ea6260b5d5d05f305182b1a9450
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74407
Auto-Submit: Adam Langley <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Update-Note: BoringSSL now requires a C++17 toolchain. This aligns with Google's Foundational C++ Support Policy as it has now been 10 years since C++14 was published. See https://opensource.google/documentation/policies/cplusplus-support Bug: 42290600 Change-Id: I54b914ccdde2b61a76a4ebfbe67c1fb28f7ba6ac Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74467 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
This test used a data buffer of size 16*32, but calls the assembly
functions with lengths of up to 16*32+7. This was okay for
aesni_gcm_{en,de}crypt which round the length down to a multiple of 16.
But aes_gcm_{enc,dec}_update_vaes_avx10_{256,512} process the full
length, which caused a buffer overrun. Fix this.
Change-Id: Ib3651a38baff35cfd9624d24747a4e53cefe6cff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74507
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Bug: 42290600 Change-Id: I0c6fec6f33ff03ac2f60276920eb6b878751478d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74468 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
Quite a lot more things can probably be std::optional, but this had a TODO. Bug: 42290600 Change-Id: Ia5fc40d1dea4c11fa2bfef21d38643732a9bb98b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74469 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
It's standard as part of C++17 apparently. Bug: 42290600 Change-Id: Ibc2a990c5c7937fee5c096e7e0540a5cf853566a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74487 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
p256-nistz-table.h was manually editted for the C++ conversion without updating the generator. Also put the alignas marker in a slightly less surprising location. Change-Id: I5ac0d1852caa7b8608c856f4fe7438ea5499f872 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74488 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
…aders Except for our public headers, our files no longer need to be consumed by C. That means inline can just be inline, and with C++17, OPENSSL_UNUSED can be [[maybe_unused]]. Bug: 42290600 Change-Id: Ibdb309bc413660e10d075fbb71d4d1dd87101c6d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74489 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
It turns out to be useful for tests to be able to read this value back. Change-Id: Icf21144c230dc59f7548b7f75749509c8b646b4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74508 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: Adam Langley <[email protected]>
Later we'll probably want to square our preferred styles against this stricter requirement. (Maybe we should just prefer to put everything file-local in anonymous namespaces rather than mark things static? Not sure. I don't like that it's hard to see whether something is static, but if we have to use this for types anyway...) Bug: 384186552 Change-Id: Ie86a56b1c7358a32262f0b9c4edb3e503aa3d08a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74547 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
This is the same as what we did for ML-KEM earlier. The Kyber code will be removed in not too long, but since we haven't quite removed it yet, let's run it through the same thing. Change-Id: Ie0833d88fd49c4b475e1512d3f3b8f44c9e1fc66 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74567 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: David Benjamin <[email protected]>
The FIPS module will export a pair of general functions for the prehashed modes. We'll split the non-standard and standard modes at the public interface. Change-Id: Ic824342678cd0ccbe3c9bd25c1a676268de6e367 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73927 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Found by code inspection. If vsnprintf wanted to write INT_MAX characters, allocating a INT_MAX + 1 scratch buffer will overflow. Since we always have INT_MAX < SIZE_MAX, just casting to size_t earlier avoids this. (If the malloc implementation is unwilling to allocate INT_MAX + 1, e.g. it is forbidden to on 32-bit, that's malloc's responsibility to detect.) Change-Id: I3c2a740ebc7ecd58464a9f63858ffcefe67f648f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74247 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]>
go get -u ./...
go mod tidy
Fixed: 385044782
Change-Id: I0ade613aa778b53a55c658122e4351f924790ddf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74587
Commit-Queue: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Change-Id: Ibc52fbe362728134fce3c90ee47a23065a2e31b2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74087 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
It flags aes_ctr_set_key as having unreachable code in no-asm builds. It is true that there is unreachable code in there, but I'm unclear on why just that function is being flagged. Since we often will no-op our platform codepaths to avoid ifdefs in build configurations that don't want them, such a sensitive warning is not useful. Just turn it off. Fixed: 385161043 Change-Id: I5ed066d6d1d95dcc57a1cac01fad553e9ef4db7d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74607 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]>
Change-Id: I41638aa7a4d00415eda593fe277fed6f768170de Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73928 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Turns out that we need this one too. Change-Id: I9d9d8871f1a45576b1ef812207cb9ae44a376a2c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74509 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Don't take the change. Document that it is intentional.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2396 +/- ##
=======================================
Coverage 96.61% 96.62%
=======================================
Files 176 176
Lines 21640 21640
Branches 529 529
=======================================
+ Hits 20908 20909 +1
Misses 618 618
+ Partials 114 113 -1 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.