Merge BoringSSL through a43c76dbe30d619188dc685b7d432a92e7c2b66b #1654
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
FileTests run sequentially and cannot be filtered. Split them up so it's easier to, say, just run the ModExp ones. Also our test sharding machinery will do a slightly better job parallelizing them when split up like this. (This is one of our slower tests.) Change-Id: Ie69864982f043655f68e592440b1f36e971b033a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55230 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
I did a go mod tidy run because https://go.dev/doc/modules/gomod-ref#go mentions something about transitive dependencies being noted differently. Fixed: 544 Change-Id: Ie631d83b8bb5e94f4ab7d47ae5d4eb4cc0b4ac06 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55365 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Change-Id: I57a65d26c2a69f7084ea80b1a565ed7cb89b2a72 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55228 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Sadly we do have to keep existing uses working, but let's make it clear in the documentation that they're not a priority. Also tweak the text about being limited by memory; we actually impose a tighter limit than memory alone. Change-Id: Ibaccd91cd0a1fe354f93f0123497115b649c0630 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55265 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Since 31dcfcd, delocate can drive cpp itself to preprocess assembly inputs. This change switches the CMake build to doing that and does it on all platforms in order to be more uniform. Change-Id: Ie28228fb1a4c63a2d43ab8a97f09cfe890ef39a1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55326 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Some compiler configuration are manipulating the 16-bit register `h0` and delocate needs to know that's a register, not a symbol. Likewise, NZCV is Aarch64's condition flags register which can be (and is) read with `msr`. Change-Id: Ica5be6a059ead61d22d60fd2db1a484d9ac2be3b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55307 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
powerbuf's layout is: - num_powers values mod m, stored transposed - one value mod m, tmp - one value mod m, am - (mont5-only) an extra copy of m powerbuf_len broadly computed this, but where tmp + am would be sizeof(BN_ULONG) * top * 2, it used sizeof(BN_ULONG) * max(top * 2, num_powers). (In the actual code it's written as a ternary op and some multiplications are factored out.) That is, it allocated enough room for tmp + am OR an extra row in the num_powers table, as if each entry were top + 1 words long instead of top, with the space overlapping. This expression dates to upstream's openssl/openssl@361512d, though the exact layout has shifted over the years as mont5 evolved. (Originally, it only contained one extra value mod m.) At the time, this was necessary because bn_mul_mont_gather5 actually overreads the table by one row! Although it only uses top * 32 words, it requires the table to have (top + 1) * 32 words. This is because the computation was scheduled so that the .Louter4x loop would read and mask off the next iteration's value while incorporating the previous iteration: There were masked reads from $bp into XMM registers at the start of the loop: https://github.com/openssl/openssl/blob/361512da0d900ba276096cbd152e304d402aca65/crypto/bn/asm/x86_64-mont5.pl#L541 The XMM logic is interleaved throughout and does not move to a general-purpose register, $m0, until much later. $m is not read again until after the jump. https://github.com/openssl/openssl/blob/361512da0d900ba276096cbd152e304d402aca65/crypto/bn/asm/x86_64-mont5.pl#L700 Meanwhile, the loop is already reading $m0 at the start of the iteration. https://github.com/openssl/openssl/blob/361512da0d900ba276096cbd152e304d402aca65/crypto/bn/asm/x86_64-mont5.pl#L551 The whole thing is bootstrapped by similar code just above it: https://github.com/openssl/openssl/blob/361512da0d900ba276096cbd152e304d402aca65/crypto/bn/asm/x86_64-mont5.pl#L531 In the final iteration, we read one extra row into $m0 but never use it. That is the overread. I also confirmed this by rewinding our x86_64-mont5.pl to this state, hacking things up until it built, and then hacking up BN_mod_exp_mont_consttime to place the table in its own allocation, with no extra slop using C11 aligned_alloc. This was so valgrind could accurately instrument the bounds. When I did that, valgrind was clean if I allocated (top + 1) * num_powers, but flagged an out-of-bounds read at top * num_powers. This no longer applies. After openssl/openssl@25d14c6, bn_mul_mont_gather5's scheduling is far less complicated. .Louter4x now begins with a masked read, setting up $m0, and then it incorporates $m0 into the product. The same valgrind strategy confirmed this. Thus, I don't believe this extra row is needed and we can allocate the buffer straightforwardly. Change-Id: I6c1ee8d5ebdb66eb4e5fec63d2140814c13ae146 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55231 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
We have two places where the current cap on BIGNUM sizes (64 MiB) is too large, both involving Montgomery reduction: bn_mul_mont allocates a spare value on the stack, and BN_mod_exp_mont_constime needs to allocate a buffer of up to 64 contiguous values, which may overflow an int. Make BN_MONT_CTX reject any BIGNUM larger than 8 KiB. This is 65,536 bits which is well above our maximum RSA key size, 16,384 bits. Ideally we'd just apply this in bn_wexpand, to all BIGNUMs across the board, but we found one caller that depends on creating an 8 MiB BIGNUM. Update-Note: This will not affect any cryptography implemented by BoringSSL, such as RSA, but other callers may run into this limit. If necessary, we can raise this a bit, but the stack allocation means we don't want to go *significantly* beyond what's in this CL. Fixed: 541 Change-Id: Ia00f3ea6714a5042434f446943db55a533752dc5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55266 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
It's weird to have both "in" and "input" in the same function. Also the vector const refs can be spans. This is a bit of preparatory cleanup along the way to a larger refactor so we can do negative tests and test the weird EVP_Cipher API more coherently. Bug: 494 Change-Id: I5cddf1b3ab88b3419bd88ce15bee56a2016bcd57 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55385 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
EVP_CIPH_NO_PADDING is a no-op when block_size is one, yet we sized the output expecting it to always add a byte of padding. (I don't think this makes a difference because most call sites of DoCipher set EVP_CIPH_NO_PADDING.) Bug: 494 Change-Id: Ic75e48a60e669270a093416b862ec03706e1d6ef Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55386 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The main FileTest callback didn't support invalid ciphertexts, while the Wycheproof one did but reimplemented a lot of logic to discover variations. Rework this a bit so we have a single TestCipher function that's agnostic to the FileTest setup. The Operation enum gains a kInvalidDecrypt mode to specify that the ciphertext is invalid. As part of this, we tighten up the test This is to make the later changes easier: 1. We don't have any negative tests for EVP_CIPHER's tag check at all! We only test it for EVP_AEAD right now. Now TestCipher can express this. 2. The weird EVP_Cipher API has no tests. It has a weird calling convention that was easier to test if calling code knew whether to expect success or failure. Previously, kInvalidDecrypt was implemented by checking if DoCipher failed, which makes it harder to embed success/failure-specific assertions. Along the way, I've made each function responsible for running through the input once, with separate TestCipherAPI and TestLowLevelAPI helpers. Otherwise we have to reset the spans and keep track of the state of intermediate values. Bug: 494 Change-Id: Ieab87c0c76586aefeb596cc90edd4910ff1b962f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55387 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
The most likely use of EVP_CIPHER_CTX copy is to work around EVP_CIPHER_CTX's lack of separation between "key" and "pending operation". That is, you'd probably first configure the key, save that as the precomputed AES key schedule, and then mint copies for each operation to avoid mutating your key-only EVP_CIPHER_CTX. Rearrange things to test that point. But, while I'm here, we can just sprinkle copies throughout the whole operation and ensure it still works. Bug: 494 Change-Id: I464aa265cee317020d6b3ae3fd2ebfa92d7e12ae Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55388 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
See upstream's 70d8b304d01b9e0c4ec182db20c33aa0698cda51. Also import upstream's AES-192-GCM tests, now that we support that in EVP_CIPHER too. Bug: 494 Change-Id: I157ebe23f32147214641aeb664ce7db2e21bf86a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55389 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This API supports that too, so we should test it. Bug: 494 Change-Id: If978705b3120697cc18fc91c06ca950e0e93bcf3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55390 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
It would be nice to have a single-shot EVP_CIPHER_CTX API. This function is not it. EVP_Cipher is absurd. It's actually just exposing the internal EVP_CIPHER 'cipher' callback, whose calling convention is extremely complex. We've currently documented it as a "single-shot" API, but it's not single-shot either, as it does update cipher state. It just can't update across block boundaries. It is particularly bizarre for "custom ciphers", which include AEADs, which completely changes the return value convention from bytes_written/-1 to 1/0, but also adds a bunch of magic NULL behaviors: - out == NULL, in != NULL: supply AAD - out != NULL, in != NULL: bulk encrypt/decrypt - out == NULL, in == NULL: compute/check the tag Moreover, existing code, like OpenSSH, relies on this behavior. To ensure we don't break it when refactoring EVP_CIPHER internals, capture the current behavior in tests. But also, no one should be using this in new code, so deprecate it. Upstream hasn't quite deprecated it, they now say "Due to the constraints of the API contract of this function it shouldn't be used in applications, please consider using EVP_CipherUpdate() and EVP_CipherFinal_ex() instead." Bug: 494 Change-Id: Icfe39a8fbbc860b03c9861f4164b7ee8da340216 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55391 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Used on the Android platform. Change-Id: I99f1f56c6a09852ec918816591371426390f1873 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55405 Commit-Queue: Pete Bentley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Interestingly, the Chromium version is a bit behind the default. I've set it to match the Chromium version. Bug: chromium:1340825 Change-Id: Ia956e42897add5849dfe09ec8e3a19f704ed9641 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55425 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
SSL_SIGN_RSA_PKCS1_MD5_SHA1 does not really exist, but is a private use value we allocated to internally represent the TLS 1.0/1.1 RSA signature algorithm. (Unlike the TLS 1.0/1.1 ECDSA signature algorithm, which is the same as SSL_SIGN_ECDSA_SHA1, the RSA one is a bespoke MD5+SHA1 concatenation which never appears in TLS 1.2 and up.) Although documented that you're not to use it with SSL_CTX_set_verify_algorithm_prefs and SSL_CTX_set_signing_algorithm_prefs (it only exists for SSL_PRIVATE_KEY_METHOD), there's nothing stopping a caller from passing it in. Were you to do so anyway, we'd get confused and sign or verify it at TLS 1.2. This CL is the first half of a fix: since we already have pkey_supports_algorithm that checks a (version, sigalg, key) tuple, that function should just know this is not a 1.2-compatible algorithm. A subsequent CL will also fix those APIs to not accept invalid values from the caller, since these invalid calls will still, e.g., dump garbage values on the wire. Change-Id: I119503f9742a17952ed08e5815fb3d1419fd4a12 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55445 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]>
It should not be possible to make BoringSSL request unknown signature algorithms, or the special SSL_SIGN_RSA_PKCS1_MD5_SHA1 value, in the ClientHello or CertificateRequest. Update-Note: This CL makes unknown values fail SSL_set_verify_algorithm_prefs, etc. SSL_SIGN_RSA_PKCS1_MD5_SHA1 is silently dropped from the list, rather than an error because, although documented as incorrect, this hole in the abstraction seems to be confusing. I think there's some code in Chromium which accidentally puts it in the signing prefs (wrong but harmless) and I often need to explain to folks that it doesn't belowing in verify prefs (puts it in the ClientHello). This makes us tolerate the value by ignoring it. This makes the previous pkey_supports_algorithm change moot because we'd never get that far with SSL_SIGN_RSA_PKCS1_MD5_SHA1, but I think the check, but I think the check belongs in that function too. The test also reveals that some of our tests have been accidentally passing zero into the preference list all this time. Change-Id: I76d4eb98682515c3b819e0ed8d44f2d708a98975 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55446 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
We currently shift between unsigned long and int. Bug: 516 Change-Id: I9e3fcc9393e24a352a2c08b9df0650a508d7a60b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55448 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]>
tag and utype are always accessed as int, so make the structs match. Boolean ASN1_ITEMs put an ASN1_BOOLEAN in it->size, so add a cast. Also fix the time set_string functions to call the underlying CBS parser directly, so they don't need to put a strlen into an int. Bug: 516 Change-Id: Ie10e7eaf58ec0b0dec59813a0ddcb0197fce1fd1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55449 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Some of them were flagging -Wshorten-64-to-32 warnings. None of these values are long, so just remove them. (I suspect this assumes unsigned int is at least 32-bit, but we already assume this rather than wrap all 32-bit constants in UINT32_C(x).) Ideally the c2l and l2c macros could be replaced with the load/store functions but, like with the ciphers in decrepit, this is probably not worth the effort for DES. Bug: 516 Change-Id: I19e8cd4a321c20b9a22e4c007d943185c10755bb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55450 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Along the way, this fixes some size_t truncations. Bug: 516 Change-Id: Iff0cf6ced0b7deb4c48c268e051a4da433088056 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55453 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
We were mixing uint64_t and unsigned, which flagged -Wshorten-64-to-32. While I'm here, switch the iteration count to uint64_t to cut down on uses of 'unsigned'. While we have no real risk of overflow a u32 here, counting the number of times we perform some operation in a loop would probably best be u64. (I'm guessing we originally used unsigned mostly so that %u worked. But PRIu64 exists, though it's wordy.) Bug: 516 Change-Id: I6abc24ecb029c2c223bb940c903d497868bab9fc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55455 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
BIO_ctrl is one of OpenSSL's ioctl-style patterns, where they jam many different function signatures into one type. BIO_ctrl returns long for the sake of other operations, but many of them are only allowed to return int. Bug: 516 Change-Id: Ieffad1da89c60a538f142b12bdebdb950efd5c6a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55454 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
- Adds the full set of architectures for Linux for which there are assembly sources listed. - Adds Android, mostly parallel to Linux. - Adds the other Apple OSs, parallel to macOS. Bug: 531 Change-Id: I8bb609d3563b2d151a404f8468b4c6b22c2692f9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55485 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Bug: b:261632678, chromium:1396479 Change-Id: I82f7ce05ece8b5c145d4394dc0d4173e357ce176 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55585 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Less code, and internally handles overflows. (Although this one cannot overflow.) Bug: 516 Change-Id: I3c2718075689d2815a43534a578a323c52787223 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55452 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
That test cert expires in 2099, which is a ways off but if this code is somehow still around by then, let's save the future some pain. With this fixed, our test all pass at least through the year 3000, so we're hopefully clear of timebombs. Change-Id: Ie9dcbc4f4db70c6bcc1ae9717c6e1ee89eb4195c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55625 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]>
For unknown reasons, ACVP now tests HKDF differently. This change updates to reflect what the demo server is currently doing. Bug: None Change-Id: I64eec2279765b63ab1296ab6b441d2f7c669c616 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55525 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
While this assembly implementation is faster in microbenchmarks, the cache pressure makes it slightly worse than the C code in larger benchmarks. Before: Did 7686 HRSS generate operations in 1056025us (7278.2 ops/sec) Did 90000 HRSS encap operations in 1010095us (89100.5 ops/sec) Did 28000 HRSS decap operations in 1031008us (27157.9 ops/sec) After: Did 3523 HRSS generate operations in 1045508us (3369.7 ops/sec) Did 43000 HRSS encap operations in 1017077us (42278.0 ops/sec) Did 17000 HRSS decap operations in 1011170us (16812.2 ops/sec) Change-Id: Ia7745b50393f2d2849867e7c5c0af59d651f243d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55885 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
More than one post-quantum group is now defined so it would be possible for two PQ groups to be 1st and 2nd preferences. In that case, we probably don't want to send two PQ initial key shares. (Only one PQ group is _implemented_ currently, so we can't write a test for this.) Change-Id: I51ff118f224153e09a0c3ee8b142aebb6b340dcb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56226 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
97873cd removed the HRSS assembly, but that file was special-cased in generate_build_files.py and so an update is also needed there. Change-Id: I3c99989da5faee6b39a3b90fee5fa89c20997c64 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56345 Auto-Submit: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
If you pass an empty assembly file into nasm, it crashes. Add a dummy instruction which the static linker will hopefully dropped. (This is a no-op unless you try to link all the assembly files together for a simpler build.) Bug: 542 Change-Id: Idd2b96c129a3a39d5f21e3905762cc34c720f6b2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56326 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
This is the same limit we already implement.
Bring in the new assembly language code but do not start using it yet. The changes to enable it will be done later.
…64.pl and add SEH unwind codes
briansmith
force-pushed
the
b/merge-boringssl-13
branch
from
db87b03 to
78b0af8
Compare
briansmith
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1654 +/- ##
=======================================
Coverage 92.09% 92.09%
=======================================
Files 132 132
Lines 18869 18869
Branches 196 196
=======================================
Hits 17377 17377
Misses 1455 1455
Partials 37 37 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
These additions break people's `cargo deny` jobs. The actual license isn't affected by the change that updated LICENSE; it just added some of Google's internal tracking numbers. Those numbers are not useful to us. The next time we update LICENSE for an important reason we should remove all the tracking numbers.
The code isn't used yet but we should avoid the openssl/ include before we forget it is there.
|
I verified that the merge files exactly match the corresponding ones from BoringSSL commit a43c76d except for |
|
Here is how to read one of these merges:
Those are the actions I took as I reviewed the BoringSSL changes.
|
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.