Create mutable EC_GROUP API for OpenSSL compatibility #1860

samuel40791765 · 2024-09-18T01:19:07Z

Issues:

Addresses CryptoAlg-2547

Description of changes:

Further investigation led us to discover that Ruby depends on mutable EC_GROUPs from OpenSSL. These were made immutable with the introduction of our default static EC_GROUP curves, which have been beneficial for us and our existing consumers. This was done by making the mutable portions of EC_GROUPs no-ops (EC_GROUP_set_asn1_flag and EC_GROUP_set_point_conversion_form), which OpenSSL couldn't do due to their more complex use case.

Ruby (and possibly other dynamic languages) mutates the data being stored in EC_GROUP and depends on it for subsequent operations. Since our default groups are static and immutable, this causes issues when integrating with Ruby. We don't want to sacrifice the benefits of static groups and reversing the optional call to EC_GROUP_free is a dangerously silent memory leak.
We've decided to introduce a special API for mutable EC_GROUPs for our default curves. This gives us a path forward for Ruby and keeps the benefits of static curves for existing consumers. Custom curves are already dynamically allocated so they don't need another special API, but the additional complexities for maintaining the underlying generator warrant another commit separate from this.

Call-outs:

Custom curves aren't mutable just yet, but will be in another PR.
It would be nice to get additional verification that the bookkeeping in EC_GROUP_dup and EC_GROUP_cmp were done right.

Testing:

Extended against tests using existing EC_GROUPs and conversion formats. These were focused on retrieving the conv_form from the EC_GROUP to use.

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.

codecov-commenter · 2024-09-18T01:41:46Z

Codecov Report

Attention: Patch coverage is 90.67797% with 11 lines in your changes missing coverage. Please review.

Project coverage is 78.49%. Comparing base (9d21f38) to head (db51866).
Report is 103 commits behind head on main.

Files with missing lines	Patch %	Lines
crypto/fipsmodule/ec/ec.c	88.88%	8 Missing ⚠️
crypto/fipsmodule/ec/ec_key.c	40.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
- Coverage   78.51%   78.49%   -0.03%     
==========================================
  Files         583      585       +2     
  Lines       98809    99602     +793     
  Branches    14159    14256      +97     
==========================================
+ Hits        77583    78178     +595     
- Misses      20598    20787     +189     
- Partials      628      637       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crypto/fipsmodule/ec/ec.c

WillChilds-Klein · 2024-09-23T17:52:34Z

crypto/fipsmodule/ec/ec.c

+  ret->has_order = a->has_order;
+  ret->a_is_minus3 = a->a_is_minus3;
+  ret->curve_name = a->curve_name;
+  ret->conv_form = a->conv_form;
+  ret->field_greater_than_order = a->field_greater_than_order;


it might be difficult for us to "remember" to update this method when new fields are added to the EC_GROUP struct. since you're doing a deep copy, there's no easy solution here but you could automatically cover all the shallow elements for free by doing a OPENSSL_memdup, then manually updating the deep elements.

