Skip to content

Temporary RSA_METHOD PR #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 46 commits into
base: eckeymethod
Choose a base branch
from
Open

Temporary RSA_METHOD PR #6

wants to merge 46 commits into from

Conversation

smittals2
Copy link
Owner

The rsaMethod PR is rebased on top of eckeymethod. This means it would be easier to see the new changes once eckeymethod is merged into aws-lc:main. Therefore this PR exists on my local fork to gather feedback preemptively while eckeymethod is merged in.

davidben and others added 30 commits August 27, 2024 15:43
This is a reland of
https://boringssl-review.googlesource.com/c/boringssl/+/64749, which was
reverted in
https://boringssl-review.googlesource.com/c/boringssl/+/65328 due to
issues in Arm mode (i.e. not Thumb mode) builds that target Armv6+
instead of Armv7+.

The issue was that sha256_block_data_order_nohw has slightly different
sizes depending on __ARM_ARCH. Prior to moving the dispatch, the sizes
worked out such that they were always encodable in ADR. After moving the
dispatch, the instructions got shorter, such that the Armv7+ build still
worked, but the Armv6+ build needed to encode an offset of 0x1060
(previously 0x1080), which does not fit.

See https://alisdair.mcdiarmid.org/arm-immediate-value-encoding/ for
details on Arm's very fussy immediate value encoding. It's not the only
form used for ADR (Thumb2 works very differently), but it's the
applicable one here.

While we could shuffle things around, this is all far too fragile. Just
use the LDR; ADD pattern we used for the other function. ADRL would
avoid a load (it splits the offset into two constants without a constant
bank), but that's a pseudo-instruction that's only supported by gas.
clang-assembler didn't want to implement it. Android have a macro at
https://android.googlesource.com/platform/ndk/+/refs/heads/master/docs/ClangMigration.md#arm,
but it didn't work for me when I tried it. Also, searching around, it
sounds like ADRL in gas only works in Arm mode and not Thumb mode?

We could probably work through all that, but the compiler emits constant
banks on 32-bit Arm all the time. (I got this pattern from Clang's
output.) This is probably not worth the trouble.

Bug: 673
Change-Id: I165544764a931b293aa66fb3fc9bb8f01eeb8092
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65808
Commit-Queue: Bob Beck <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
(cherry picked from commit 12316ab445eef5317391a94bef733fa6ff175173)
Some 'return' lines snuck in which cause us to skip some tests.

Change-Id: I2806d6fb4fe3a6bd1fa58932c213d6af8991352e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65827
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 97dc33638b40a428ce666d61d71ee3db08083bf3)
Replace |bn_sqr8x_mont|'s unused |bp| parameter with a flag that
indicates whether MULX and ADX are enabled.

Bug: 673
Change-Id: I56632ad51bdc2f7f5ddd4278419d67e467b84d8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65587
Reviewed-by: Bob Beck <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
This clears the last reference to OPENSSL_armcap_P from assembly!

Bug: 673
Change-Id: Id5d6115535742b2e980ed262d920ae28941841e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65868
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>

(cherry picked from commit 01ea563b92e2b50cfaff23ed9c99d7603c976f3e)
Update-Note: Building for 32-bit x86 may require fixing your builds to
pass -msse2 to the compiler. This will also speed up the rest of the
code in your project. If your project needs to support the Pentium III,
please contact BoringSSL maintainers.

As far as I know, all our supported 32-bit x86 consumers require SSE2.
I think, in the past, I've asserted that our assembly skips SSE2
capability detection. Looking at it again, I don't think that's true.
OPENSSL_IA32_SSE2 means to enable runtime detection of SSE2, not
compile-time.

Additionally, I don't believe we have *ever* tested the non-SSE2
assembly codepaths. Also, now that we want to take the OPENSSL_ia32cap_P
accesses out of assembly, those runtime checks are problematic, as we'd
need to bifurcafe functions all the way down to bn_mul_words.

Unfortunately, the situation with compilers is... complicated. Ideally,
everyone would build with the equivalent of -msse2. 32-bit x86 is so
register-poor that running without SSE2 statically available seems
especially foolish. However, per
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9868, while
Clang defaults to enabling SSE2, GCC does not.

