Upstream merge 2023-10-02 #1221
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
dkostic
force-pushed
the
upstream-merge-2023-10-02
branch
from
a2a33be to
621972f
Compare
samuel40791765
approved these changes
Oct 4, 2023
WillChilds-Klein
force-pushed
the
upstream-merge-2023-10-02
branch
from
621972f to
bd8ab72
Compare
andrewhop
approved these changes
Oct 10, 2023
Many of our point addition functions internally check for the doubling case and branch because the addition formulas are incomplete. This branch is fine because the multiplication formulas are arranged to not hit this case. However, we don't want to leak the couple of intermedate values that determine whether to branch. Previously, we ran into this with https://boringssl-review.googlesource.com/c/boringssl/+/36465. This wasn't sufficient. The compiler understands if (a & b) enough to compile into two branches. Thanks to Moritz Schneider, Nicolas Dutly, Daniele Lain, Ivan Puddu, and Srdjan Capkun for reporting this! Fix the leak by adding a value barrier on the final value. As we're also intentionally leaking the result of otherwise secret data flow, I've used the constant_time_declassify functions, which feed into our valgrind-based constant-time validation and double as barriers. Accordingly, I've also added some CONSTTIME_SECRET markers around the ECDSA nonce value, so we can check with valgrind the fix worked. The marker really should be at a lower level, at ec_random_nonzero_scalar or further (maybe RAND_bytes?), but for now I've just marked the nonce. To then clear valgrind, add constant_time_declassify in a few other places, like trying to convert infinity to affine coordinates. (ECDH deals with secret points, but it is public that the point isn't infinity.) Valgrind now says this code is constant-time, at least up to compilation differences introduced by the annotations. I've also inspected the compiler output. This seems to be fine, though neither test is quite satisfying. Ideally we could add annotations in ways that don't influence compiler output. Change-Id: Idfc413a75d92514717520404a0f5424903cb4453 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60225 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> (cherry picked from commit 55b069de8d3ed53fe578fde5c15499cc4c177af5)
This silences a few false positives in the valgrind-based constant-time validation. First, there are a few precondition checks that are publicly true, but valgrind doesn't know that. I've added a constant_time_declassify_int function and stuck those in there, since the existing macro is mostly suited for macros. It also adds a value barrier in production code (see comment for why). If we more thoroughly decoupled RSA from BIGNUM, we could probably avoid this, since a lot of comes from going through public BIGNUM APIs. Next, our BIGNUM strategy is such that bounds on bignums are sometimes computed pessimally, and then clamped down later. Modular arithmetic is trivially bounded and avoids that, but RSA CRT involves some non-modular computations. As a result, we actually compute a few more words than necessary in the RSA result, and then bn_resize_words down. bn_resize_words also has a precondition check, which checks that all discarded words are zero. They are, but valgrind does not know that. Similarly, the BN_bn2bin_padded call at the end checks for discarded non-zero bytes, but valgrind does not know that, because the output is bounded by n, the discarded bytes are zero. I've added a bn_assert_fits_in_bytes to clear this. It's an assert in debug mode and a declassification in constant-time validation. I suspect a different secret integer design would avoid needing this. I think this comes from a combination of non-modular arithmetic, not having callers pass explicit width, and tracking public widths at the word granularity, rather than byte or bit. (Bit would actually be most ideal.) Maybe worth a ponder sometime. Change-Id: I1bc9443d571d2881e2d857c70be913074deac156 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56825 Commit-Queue: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit aa83c12069f3d62704fce3d499b068b5bf1b6e31)
We check OAEP padding in constant time, but once the padding is determined to be valid (or not), this fact and, if valid, the output length are public. Change-Id: I2aa6a707ca9a91761776746264416736c820977c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56845 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 8f220ece1e855b969f8b764c49212dc655b55f04)
Also add some tests for X25519_public_from_private, as we apparently weren't directly testing it with test vectors. Change-Id: I1b73a9655323d507a8e022c62530ddd4610db4b9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60109 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]> (cherry picked from commit da757e601005704a00e58ac04f9dfeac184f0dd2)
All inputs are marked as secret. This is not to support a use case for calling X25519 with a secret *point* as the input, but rather to ensure that the choice of the point cannot influence whether the scalar is leaked or not. Same for the initial contents of the output buffer. This is a conservative choice and may be revised in the future. Change-Id: I595d454a8e1fdc409912aee751bb0b3cf46f5430 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60186 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit be0fdf7fde7c96c54b463fa3e5033298201fde94)
Andres Erbsen noticed we didn't actually have tests to catch when the format macros were wrong. In doing so, remove BN_DEC_FMT2. It was unused and only makes sense in the context of the bignum-to-decimal conversion, where we no longer use it anyway. None of these macros are exported in OpenSSL at all, so it should be safe to remove it. (We possibly can remove the others too. I see one use outside the library, and that use would probably be better written differently anyway.) Change-Id: I4ffc7f9f7dfb399ac060af3caff0778000010303 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60325 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> (cherry picked from commit e106b536ee6233fec2e8876a16686d10607911c5)
BN_nnmod() can deal with the situation that the first and the second arguments are the same, but it cannot deal with the first and the second argument being equal. In that situation, if BN_mod(x, y, x, ctx) results in a negative x, then the result of BN_nnmod() is zero. This breaks the strange BN_mod_inverse(m, a, m, ctx). Reported by Guido Vranken in openssl/openssl#21110 Change-Id: I8584720660f214f172b3b33716a5e3b29e8f2fd8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60365 Reviewed-by: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit b0341041b03ea71d8371a9692aedae263fc06ee9)
The assembly functions from OpenSSL vary in how the counter overflow works. The aarch64 implementation uses a mix of 32-bit and 64-bit counters. This is because, when packing a block into 64-bit general-purpose registers, it is easier to implement a 64-bit counter than a 32-bit one. Whereas, on 32-bit general-purpose registers, or when using vector registers with 32-bit lanes, it is easier to implement a 32-bit counter. Counters will never overflow with the AEAD, which sets the length limit so it never happens. (Failing to do so will reuse a key/nonce/counter triple.) RFC 8439 is silent on what happens on overflow, so at best one can say it is implicitly undefined behavior. This came about because pyca/cryptography reportedly exposed a ChaCha20 API which encouraged callers to randomize the starting counter. Wrapping with a randomized starting counter isn't inherently wrong, though it is pointless and goes against how the spec recommends using the initial counter value. Nonetheless, we would prefer our functions behave consistently across platforms, rather than silently give ill-defined output given some inputs. So, normalize the behavior to the wrapping version in CRYPTO_chacha_20 by dividing up into multiple ChaCha20_ctr32 calls as needed. Fixed: 614 Change-Id: I191461f25753b9f6b59064c6c08cd4299085e172 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60387 Commit-Queue: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> (cherry picked from commit df9955b62d71d896198364465b5f5871c69ba93e)
dkostic
force-pushed
the
upstream-merge-2023-10-02
branch
from
bd8ab72 to
a1aadda
Compare
Merged
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.
Issues:
N/A
Description of changes:
Upstream merge 2023-10-02
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.