Upstream merge 2023 04 14 #958
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
skmcgrail
force-pushed
the
upstream-merge-2023-04-14
branch
2 times, most recently
from
683005c to
5e7c322
Compare
skmcgrail
force-pushed
the
upstream-merge-2023-04-14
branch
from
5e7c322 to
7f19173
Compare
skmcgrail
force-pushed
the
upstream-merge-2023-04-14
branch
3 times, most recently
from
5cf6acb to
44b59c4
Compare
nebeid
previously approved these changes
Apr 21, 2023
justsmth
previously approved these changes
Apr 24, 2023
Ran go get -u all, followed by go mod tidy. Some tools are flagging CVE-2021-43565 and CVE-2022-27191 in some of the Go packages. Our uses of x/crypto are x/net are not impacted by either bug, but update anyway to silence the tools. AWS-LC: Took these dependency changes but also ran `go mod tidy` which picked up missing dependencies and missing go.sum entries. Change-Id: Ia0e2757625b58d964aedd4217f21b72f293b910b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57485 Commit-Queue: Bob Beck <bbe@google.com> Reviewed-by: Bob Beck <bbe@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit bade46179ea3d729a434b92c2577be18d8a1cc4b)
Go has a bzip2 decoder but not an encoder (because I only needed a decoder when I wrote the package). Thus when updated ACVP tests are written they aren't compressed. This change removes the `.bz2` suffix from paths when writing updated tests, which makes it easier to compress them. Change-Id: I9de6a1845cdf1cddca2e5d253b97262b023393f6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57466 Auto-Submit: Adam Langley <agl@google.com> Reviewed-by: David Benjamin <davidben@google.com> (cherry picked from commit f39826d9f5248ee66e9a32ac0513d81349c969af)
After https://chromium-review.googlesource.com/c/chromium/tools/build/+/4277422, the old ANDROID_NATIVE_API_LEVEL values are no longer in CMakeCache.txt. Change-Id: If0c724081ef99bf3bc2e714fe84b6d16925bd116 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57507 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit 92272956a6335e61f99b76b26b9bd677fb9c9706)
OpenSSL's APIs are full of empty objects that aren't captured in the type system. A DSA object may represent any of nothing, a group, a public key, or a private key. Since the type system doesn't capture this, better to check which type we've got and NULL-check the fields we need for the operation. Change-Id: I32b09ebaad58fcb2a0bfc9ad56d381f95099bf7b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57225 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 7c860a4f8e0ddd33bed326dfa7903c9c2ba12433)
We already reject values that are out of bounds. Also we were using the wrong error, so fix that. Zero should additionally be rejected, otherwise, when signing an all-zero digest (impossible unless your system signs untrusted, pre-hashed inputs), ECDSA can infinite loop. Thanks to Guido Vranken who reported an analogous issue with DSA in openssl/openssl#20268 When EC_KEYs are obtained through the parser, this CL is a no-op. The corresponding public key is the point at infinity, which we'll reject at both parse time and in EC_KEY_check_key. But as EC_KEY runs into OpenSSL's usual API design flaw (mutable, field-by-field setters over constructor functions for immutable objects), we should reject this in EC_KEY_set_private_key too. Update-Note: Systems that manually construct an EC_KEY (i.e. not from parsing), and either omit the public key or don't call EC_KEY_check_key will start rejecting the zero private key. If such a system *also* signs untrusted digests, this fixes an infinite loop in ECDSA. Change-Id: I3cc9cd2cc59eb6d16826beab3db71d66b23e83ff Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57226 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> (cherry picked from commit aa8d3b5a53ecff9befb7b0edcbbe56bb94dd3ae0)
DSA private keys cannot be zero. If they are, trying to sign an all zeros digest loops forever. Thanks to Guido Vranken who reported this in openssl/openssl#20268 Along the way, because OpenSSL's bad API design made constructing DSA objects such a mess, just move all the consistency checks to dsa_check_parameters (now dsa_check_key) so it is consistently checked everywhere. Ideally we'd get a better handle on DSA state, like we hope to do for RSA state (though not there yet), so checks only happen once. But we consider DSA deprecated so it's not worth putting much effort into it. Update-Note: Some invalid DSA keys will be rejected by the parser and at use. Nothing should be using DSA anymore. Change-Id: I25d3faf145a85389c47abdd9db8e9b0056b37d8a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57227 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> (cherry picked from commit 788bf74188fd091b7e67f1ff4a5258bec653b1ea)
When the parameters are incorrect, all assumptions of (EC)DSA fly out the window, including whether the retry loop actually terminates. While ECDSA is broadly used with fixed, named groups, DSA was catastrophically mis-specified with arbitrary parameters being the default and only mode. Cap the number of retries in DSA_do_sign so invalid DSA groups cannot infinite loop, e.g. if the "generator" is really nilpotent. This also caps the iteration count for ECDSA. We do, sadly, support arbitrary curves via EC_GROUP_new_curve_GFp, to help Conscrypt remain compatible with a badly-designed Java API. After https://boringssl-review.googlesource.com/c/boringssl/+/51925, we documented that untrusted parameters are not supported and may produce garbage outputs, but we did not document that infinite loops are possible. I don't have an example where an invalid curve breaks ECDSA, but as it breaks all preconditions, I cannot be confident it doesn't exist, so just cap the iterations. Thanks to Hanno Böck who originally reported an infinite loop on invalid DSA groups. While that variation did not affect BoringSSL, it inspired us to find other invalid groups which did. Thanks also to Guido Vranken who found, in openssl/openssl#20268, an infinite loop when the private key is zero. That was fixed in the preceding CL, as it impacts valid groups too, but the infinite loop is ultimately in the same place, so this change also would have mitigated the loop. Update-Note: If signing starts failing with ECDSA_R_INVALID_ITERATIONS, something went horribly wrong because it should not be possible with real curves. (Needing even one retry has probability 2^-256 or so.) Change-Id: If8fb0157055d3d8cb180fe4f27ea7eb349ec2738 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57228 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> (cherry picked from commit 474ddf8ba95f30e69acea37d76b3e671d89381c3)
Some Android devices in our builder pool are so old that they lack getrandom. This change attempts to make them happy. Change-Id: I5eea04f1b1dc599852e3b8448ad829bea05b9fe9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57527 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> Auto-Submit: Adam Langley <agl@google.com> (cherry picked from commit 3cd7faadc982ca6c33c05a0ba8030f43bde80707)
Various constants and strings identifying the authors are currently misplaced in .text. This change allows using execute-only .text on platforms that enforce it by default, such as OpenBSD. Modify x86_64-xlate.pl to replace .rodata with __DATA,__const for macs. Adapt the nasm/masm path to emit an .rdata segment with alignment of 8. This last change is not strictly needed but makes things explicit. AWS-LC: - Moved data to .rodata in aesni-gcm-avx512.pl. Change-Id: If716b892c1faabd85c6c70bdd75e145304841f83 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57445 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit ebd43ef8dd3c062dccf6adf264c5332efda1f4b3)
Change-Id: I736c7dc17efcaa5c3d8bd5fdee36d2dcb86ae627 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57567 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> (cherry picked from commit bffae8a10e7c19930276df2609806be2b92ed1db)
We used to have a tower of fallbacks to support older Androids that were missing getauxval. The comments say getauxval is available in Android API level 20 or higher, but this wasn't right. It's actually API level 18 or higher per the NDK headers and https://developer.android.com/ndk/guides/cpu-features Android API level 18 is Android 4.3, or Jelly Bean MR2. Recent versions of the NDK (starting r24, March 2022) don't even support Jelly Bean, i.e. the minimum API level is 19, and the usage statistics in the latest Android Studio stop at KitKat. As far as I know, nothing needs us to support API levels 17 and below anymore. Update-Note: BoringSSL now requires API level 18 or later. Projects needing to support API level of 17 or below will fail to build due to the use of getauxval. If any such projects exist, please contact BoringSSL maintainers. Change-Id: Iedc4836ffd701428ab6d11253d4ebd5a9121e667 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57506 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 6ab4f0ae7f2db96d240eb61a5a8b4724e5a09b2f)
We call Accept as a TLS 1.2 client and a TLS 1.3 server. In the latter, we create an SSLKeyShare object, Accept, and immediately destroy it. In the former, we create the SSLKeyShare object a couple steps before actually using it. It's equivalent to create the object just before Accept, so switch to that. This change means that hs->key_shares now only ever contains objects in between Offer and Finish. Or, in KEM terms, it only ever contains KEM private keys. (SSLKeyShare objects are currently a little confused about what kind of state they contain.) Change-Id: Idec62ac298785f784485bc9065f7647034d2a607 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57605 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> (cherry picked from commit 7fa0910a6511895119a4aa3f474920594e734564)
We've got two layers of serialization. There's the lower-level SerializePrivateKey/DeserializePrivateKey functions that just encode a private key assuming you already know the group, and then there's Serialize/Create which output an INTEGER and OCTET STRING pair. The latter is only used by handoff.cc, so move them there. This trims the SSLKeyShare abstraction slightly. Change-Id: I1c901d7c16b082bfe1b6acd0a1711575e7f95c05 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57625 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit a5dcf35caf6857f08be29586fd41ce8349d9e857)
Since this was written, we've tidied up the EC code a bit: 1. While not quite yet infallible (but we should get there), the output of EC_GROUP_new_by_curve_name no longer needs to be freed. 2. BN_CTX no longer does anything in EC code, so just pass in NULL. We really should build a real ECDH API, but for now just improve our use of the current thing. Change-Id: I44f5429afec06c28372ae70148eb8de263d716f3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57626 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 9cbff81cec055e34d3f6e267d5509d4aad6bed41)
skmcgrail
force-pushed
the
upstream-merge-2023-04-14
branch
from
44b59c4 to
9d055b5
Compare
nebeid
approved these changes
Apr 24, 2023
justsmth
approved these changes
Apr 24, 2023
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.
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.