Update to upstream f10c1dc #122
Merged
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
Change-Id: I4f349d2215c9cdea947f2e982b1601d022744c98 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70167 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Section 5.9 of RFC 9147 changes the TLS 1.3 key schedule for DTLS 1.3 by changing the label prefix from "tls13 " to "dtls13". Bug: 715 Change-Id: Ia3c84d27145a225d27dd5bc082361273ce7e6dbc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70007 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: Nick Harper <[email protected]> Commit-Queue: Bob Beck <[email protected]>
The comment says something about adding assembly for other ISAs, but it seems most ISAs don't actually have double-wide division instructions. (Despite this, the division-based BN_MONT_CTX_set still seems to beat the Montgomery one on Arm. Less drastically than before https://boringssl-review.googlesource.com/c/boringssl/+/60686, but division still makes things faster.) Also update the bug links post LLVM's GitHub migration. Finding the corresponding GitHub issue is not always trivial. Bug: 358687140 Change-Id: Iafb5118461a2c09c66840a44fbd257320a8d98b4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70168 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This is part of the public API and should be documented as such. Bug: 358687140 Change-Id: I1d736f39c5cff18f7c8e3ff8207a4b60ee96cd18 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70169 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
After spending a while trying to divine where all the bounds came from, and coming up with some of the messy proofs for why it works, I found this exact algorithm in Knuth, Volume 2, with... different messy proofs. Sadly, this algorithm seems to just be messy. Cite it as reference rather than trying to repeat it in code. As part of this, update the discussion on branches. That was added in https://boringssl-review.googlesource.com/c/boringssl/+/9105, back when BN_div was used on secret inputs. It no longer is and, back then, the function still wasn't constant-time anyway. We could, in principle, restore the special cases now. But this would be more complicated and diverge from Knuth's formulation, so let's just keep it simple. (Although it might actually be a hair faster. We care about this function to compute R^2 mod n, and the special case would save an extra iteration through the loop. Though I think that optimization could actually be restored with much, much less code than OpenSSL originally did it. Probably not worth the fuss.) Subsequent CLs will clean this code up in reference to Knuth's formulation. Bug: 358687140 Change-Id: I56da99c560b845f1736ab86edc79b8e711890fe3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70170 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
82f9853 replaced ssl_seal_align_prefix_len with two functions, tls_seal_align_prefix_len and dtls_seal_align_prefix_len. This change updates documentation that referred to the old ssl_seal_align_prefix_len function to refer to the correct function. Change-Id: Ieb8891eff03efc3d894aa56729ae6e47f4be3288 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70207 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]> Auto-Submit: Nick Harper <[email protected]>
By having the caller provide the sequence number and the record header length, the decrypt function doesn't need to know anything about the format of the record header. Change-Id: If3389e79d6823c63c884bb9ddb764fa68223e765 Bug: 715 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69948 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
The length of the DTLS record header isn't a constant - update variables and functions to match that reality. Change-Id: Ib6abc3af98a15994c72a22b8fdd8e230e87b966a Bug: 715 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69949 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
This was just checking the bn_correct_top invariant. But since we got rid of the bn_correct_top invariant and dynamically compute bn_minimal_width anyway, bn_minimal_width will always be computed such that the check succeeds. Bug: 358687140 Change-Id: Idc1abbc46c38d47f319ee5835a5a601a8a3d9c0e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70171 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Also add an assert for the invariant it is maintaining. Bug: 358687140 Change-Id: I3bcb9838198735b6f42e4f732b00e0fc990c5ffd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70172 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Bug: 358687140 Change-Id: Ifbc8bf34a93543c6035bfee29d915818ef2875db Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70173 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Although Knuth iterates the index forwards, it makes more sense for us to do it backwards because he numbers words big-endian and we use little-endian. Ultimately each loop iteration i is about computing res->d[i] in the quotient. Once we do that, we can assert some pointer invariants. Subsequent CLs will remove some of the pointers. The compiler can figure it out and they make it harder to even confirm we stay within bounds. Bug: 358687140 Change-Id: I159489fafb8b071725c0e49a6fea66d6006f5a78 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70174 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
We can use bn_resize_words, which zeros the extra words and updates the width in one step. Also clarify what this is achieving. It's to establish a bunch of invariants that the loop cares about. Bug: 358687140 Change-Id: Id78e81bc08a1ca506b5d6ef6b01936f860fddd86 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70175 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
It's much clearer if we just reference res->d[i] directly. Note that the removed res->neg clearing is a no-op because bn_set_minimal_width fills the value in anyway. It was also impossible for res->width to be zero because of the resizing step (see the bn_resize_words call). Even if it were possible for it to be zero, that would mean the loop doesn't run, and the resp pointer was only read outside the loop. So we can treat the function as if it unconditionally decremented resp. Bug: 358687140 Change-Id: I5e2d4ca03fd808cacd4f4647843a7894bf7a2f05 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70176 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
We allocated two more words than were needed. Sizing it more than the width is suspicious and with the confusing pointer indirection removed, it becomes clear that, throughout the entire function, we only ever write to indices 0 through loop-2. That is, it should be sized for loop-1. Bug: 358687140 Change-Id: I9e33ce7d2c4e5b6fae9ec59bdee34b2d3480addc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70177 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
According to Intel's documentation, if not all the AVX512 bits in XCR0 are set (meaning that the operating system doesn't fully support AVX512), then no AVX512 feature can be used, even on xmm and ymm registers. Make OPENSSL_cpuid_setup() correctly handle this case by clearing all the AVX512 feature bits when this situation is detected. Change-Id: I2774dbc28bfbac1196e405c0920ba2909e7f0eb3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68907 Reviewed-by: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]> Auto-Submit: Eric Biggers <[email protected]>
Change-Id: Icdd1192a24d3bdc62198ca9243f4bbf9f64f3c29 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70287 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
This is necessary to avoid the following error when building with MSVC
using the latest STL:
error C2039: 'string': is not a member of 'std'
Change-Id: I4c926f7a020c2e920bcc78667bc04951cdab4cf1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70272
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
With reference to Knuth. I'm not sure what the comment about overflowing when q = 0 is about. The bounds in "the first part of the loop" imply that we've either computed q+1 or q and the borrow check exactly captures the q+1 case. Moreover, this addition is expected to *always* overflow. It cancels out the underflow from subtracting too many. Bug: 358687140 Change-Id: I24bf8c9c37dcd1145667d7f0e8457c0e63e8783c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70178 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
These files were derived from CAVP, which used DOS line endings. Several environments will automatically convert line endings and this leads to a mismatch if strictly comparing against the offical FIPS source distribution tarballs. Thus convert these files to UNIX line endings, matching the rest of the repository. Change-Id: If0f5835108a6b26bba5de0b6b950a69a4faa1410 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70307 Auto-Submit: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]>
This is needed to cover the 512-bit code path in the new AES-GCM code. Change-Id: I1a0eeb7cd6f330d82577159a1e0055f2ff6ec4ce Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70247 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]>
This extra word was allocated so that the fixup portion of quotient estimation could read from wnump[-2] without checking if div_n > 1. This was actually subtle because the value it got back was wrong. It just didn't matter because the loop was a no-op. As a result of all this, all the indices into snum were off, and the remainder needed to be shifted down by one word to compensate. Really, if div_n > 1, we could just call BN_div_word, but the calling conventions are different enough that it didn't seem worth the effort. Bug: 358687140 Change-Id: Id694a33003f51536ee836a5bdb75ff8006b11a51 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70179 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The DTLS 1.3 record header is formatted differently than the old record header, but the code to read/process a DTLS record mixes record header parsing with other record processing code. This change provides a clear delineation between processing the record header and processing the record, which will assist in adding support for the DTLS 1.3 record header. Bug: 715 Change-Id: I13a0bb5c184e79b88f064e9ac8ecbc82eb56750a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69950 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Expressing everything in terms of i makes it at lot easier to tell what words are being written to where, and convince oneself that everything stays in bounds. I kept a wnum variable in there since it's used so frequently but added a note about the bounds. In a higher-level language, wnum would be a slice of width div_n + 1. Bug: 358687140 Change-Id: Iae39b1915f80008ab5ed91e1e7fc5cd1349e8c1e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70227 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Tidy up the setup. Also we can simplify all the sign management. If snum and sdiv just preserve the sign bits of numerator and denominator, the remainder will have the correct sign from the start. (The original code called BN_cmp and BN_add in places, which is sensitive to the sign.) Fixed: 358687140 Change-Id: I2d5f952814c9910552330b18462796ffc3fe5dab Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70228 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Bug: 342657857 Test: Validated pulling & using Change-Id: I5b6dda58b21cf237e66064a7da2fdc8003fa047b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70273 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This change implements FIPS 204. Change-Id: I0043850767c93cc7235a15c701798fee6e1af1bf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69987 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Bug: 715 Change-Id: I69c82eed41946da404fb13129aa790d61ec0fb78 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69689 Auto-Submit: Nick Harper <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Now that ML-DSA has been standardized, code should be using <openssl/mldsa.h> not <openssl/experimental/dilithium.h>. This marks the dilithium functions as OPENSSL_DEPRECATED and removes the dilithium speed from bssl. The code remains in the library for a short while to allow anyone who used it to transition to mldsa. Change-Id: I5c9fab376185dc045d7d588eff4b6a626527aff5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70329 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: Bob Beck <[email protected]>
This change implements FIPS 203. This marks the first use of C++ in libcrypto. If you can't compile C++ in this context, please reach out to boringssl@ and filter out the .cc files for now. This also makes marshaling a private key an internal function and, instead, exposes the seed from the generation process and a function to calculate a private key from a seed. Seeds are significantly smaller than NIST's format for private keys and don't require validation. On an M1 Pro: Did 22320 Kyber generate + decap operations in 1001900us (22277.7 ops/sec) Did 39000 Kyber parse + encap operations in 1005523us (38785.8 ops/sec) Did 22608 ML-KEM-768 generate + decap operations in 1010509us (22372.9 ops/sec) Did 44000 ML-KEM-768 parse + encap operations in 1013729us (43404.1 ops/sec) Did 15410 ML-KEM-1024 generate + decap operations in 1011500us (15234.8 ops/sec) Did 29000 ML-KEM-1024 parse + encap operations in 1003919us (28886.8 ops/sec) Change-Id: Ib563bd4d45228237b55cedbe7d7fdf0f0221a3cc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69928 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Some functions try to accomodate negative moduli by figuring out whether to BN_add or BN_sub. Under the hood, those functions will do further sign bits and comparisons to decide whether to BN_uadd or BN_usub. We can just call the right one from the start. Change-Id: I2e64b05522c93ee831f6d6e9f7d1380411fbb71b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70813 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The inputs may be negative, it just ignores the sign bits. Change-Id: Icaab47c159e45ab2e6fe2d770188767976aff521 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70812 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
I should have removed this when the OPENSSL_ia32cap_P dispatch was removed from the ChaCha20-Poly1305 assembly. Bug: 42290548 Change-Id: Ic2ca0f6a897c27974833155935e42189fcbc1494 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70887 Commit-Queue: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
The numerator is per shard, but the denominator wasn't. Change-Id: I1afd784038c51b8db51192b9a2b391073675e390 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70867 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]>
We can't have two source files with the same name, it seems, so since crypto/spx/ will be going away, move its files out of the way so that SLH-DSA can use those names. Change-Id: Iedee8453cb77291eeff5ec33aa9836ea5d00d9a2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70908 Auto-Submit: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This isn't part of fips, so we move it to digest_extra Change-Id: Ia9aeb81c314bdb34c6c9bd567242c90821f372d0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70707 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]>
libssl's error-handling is one of the most difficult things to get right with this API. Leave some more notes, in case the reader does not know what "error queue" means. Change-Id: I91464ccdc12bf9e05ac9ed61930bc733244a9b36 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70929 Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
We weren't testing all kinds of load-bearing checks in the TLS stack around key shares. Fix this. - Rework runner's KEM abstraction so that all the operations can get at config. That saves a lot of manual plumbing. - Make the bad ECDH point something not on the curve. That's a bit more interesting of a test case. - Test X25519 low order point rejection. - Test truncating and extending the key share for all cases. - Run X25519 tests in X25519-based hybrids as well. Bug: 40910498 Change-Id: I93907dbb4bd4177252376c8efb859de6db3c4189 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70927 Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
MD5 is no longer approved as part of fips, so we move it to digest_extra. Change-Id: I504c3d0d381cba72345c615209b99d4451886d96 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70727 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Bob Beck <[email protected]>
pregenerate does not ensure that now unused files are gone. crbug.com/365169741 Change-Id: I3876fe60576c27dac9571e0473d807fd8c86fb80 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71007 Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Bug: chromium:365320414 Change-Id: I5d18425257dd87fe33a72dece194c2fe1977e51d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71067 Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
|
It looks like this update adds support for Google's implementation of ML-DSA. How does this interact with the liboqs implementation? |
|
@SWilson4 |
baentsch
reviewed
Sep 12, 2024
baentsch
requested changes
Sep 12, 2024
There was a problem hiding this comment.
I don't mind landing this -- but this blurs the line substantially in regard to the relationship to OQS: The new code surely does not use liboqs exclusively for the PQ crypto any more, so I'm afraid there's a significant README update required if this lands: Something along the lines: This uses boringSSL crypto for standardized crypto algorithms and OQS for non-standardized PQ operations -- also the algorithms' lists now ought to be suitably "dissected".
add a note about Google's implementations of quantum-safe crypto algorithms Signed-off-by: PI <[email protected]>
|
@baentsch Although Google provides an implementation of mlkem, I can configure OQS-BoringSSL to ignore the existing Google's implementation and use the liboqs implementation instead. However, I do NOT plan to do this because using Google's implementation offers broader compatibility. |
|
Thank you for this work @pi-314159!
What you say makes sense as a default. How about a build option to choose which implementation is used? It may be of research interest to be able to use a specific implementation of ML-KEM/ML-DSA with boringssl/chromium. For instance, liboqs gets formally verified implementations from libjade (only Kyber at the moment but this will likely include ML-KEM and ML-DSA in the future); one may want to use such a formally verified implementation of ML-KEM instead. |
|
@praveksharma Good suggestion! I'm quite busy this year and don't think I'll have the time to add that build option. I'll check that out when I have some free time, or perhaps someone else could look into adding that option. Essentially, we need to modify |
baentsch
reviewed
Sep 13, 2024
Reasonable suggestion @praveksharma -- assuming a) OQS retains its "research mission" (bringing in/trialing new PQC) and b) such implementations (for standardized algorithms) are available (not the case right now if I'm not mistaken). Hence I'd suggest you create an issue asking for this feature (possibly across the board of all OQS sub projects): I'd call it "build option to override built-in PQC" (as this issue will begin to appear in all sub projects, maybe with the exception of |
baentsch
approved these changes
Sep 13, 2024
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.
Note this implements x25519-mlkem768