Skip to content
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

P-384/521 fallback to small implementation when OPENSSL_SMALL is set #984

Merged
merged 14 commits into from
May 9, 2023

Conversation

dkostic
Copy link
Contributor

@dkostic dkostic commented Apr 26, 2023

Issues:

N/A

Description of changes:

We should use the generic EC_GFp_mont_method for P-384/521 when OPENSSL_SMALL flag is defined.

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.

@dkostic dkostic enabled auto-merge (squash) April 26, 2023 17:03
Copy link
Contributor

@andrewhop andrewhop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the size of libcrypto before/after this cahnge? Have you checked that EC_GFp_nistp521_method/EC_GFp_nistp384_method are not in the final library when SMALL is set?

@dkostic
Copy link
Contributor Author

dkostic commented Apr 28, 2023

What is the size of libcrypto before/after this cahnge? Have you checked that EC_GFp_nistp521_method/EC_GFp_nistp384_method are not in the final library when SMALL is set?

good point, I actually didn't guard the implementation in p384/521.c with #if !defined(OPENSSL_SMALL) so it was compiled even though it wasn't used. Fixed that now.

The OPENSSL_SMALL Release build on my M1 mac goes from 8.3MB to 8.0MB.

andrewhop
andrewhop previously approved these changes Apr 28, 2023
@torben-hansen torben-hansen requested a review from nebeid May 2, 2023 17:43
@@ -130,12 +130,19 @@ TEST(ECDHTest, TestVectors) {
});
}

static int has_uint128_and_not_small() {
// Returns 1 if the curve defined by |nid| is using Montgomery representation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment in

// The following call converts the point to Montgomery form for P-256/384.
// For P-224, when the functions from simple.c are used, i.e. when
// group->meth = EC_GFp_nistp224_method, the coordinate representation
// is not changed. This is determined based on compile flags in ec.c
// that are also used below in has_uint128_and_not_small().
// For P-521, the plain non-Motgomery representation is always used.
needs to be adjusted:

// The following call converts the point to Montgomery form for P-256/384.
// For P-224, when the functions from simple.c are used, i.e. when
// group->meth = EC_GFp_nistp224_method, the coordinate representation
// is not changed. This is determined based on compilation flags in ec.c
// that are duplicated below in is_curve_using_mont_felem_impl().
// For P-521, the plain non-Motgomery representation is always used also except for 
// the flag used in the same function.

Or feel free to edit it to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

static int has_uint128_and_not_small() {
// Returns 1 if the curve defined by |nid| is using Montgomery representation
// for field elements (based on the build configuration). Returns 0 otherwise.
static int is_curve_using_mont_felem_impl(int nid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment on

// The following call converts the point to Montgomery form for P-256/384.
// For P-224, when the functions from simple.c are used, i.e. when
// group->meth = EC_GFp_nistp224_method, the coordinate representation
// is not changed. This is determined based on compile flags in ec.c
// that are also used below in has_uint128_and_not_small().
// For P-521, the plain non-Motgomery representation is always used.
as on ecdh_test.cc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@@ -30,7 +32,7 @@
// #define p384_felem_add(out, in0, in1) bignum_add_p384(out, in0, in1)
// when s2n-bignum is used.
//
#if !defined(OPENSSL_NO_ASM) && \
#if !defined(OPENSSL_NO_ASM) && !defined(OPENSSL_SMALL) && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check now a duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, removed it.

crypto/fipsmodule/ec/ec_test.cc Outdated Show resolved Hide resolved
crypto/ecdh_extra/ecdh_test.cc Outdated Show resolved Hide resolved
dkostic and others added 2 commits May 5, 2023 08:28
Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>
Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>
@dkostic dkostic merged commit 82e43ab into aws:main May 9, 2023
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.

3 participants