Merge main into integrate-pq branch #599
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
Liberalized input-output aliasing, more curve25519 field operations s2n-bignum original commit: awslabs/s2n-bignum@9c5ab11
No labels on the same line as an instruction, and all instructions with initial 8 spaces. Also tweak a couple of instances in the testing code where "bignum_copy" was used inside a reference function. s2n-bignum original commit: awslabs/s2n-bignum@6d2bf13
The getters would leave the length uninitialized when empty, which is
dangerous if the caller does not check. Instead, always fill it in.
This opens a can of worms around whether empty alias and missing alias
are meaningfully different. Treating {NULL, 0} differently from
{non-NULL, 0} has typically caused problems. At the PKCS#12 level,
neither friendlyName, nor localKeyId are allowed to be empty, which
suggests we should not distinguish. However, X509_CERT_AUX, which is
serialized in i2d_X509_AUX, does distinguish the two states. The getters
try to, but an empty ASN1_STRING can have NULL data pointer. (Although,
when parsed, they usually do not because OpenSSL helpfully
NUL-terminates it for you.)
For now, I've just written the documentation to suggest the empty string
is the same as the missing state. I'm thinking we can make the PKCS#12
functions not bother distinguishing the two and see how it goes. I've
also gone ahead and grouped them with d2i_X509_AUX, although the rest of
the headers has not yet been grouped into sections.
Bug: 426, 481
Change-Id: Ic9c21bc2b5ef3b012c2f812b0474f04d5232db06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51745
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
This type is opaque, with no accessors or setters, and there is no way to get a hold of one except by parsing it. It's only used indirectly via X509 functions. The 'other' field is unused and appears to be impossible to set or query, in either us or upstream. Change-Id: I4aca665872792f75e9d92e5af68da597b849d4b6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51746 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
VS2015 has finally dropped off our support window. As part of dropping it from the bots, I'm thinking of using the current vs2017 builders to test vs2019. In preparation for that, add a vs2019 hash to vs_toolchain.py. Change-Id: I4c3dde2825f57c39a8da0e155e96d08550d39893 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52005 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Change-Id: I934376ead1bc3e4e8349540c4a3da99cd0b49181 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51985 Reviewed-by: David Benjamin <[email protected]>
We need a function that returns a version that links to a certificate. Previously we have used the git hash as the version of our modules but the source cannot contain its own hash. Thus this change defines a new format for FIPS module versions which will be filled in once we're ready to define a version. Change-Id: Ie4641945119106bc47e8da94ed8a45a86abb6f92 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51986 Reviewed-by: David Benjamin <[email protected]>
Change-Id: If45a98427516c5a26f2048adb8f8d0415417dcf8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51987 Reviewed-by: David Benjamin <[email protected]>
Our FIPS module only claims support for RSA signing/verification, and |RSA_generate_key_fips| already performs a sign/verify pair-wise consistency test (PCT). For ECDSA, |EC_KEY_generate_fips| performs a sign/verify PCT too. But when |EC_KEY_generate_fips| is used for key agreement a sign/verify PCT may not be correct. The FIPS IG[1], page 60, says: > Though not a CAST, a pairwise consistency test (PCT) shall be > conducted for every generated public and private key pair for the > applicable approved algorithm (per ISO/IEC 19790:2012 Section > 7.10.3.3). To further clarify, at minimum, the PCT that is required by > the underlying algorithm standard (e.g. SP 800- 56Arev3 or SP > 800-56Brev2) shall be performed. SP 800-56Ar3, page 36, says: > For an ECC key pair (d, Q): Use the private key, d, along with the > generator G and other domain parameters associated with the key pair, > to compute dG (according to the rules of elliptic-curve arithmetic). > Compare the result to the public key, Q. If dG is not equal to Q, then > the pair-wise consistency test fails But |EC_KEY_generate_fips| has always done that via |EC_KEY_check_key|. So I believe that |EC_KEY_generate_fips| works for either case. This change documents that. [1] FIPS 140-3 IG dated 2022-03-14 and with SHA-256 2f232f7f5839e3263284d71c35771c9fdf2e505b02813be999377030c56b37e4 Change-Id: I4b4e2ed92ae3d59e2f2404c41694abeb3eb283f4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51988 Reviewed-by: David Benjamin <[email protected]>
Change-Id: Ifdb2fe5952930c33dfa9ea5bbdb9d1ce699952a4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52027 Reviewed-by: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
It appears to be unused. It has global effects and is not thread-safe. Rather than try to make the double-function-pointer declaration readable, remove it. Change-Id: If58ecd0c9367bbb27cf8c5e27ac9997fe4c1225d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51965 Reviewed-by: Alex Gaynor <[email protected]> Reviewed-by: Adam Langley <[email protected]>
VS 2017 was released in March 2017, five years ago now. This means VS 2015 is now past our support window. This will make the unmarked and "vs2017" configs in CI/CQ do the same thing. I'll follow up with a separate CL in infra/config to switch the test VS 2019 instead. Update-Note: BoringSSL may no longer build with VS 2015. Consumers should upgrade to the latest Visual Studio release. VS 2017 or later is required. Change-Id: I477759deb95a27efe132de76d9ed103826110df0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52085 Reviewed-by: Bob Beck <[email protected]> Reviewed-by: Adam Langley <[email protected]>
The files no longer need to be patched because fiat-crypto now has its own copy of our value barrier. It does, however, require syncing our NO_ASM define with fiat's. fiat-crypto is now licensed under any of MIT, BSD 1-clause, or Apache 2. I've stuck with the MIT one as that's what we were previously importing. No measurable perf difference before/after this CL, with GCC or Clang on x86_64. Change-Id: I2939fd517de37aabdea3ead49150135200a1b112 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52045 Reviewed-by: Adam Langley <[email protected]>
Should not run jemalloc with ASAN simultaneously. Change-Id: I2ea3107178c11fe34978bb093737564e1222c0d5 Signed-off-by: niewei <[email protected]> Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51945 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This is another instance of https://boringssl-review.googlesource.com/c/boringssl/+/38584. We missed it because our UBSan bots only run on x86-64, which uses a different version of this function. See also https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=96307 Change-Id: Ib27eaca581c27fe9b7fd0e532d1a0e2850cb83d4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52125 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
With the UCRT, introduced in VS 2015, vsnprintf in MSVC is now C99-conformant. See: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/vsnprintf-vsnprintf-vsnprintf-l-vsnwprintf-vsnwprintf-l?view=msvc-170 It is a little unclear to me whether "Beginning with the UCRT in Visual Studio 2015 and Windows 10" means it is only C99-conformant in Windows 10, or if this is referring to how the UCRT starts becoming an OS component in Windows 10. I think the latter. This document talks about the UCRT: https://docs.microsoft.com/en-us/cpp/porting/upgrade-your-code-to-the-universal-crt?view=msvc-170 But we have tests which cover this in BIOTest.Printf. If it's not C99-compliant in Windows 7, we'll notice in Chromium's CI. Change-Id: I932ec2633f94bd77dbe797b06a6bfbc95a568335 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52086 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Bug: 481 Change-Id: I5c1dd6e39874cd1e88cb6fd0b3a6c6c6fac0db85 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51665 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This is causing -Werror failures with recent Clang builds. Change-Id: I0f82d99ae51d02037f5f614a8fadfb3efc0da2da Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52145 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
The x86 ABI on Windows differs from the "standard" one used on Linux,
Mac OS etc. For our purposes the relevant differences are that RDI and
RSI are callee-saved (need to be preserved if modified), and that the
input arguments are in different places. Integer return values are
still in RAX. Here are the conventions for the first 6 integer
arguments (the maximum that s2n-bignum functions rely on so far):
Argument number Standard Windows
1 RDI RCX
2 RSI RDX
3 RDX R8
4 RCX R9
5 R8 [RSP+40]
6 R9 [RSP+48]
Here we add a wrapper round each s2n-bignum function that optionally
(controlled by the WINDOWS_ABI variable in preprocessing) makes the
function compatible with the Windows ABI. This is done in a simplistic
way simply by pushing the extra callee-saved registers (RSI and RDI),
shuffling arguments (only the ones actually used by that function),
performing the same computation in exactly the original way, and
finally popping back the extra registers.
The Makefile currently selects this WINDOWS_ABI mode when `uname -s`
gives "CYGWIN_NT-10.0", as on the main test platform, but this can be
configured.
s2n-bignum original commit: awslabs/s2n-bignum@8ac4010
There are paperwork reasons why it's useful to use the same hash function in all cases. Thus unify on SHA-256 because contexts where SHA-512 is faster, are faster overall and thus less sensitive. Change-Id: I7a782a3adba4ace3257313a24dc8bc213b9d64ec Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52165 Reviewed-by: David Benjamin <[email protected]>
Polynomials have more elements than strictly needed in order to better align them for when vector instructions are available. The |poly_mul_vec| path is sensitive to this and so zeros out the extra elements before starting work. That means that it'll write to a const pointer, even if it'll always write the same value. However, while this code is intended for ephemeral uses, we have a case where this is a data race that upsets tools. Therefore this change always normalises the polynomials, even if it's running in a configuration that doesn't care. Change-Id: I83eb04c5ce7c4e7ca959f2dd7fbd3efbe306d989 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52186 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
The ARMv8 assembly code in this commit is mostly taken from OpenSSL's `ecp_nistz256-armv8.pl` at https://github.com/openssl/openssl/blob/19e277dd19f2897f6a7b7eb236abe46655e575bf/crypto/ec/asm/ecp_nistz256-armv8.pl (see Note 1), adapting it to the implementation in p256-x86_64.c. Most of the assembly functions found in `crypto/fipsmodule/ec/asm/p256-x86_64-asm.pl` required to support that code have their analogous functions in the imported OpenSSL ARMv8 Perl assembly implementation with the exception of the functions: - ecp_nistz256_select_w5 - ecp_nistz256_select_w7 An implementation for these functions was added. Summary of modifications to the imported code: * Renamed to `p256-armv8-asm.pl` * Modified the location of `arm-xlate.pl` and `arm_arch.h` * Replaced the `scatter-gather subroutines` with `select subroutines`. The `select subroutines` are implemented for ARMv8 similarly to their x86_64 counterparts, `ecp_nistz256_select_w5` and `ecp_nistz256_select_w7`. * `ecp_nistz256_add` is removed because it was conflicting during the static build with the function of the same name in p256-nistz.c. The latter calls another assembly function, `ecp_nistz256_point_add`. * `__ecp_nistz256_add` renamed to `__ecp_nistz256_add_to` to avoid the conflict with the function `ecp_nistz256_add` during the static build. * l. 924 `add sp,sp,aws#256` the calculation of the constant, 32*(12-4), is not left for the assembler to perform. Other modifications: * `beeu_mod_inverse_vartime()` was implemented for AArch64 in `p256_beeu-armv8-asm.pl` similarly to its implementation in `p256_beeu-x86_64-asm.pl`. * The files containing `p256-x86_64` in their name were renamed to, `p256-nistz` since the functions and tests defined in them are hereby running on ARMv8 as well, if enabled. * Updated `delocate.go` and `delocate.peg` to handle the offset calculation in the assembly instructions. * Regenerated `delocate.peg.go`. Notes: 1- The last commit in the history of the file is in master only, the previous commits are in OpenSSL 3.0.1 2- This change focuses on AArch64 (64-bit architecture of ARMv8). It does not support ARMv4 or ARMv7. Testing the performance on Armv8 platform using -DCMAKE_BUILD_TYPE=Release: Before: ``` Did 2596 ECDH P-256 operations in 1093956us (2373.0 ops/sec) Did 6996 ECDSA P-256 signing operations in 1044630us (6697.1 ops/sec) Did 2970 ECDSA P-256 verify operations in 1084848us (2737.7 ops/sec) ``` After: ``` Did 6699 ECDH P-256 operations in 1091684us (6136.4 ops/sec) Did 20000 ECDSA P-256 signing operations in 1012944us (19744.4 ops/sec) Did 7051 ECDSA P-256 verify operations in 1060000us (6651.9 ops/sec) ``` Change-Id: I9fdef12db365967a9264b5b32c07967b55ea48bd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51805 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
…l-merge-f7e1a94bd94980c58f61f300cd80275471b210e7
The query function was added in google/boringssl@7f4057e to tell if an algorithm is FIPS approved. The version function was added in google/boringssl@c6e8f3e. They are commented out until they are validated since AWS-LC contains its own service indicator and version functions.
This reverts commit 2144076. AWS-LC continues to support VS 2015.
…c58f61f300cd80275471b210e7 Boringssl merge f7e1a94 This change merges in commits from BoringSSL between google/boringssl@d0f14f3 (Mar 18, 2022) and google/boringssl@f7e1a94 (Apr 8, 2022): * The integrity test now only uses SHA-256 which was the case already for AWS-LC. The option was removed from `inject_hash.go` and the conflicts were resolved such as to remove any residuals of it. * Fiat crypto code for P-256 and Curve25519 was updated in upstream. We kept the fixes we made for `int128_t` to accommodate older compilers. * The newly-added FIPS version and function-status query functions were commented out in an additional commit: aws@09bb41a (see commit message).
This creates an install directory under the top level source directory. The install contains a CMake config file that produces variables and targets compatible with FindOpenSSL, or the directory can be scanned by FindOpenSSL via -DOPEN_SSL_ROOT. This allows using BoringSSL with third-party dependencies that find an SSL implementation via CMake. Change-Id: Iffeac64b9cced027d549486c98a6cd9721415454 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52205 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
[UNIVERSAL 0] is reserved by X.680 for the encoding to use. BER uses this to encode indefinite-length EOCs, but it is possible to encode it in a definite-length element or in a non-EOC form (non-zero length, or constructed). Whether we accept such encodings is normally moot: parsers will reject the tag as unsuitable for the type. However, the ANY type matches all tags. Previously, we would allow this, but crypto/asn1 has some ad-hoc checks for unexpected EOCs, in some contexts, but not others. Generalize this check to simply rejecting [UNIVERSAL 0] in all forms. This avoids a weird hole in the abstraction where tags are sometimes representable in BER and sometimes not. It also means we'll preserve this check when migrating parsers from crypto/asn1. Update-Note: There are two kinds of impacts I might expect from this change. The first is BER parsers might be relying on the CBS DER/BER element parser to pick up EOCs, as our ber.c does. This should be caught by the most basic unit test and can be fixed by detecting EOCs externally. The second is code might be trying to parse "actual" elements with tag [UNIVERSAL 0]. No actual types use this tag, so any non-ANY field is already rejecting such inputs. However, it is possible some input has this tag in a field with type ANY. This CL will cause us to reject that input. Note, however, that crypto/asn1 already rejects unexpected EOCs inside sequences, so many cases were already rejected anyway. Such inputs are also invalid as the ANY should match some actual, unknown ASN.1 type, and that type cannot use the reserved tag. Fixed: 455 Change-Id: If42cacc01840439059baa0e67179d0f198234fc4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52245 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
I don't think these are all UB by C's rules, but it's easier not to think about the pointers. Still more to go, but these were some easy ones. Bug: 301 Change-Id: Icdcb7fb40f85983cbf566786c5f7dbfd7bb06571 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52905 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]> (cherry picked from commit 1e469e45a46ff580899cbef939babe02ad916c85)
Change-Id: Iddc34918dcd3e4a6e80c79e5b8efa11e846c73d1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52885 Reviewed-by: David Benjamin <[email protected]> (cherry picked from commit 572c416b257991ad1433b907bf827824509e9f3e)
Not that we get much type-checking from this, as an assembly function. Change-Id: I21643444cfc577e2d68f11891e602724ded52e7f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52831 Reviewed-by: Adam Langley <[email protected]> (cherry picked from commit efd09b7e370e6975f795cdbc54ff6e0941ebf274)
bind uses this function. Change-Id: I97ba86d9f75597bff125ae0b56952effc397e6b8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53010 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]> (cherry picked from commit 0ebd69bd1e0ae834e01935ad0c5cfac63a5aea32)
This is required to run SHA3 tests otherwise we get an error of unknown algorithm. Change-Id: I085da2b6757ba1f452f33abc7f1bafc4a404e025 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52926 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: Corie Pressley <[email protected]> (cherry picked from commit 7c2323820e0d456fa64b4c6d7a3b0b52eebf566c)
Adds `crypto/obj/objects.go` and `util/make_errors.go` checks to our ci.
Add SHAKE128 and SHAKE256 internal APIs and test vector units `SHAKE128Test.NISTTestVectors` and `SHAKE256Test.NISTTestVectors` based on NIST Test Vectors. Co-authored-by: Anastasova <[email protected]> Co-authored-by: dkostic <[email protected]> Co-authored-by: torben-hansen <[email protected]> Co-authored-by: Andrew Hopkins <[email protected]>
Add ARMv8 assembly optimized implementations for the SHA3/SHAKE underlying subroutines based on the general instruction set architecture (ISA). Included are also implementations that utilize the SHA3 vector extension set (4 vector instructions), but they are not currently enabled. The implementations are imported from OpenSSL: https://github.com/openssl/openssl/blob/479b9adb88b9050186c1e9fc94879906f378b14b/crypto/sha/asm/keccak1600-armv8.pl Co-authored-by: Anastasova <[email protected]> Co-authored-by: dkostic <[email protected]> Co-authored-by: torben-hansen <[email protected]> Co-authored-by: Ubuntu <[email protected]>
Upstream merge 2022-08-09
* Fix aws_lc_sys::ERR_xxx; support optimized build * Remove 'openssl/cast.h'
* moved AES-XTS into FIPS boundary * added AES-XTS to service indicator * added AES-XTS test vectors from NIST
Currently we have a list of allowed architectures to prevent people from trying to build on Big Endian systems since we don't support them. It's simpler to instead explicitly disallow Big Endian systems and have unknown architectures compile as generic builds.
|
Nit: why not rebase the pq branch? |
just didn't think of it :) I can do that, have to fix another thing anyway (generate some files). |
actually, I just tried to rebase but in that case I can't open a "clean" PR to the integrate-pq branch (I first have to resolve all the conflicts locally, and then again when I try to merge into the branch). So if you don't mind, I'd stick with merging main into the pq branch. |
bryce-shang
previously approved these changes
Aug 23, 2022
The NID and other identifiers of an algorithm in AWS-LC are generated by a script rather than adding the NID by hand. This commit fixes the Kyber512 NID previously added by hand.
dkostic
force-pushed
the
merge-main-into-pq
branch
from
d1aff68 to
14247c8
Compare
The prefix build doesn't work with the Kyber source code because Kyber is doing it's own prefixing that clashes with the one we do in aws-lc. This will be fixed before moving Kyber to the main aws-lc branch.
torben-hansen
approved these changes
Aug 25, 2022
bryce-shang
approved these changes
Aug 25, 2022
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:
Merging the tip of main into the integrate-pq branch.
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.