We once broke gRPC's build, in
grpc/grpc#17540, by inadvertently assuming
SSE2. In that discussion, gRPC maintainers were okay requiring Pentium 4
as the minimum CPU, but it's unclear if they actually changed their
build. That discussion also said GCC 8 assumes SSE2, but I'm not able to
reproduce this.

LLVM does indeed interpret "i686" as implying SSE2:
llvm/llvm-project#61347
rust-lang/rust#82435

However, Debian LLVM does *not*. Debian carries a patch to turn this
off!
https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/disable-sse2-old-x86.diff?ref_type=heads

Meanwhile, Fedora fixed their baseline back in 2018.
https://fedoraproject.org/wiki/Changes/Update_i686_architectural_baseline_to_include_SSE2

So let's start by detecting builds that forgot to pass -msse2 and see if
we can get them fixed. If this sticks, I'll follow up by unwinding all
the SSE2 branches.

Bug: 673
Change-Id: I851184b358aaae2926c3e3fe618f3155e71c2f71
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65875
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 56d3ad9d23bc130aa9404bfdd1957fe81b3ba498)
After all, we have to keep this robust, modern cipher conforming to C
well-definedness expectations!

These functions should have simply taken uint8_t* pointers. Make
internal _ex variants that fix this. I've not bothered updating the
public APIs because it will cause a ton of downstream churn and make
those APIs even more OpenSSL-incompatible.  (OpenSSL's APIs take a
const-incorrect uint8_t (*in)[8]. Both our struct and their pointers
expect callers to call with &foo.) This does not seem worth the trouble.

Also since the underlying functions now access as uint8_t*, I suspect
this broadly fixes strict aliasing issues with callers that cast from a
byte array. (Though perhaps in->bytes should be (const uint8_t*)in?)

Ideally c2l and l2c would be replaced with CRYPTO_load_u32_le and
CRYPTO_store_u32_le. (It's a little rude for a header to squat those
names, especially when those name often vary in endianness.) I did that
in a couple places where we'd otherwise increment a pointer declared
with the funny array parameter syntax.  Otherwise I left it alone for
now.

Fixed: 683
Change-Id: I7b0d8b2a16697095ebf42a71482c4ba805a193e4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65690
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>

(cherry picked from commit 608becc67282174594fdaf0ec9c96daca9710d2f)
from libressl

Change-Id: Iaec558fb6d3c698deb000c45b34aa94911e60a06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65967
Auto-Submit: Bob Beck <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
See discussion in
https://boringssl-review.googlesource.com/c/boringssl/+/65967/comment/4b0fb2a6_78bfab3a/

struct tm is defined with tm_ values as all ints.

Conversion inside this code is all bounds checked around valid
times and can't overflow, but because struct tm uses 1 based months
and 1900 based years, we need to modify the input values when
converting a struct tm.

Rather than do awkward bounds checks, just accept an int64_t on
input and we don't have to care.

OPENSSL_gmtime_adj gains checks to ensure the cumulative days
and seconds values passed in may not overflow.

While we are here we also ensure that OPENSSL_gmtime_adj does
not modify the output struct tm in the failure case.

Also while we are here, just make the offset_seconds value of
OPENSSL_gmtime_adj an int64_t - because long was never the
correct type.

Add tests for all this.

Change-Id: I40ac019c4274b5388c97736cf85cede951d8b7ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66047
Commit-Queue: Bob Beck <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: Bob Beck <[email protected]>

(cherry picked from commit 34b51faf3a58fe36e3ab1db99a2a441d0f69c754)
RSAES-PKCS1-v1_5 is broken and should be described as such.

Change-Id: I11f74fbfcef7b44579a333798240147f67cf896c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66107
Commit-Queue: Adam Langley <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 8ff5add548e89f3680da398a41ecfca95a863fcd)
Ok so this one is purely for keeping this the same between forks.
Boring today doesn't need this (although some of our outside
uses might be able to use it). Effectively, this function
is the same as converting to a posix time, followed by a safe
check to see if you can put the result in a time_t.