After staring more closely into the EC_GROUP structure, it seems like only generator->group is a pointer redirection (and it's directing to itself). I think we can do a shallow copy here, all the tests pass with this behavior.
The BN_MONT_CTXs field and order also have a pointer in their BIGNUMs. I've updated to do a deep copy there as well.

aws-lc/crypto/fipsmodule/ec/internal.h

Lines 621 to 654 in 2835116

struct ec_group_st {

const EC_METHOD *meth;

// Unlike all other |EC_POINT|s, |generator| does not own |generator->group|

// to avoid a reference cycle. Additionally, Z is guaranteed to be one, so X

// and Y are suitable for use as an |EC_AFFINE|. Before |has_order| is set, Z

// is one, but X and Y are uninitialized.

EC_POINT generator;

BN_MONT_CTX order;

BN_MONT_CTX field;

EC_FELEM a, b; // Curve coefficients.

// comment is a human-readable string describing the curve.

const char *comment;

int curve_name; // optional NID for named curve

uint8_t oid[9];

uint8_t oid_len;

// a_is_minus3 is one if |a| is -3 mod |field| and zero otherwise. Point

// arithmetic is optimized for -3.

int a_is_minus3;

// has_order is one if |generator| and |order| have been initialized.

int has_order;

// field_greater_than_order is one if |field| is greate than |order| and zero

// otherwise.

int field_greater_than_order;

CRYPTO_refcount_t references;

} /* EC_GROUP */;

crypto/fipsmodule/ec/ec.c

crypto/fipsmodule/ec/ec_key.c

crypto/fipsmodule/ec/internal.h

WillChilds-Klein · 2024-09-30T18:30:27Z

crypto/fipsmodule/ec/ec.c

+    if (a->curve_name != b->curve_name) {
+      return 1;
+    }
+    if (a->curve_name != NID_undef) {


nit: so, the idea here is that if both static/built-in curves have curve_name == NID_undef, we want to fall through to comparing individual parameters? this a bit confusing, you may want to either add a comment explaining the intent or re-write L449-504 to clarify.

I see what the confusion is. I think it'd make sense to move this comment higher and re-clarify the intent behind returning 0 here. As you were saying, the fall through to comparing custom curve contents is what we're trying to convey.

WillChilds-Klein · 2024-09-30T18:36:17Z

crypto/fipsmodule/ec/ec_test.cc

-    nids.push_back(curve.nid);
+  std::vector<CurveParam> nids;
+  for (const auto &curve : curves) {
+    CurveParam curve_param{};


GCC 4 doesn't seem to like lack of initialization here.

WillChilds-Klein · 2024-09-30T18:40:53Z

crypto/fipsmodule/ec/ec.c

    return;
  }

+  if (!group->mutable_ec_group) {
+    if (group->curve_name != NID_undef ||


are we sure that custom curves will always have curve_name == NID_undef? because we currently treat all custom curves as "immutable", if the caller sets some other NID on a custom curve, we'd decrement the refcount and return instead of free'ing. is that what we want?

From what I've observed for our EC_GROUP implementations it does seem like this is the main differentiation between custom and named curves. We don't expose any methods to set an NID for custom curves, although that may change if we are ever forced to implement EC_GROUP_set_curve_name.

It does make me feel a bit more at ease, when looking at OpenSSL's logic for the function though.

void EC_GROUP_set_curve_name(EC_GROUP *group, int nid) { group->curve_name = nid; group->asn1_flag = (nid != NID_undef) ? OPENSSL_EC_NAMED_CURVE : OPENSSL_EC_EXPLICIT_CURVE; }

Only named curves are expected to have NIDs, explicit/custom curves follow the same logic of having NID_undef in OpenSSL as well.

dkostic · 2024-09-27T18:55:56Z

crypto/fipsmodule/ec/ec.c

+  // are additional complexities around untangling the expectations already set
+  // for |EC_GROUP_new_curve_GFp| and |EC_GROUP_set_generator|.
+  // Note: See commit cb16f17b36d9ee9528a06d2e520374a4f177af4d.
+  ret->mutable_ec_group = 0;


should you set conv_form as well?

dkostic · 2024-09-30T22:52:29Z

crypto/fipsmodule/ec/ec.c

-  if (a == NULL ||
-      // Built-in curves are static.
-      a->curve_name != NID_undef) {
+  if (a == NULL) {
    return (EC_GROUP *)a;


Suggested change

return (EC_GROUP *)a;

return NULL;

dkostic · 2024-09-30T22:59:33Z

crypto/fipsmodule/ec/ec.c

-  if (a->curve_name != b->curve_name) {
-    return 1;
+
+  // Directly duplicate the |EC_GROUP| if it was dynamically allocated. We do a


why can't we remove the code above and always duplicate the group like this?

dkostic · 2024-09-30T23:01:36Z

crypto/fipsmodule/ec/ec.c


+int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ignored) {
+  if (!a->mutable_ec_group && !b->mutable_ec_group) {


not sure I understand why you need all this new code, wouldn't the original code be sufficient?

dkostic · 2024-09-30T23:06:35Z

crypto/fipsmodule/ec/ec_key.c

  key->conv_form = cform;
+  if (key->group->mutable_ec_group) {


why do we set the conversion form in the parent EC_GROUP as well? Can that group be associated with other keys?

Create mutable EC_GROUP API for OpenSSL compatibility

7776e4e

samuel40791765 requested review from WillChilds-Klein and dkostic September 18, 2024 01:19

samuel40791765 requested a review from a team as a code owner September 18, 2024 01:19

WillChilds-Klein reviewed Sep 23, 2024

View reviewed changes

PR comments; use shallow copy and fixes

da01032

samuel40791765 force-pushed the ec-conversion-custom-3 branch from 2b18951 to da01032 Compare September 25, 2024 20:08

samuel40791765 mentioned this pull request Sep 26, 2024

Add EC_GROUP mutablility to custom curves #1881

Draft

WillChilds-Klein reviewed Sep 30, 2024

View reviewed changes

PR comments; documentation and fix test

db51866

dkostic reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create mutable EC_GROUP API for OpenSSL compatibility #1860

Create mutable EC_GROUP API for OpenSSL compatibility #1860

samuel40791765 commented Sep 18, 2024

codecov-commenter commented Sep 18, 2024 •

edited

Loading

WillChilds-Klein Sep 23, 2024

samuel40791765 Sep 25, 2024 •

edited

Loading

WillChilds-Klein Sep 30, 2024

samuel40791765 Sep 30, 2024

WillChilds-Klein Sep 30, 2024

WillChilds-Klein Sep 30, 2024

samuel40791765 Sep 30, 2024

dkostic Sep 27, 2024

dkostic Sep 30, 2024

dkostic Sep 30, 2024

dkostic Sep 30, 2024

dkostic Sep 30, 2024

	struct ec_group_st {
	const EC_METHOD *meth;

	// Unlike all other \|EC_POINT\|s, \|generator\| does not own \|generator->group\|
	// to avoid a reference cycle. Additionally, Z is guaranteed to be one, so X
	// and Y are suitable for use as an \|EC_AFFINE\|. Before \|has_order\| is set, Z
	// is one, but X and Y are uninitialized.
	EC_POINT generator;

	BN_MONT_CTX order;
	BN_MONT_CTX field;

	EC_FELEM a, b; // Curve coefficients.

	// comment is a human-readable string describing the curve.
	const char *comment;

	int curve_name; // optional NID for named curve
	uint8_t oid[9];
	uint8_t oid_len;

	// a_is_minus3 is one if \|a\| is -3 mod \|field\| and zero otherwise. Point
	// arithmetic is optimized for -3.
	int a_is_minus3;

	// has_order is one if \|generator\| and \|order\| have been initialized.
	int has_order;

	// field_greater_than_order is one if \|field\| is greate than \|order\| and zero
	// otherwise.
	int field_greater_than_order;

	CRYPTO_refcount_t references;
	} /* EC_GROUP */;


		int EC_GROUP_cmp(const EC_GROUP a, const EC_GROUP b, BN_CTX *ignored) {
		if (!a->mutable_ec_group && !b->mutable_ec_group) {

Create mutable EC_GROUP API for OpenSSL compatibility #1860

Are you sure you want to change the base?

Create mutable EC_GROUP API for OpenSSL compatibility #1860

Conversation

samuel40791765 commented Sep 18, 2024

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Sep 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

samuel40791765 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 18, 2024 •

edited

Loading

samuel40791765 Sep 25, 2024 •

edited

Loading