posix_time.h is about to get added as public in LibreSSL, and
while not strictly necessary there because they could inline
what it does, Libre is finding that may be needed in rpki-client,
ocsp-client, as well as libtls (including for libtls portable).

  In the interests of keeping the same API in the same file, Libre
would like this to be here so it's just consistent from both places.

Change-Id: I2f234005e83cdea5e61fa41fbf03ef61516c45f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66127
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
(cherry picked from commit 5dd15f3a3b26995aeced71da4b404438df2a6363)
Re-ran clang-format on those regions, now that clang-format knows about
STACK_OF(T).

Change-Id: Ied350343b8aaef0dc25dbe3f215c1fae25af90c4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66147
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
Reviewed-by: Adam Langley <[email protected]>

(cherry picked from commit 10605c0d1e4b408dbd8fcfcae3897581afefb730)
This matches upstream OpenSSL. It's also counting a total number of
bytes, not a single buffer. On a 32-bit platform, one may still transfor
more than 4GiB of data through a single BIO.

Change-Id: I1c668d84ee5ce13f7ab5c476cb168ae9c0e0109e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66167
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
(cherry picked from commit c39e6cd9ec5acebb6de2adffc03cfe03b07f08ab)
This is not sufficient to get the correct type on these constants,
because bindgen is broken and does not correctly bind constants. But in
preparation for that bug being fixed, we should at least mark the
headers correctly.

Bug: 636
Change-Id: Ib42a2d4f42a84b385dd7bd286564ccfe457923eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66227
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 38d17d345c73a3ac0cb451fa0696fd8efabadc0d)
### Description of changes: 
Adds a cryptographic algorithm self test (CAST) for SP 800-56Cr2 OneStep
function.

Per the FIPS 140-3 I.G. Guidance Item:
> if the module implements an approved KDA (SP 800-56C rev1 or rev2),
the module shall perform at
least one CAST covering a SP 800-56Crev1 Section 4 one-step KDF (if
implemented) and another
CAST covering a SP 800-56Crev1 Section 5 two-step KDF (if implemented),
including at least one
auxiliary function for each. For example, if a module implements both a
two-step KDF using either
the HMAC or AES-CMAC auxiliary function, and one-step KDF using either
the hash or HMAC
auxiliary function, then at least two CASTs are required: one for the
two-step KDF (with either
HMAC or AES-CMAC), and the other for the one-step KDF (with either the
hash or HMAC). The
implementations of the auxiliary functions used in the KDA CASTs do not
require separate CASTs.
In addition, the module shall self-test all underlying prerequisite
algorithms used in the remaining SP
800-56C rev1 or rev2 schemes implemented in the module, if the same
implementations of the
underlying algorithms are not already self-tested either on their own or
as part of other higher-level
algorithm CASTs.

I have interpreted this item as requiring us only needing to test either
`SSKDF_digest` or `SSKDF_hmac` but aren't required to test both.

### Testing:
The KAT used in this test case was sourced from
`util/fipstools/acvp/acvptool/test/vectors/KDA-OneStep.bz2` and
`util/fipstools/acvp/acvptool/test/expected/KDA-OneStep.bz2`.

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.
### Issues:
Resolves #CryptoAlg-2493

### Description of changes: 

This PR is the first in a series implementing Ruby's supported subset of
the PKCS7 standard. To do this, we remove AWS-LC's customized ASN.1
serialization logic and delegate to `asn1.h` and `asn1t.h`. We also
update the various `PKCS_type_is_*` no-op functions to actually check
the input's type.

The PKCS7 tests currently contain a test signedData structure that is
encoded using BER. Because our `ASN1_get_object` currently
[disallows](https://github.com/aws/aws-lc/blob/8080ce32b118746475c718d14a609bd1325166e1/crypto/asn1/asn1_lib.c#L136)
indefinite-length BER, we need to modify `d2i_PKCS7` to detect and
convert BER (if present) into DER before parsing. This allows us to
retain backwards compatibility with BER-encoded PKCS7 objects. Please
see the PR description on GitHub for more discussion on this topic.

The next PR in this series will implement the various getters, setters,
and allocation functions needed to support CRruby with testing for each.
Please see PR aws#1780 for an idea of what that will look like.
### Issues
* Addresses: aws/aws-lc-rs#470

### Description of changes: 
* NASM object files were previously setup to always contain debug
information, including source file paths.
* `-gcv8` produces the ["CodeView 8"
format](https://www.nasm.us/doc/nasmdoc8.html#section-8.5.3).
* Simply using `-g` will produce the [default debug
format](https://www.nasm.us/doc/nasmdoc2.html#section-2.1.14) for the
target.
* For "release" and "minsizerel" builds, inclusion of debug information
is not desirable.

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.

---------

Co-authored-by: dkostic <[email protected]>
`OCSP_request_verify` is the last OCSP API needed for Ruby. OCSP request
signatures are optional according to the OCSP RFC, so this is less used
than `OCSP_basic_verify`. Much of the logic & flags are reused across
each other however. A significant portion of the logic was also reworked
without changing the behavior.
Documentation was also rewritten slightly so that it could be applied to
both OCSP verification APIs.
We don't support `OCSP_NOSIGS` for `OCSP_basic_verify` either, so I've
left that out of here for the time being.

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.
### Issues:
Resolves P149524833

### Description of changes: 
* Fix our CodeBuild Purge Lambda


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.
### Description of changes: 
Add `KBKDF_ctr_hmac` FIPS service indicator with approved digest
algorithms. ACVP KAT testing will come in a separate PR.

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.
andrewhop and others added 14 commits August 29, 2024 12:54
… stdout (aws#1800)

### Description of changes: 
Previously only some output followed the user's request using `-out` to
direct the output to a file. This change updates everything to use a
common `BIO` which is `stdout` by default or the user provided file.

### Testing:
Added new tests that test writing to a file directly with `-out` we have
existing tests that cover writing to `stdout` (and then getting
redirected to a file with `>`) works as expected.

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.
### Description of changes: 
Bump AWSLC_API_VERSION to 30 so speed.cc can tell which library versions
have ML-KEM support. Add the ML-KEM benchmark for all future versions of
AWS-LC that support it.

### Testing:
Ran speed locally:
```
./tool/bssl speed -filter ML-KEM
Did 85000 ML-KEM-512 keygen operations in 1002430us (84794.0 ops/sec)
Did 84000 ML-KEM-512 encaps operations in 1008991us (83251.5 ops/sec)
Did 70000 ML-KEM-512 decaps operations in 1009795us (69321.0 ops/sec)
Did 48000 ML-KEM-768 keygen operations in 1014221us (47327.0 ops/sec)
Did 52000 ML-KEM-768 encaps operations in 1011926us (51387.2 ops/sec)
Did 44000 ML-KEM-768 decaps operations in 1012853us (43441.6 ops/sec)
Did 30000 ML-KEM-1024 keygen operations in 1004572us (29863.5 ops/sec)
Did 34000 ML-KEM-1024 encaps operations in 1012471us (33581.2 ops/sec)
Did 29000 ML-KEM-1024 decaps operations in 1021323us (28394.5 ops/sec)
```

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.
On Amazon Linux we install into lib64 and the Ninja executable is called ninja-build. Teach the integration script this.
### Description of changes: 
* Merge changes from BoringSSL. See internal docs.


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.
…ws#1785)

### Description of changes: 
This is the second pull request in a series of three ([prev
PR](aws#1776)). This PR primarily focuses
on refactoring and enhancing the EC_KEY_METHOD API. The key changes are
as follows:

1. **Removal of ECDSA_METHOD**: ECDSA_METHOD support, including the
associated ENGINE functions, has been fully removed and replaced by
EC_KEY_METHOD. This aligns with the transition seen in OpenSSL 1.1.1 and
was also driven by customer requirements.
2. **Hiding Struct definition**: The EC_KEY_METHOD struct definition is
now internal. Unlike OpenSSL, where static allocation was possible,
AWS-LC now supports only dynamic allocation of this struct.
3. **Expansion of EC_KEY_METHOD Functions**: Added various
EC_KEY_METHOD_* functions to create, free, get/set, and manipulate the
EC_KEY_METHOD struct.

### Testing:
Tested with various fields initialized in EC_KEY_METHOD to ensure that
custom functionality is correctly invoked.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants