Skip to content

Commit

Permalink
Add EC_GROUP mutablility to custom curves (#1881)
Browse files Browse the repository at this point in the history 
Following up on ca8180c which introduces an API to return a mutable
`EC_GROUP`, this commit applies the same concept for custom curves.
Custom curves are already dynamically allocated, but were logically
immutable due to the strict enforcement of calling
`EC_GROUP_new_curve_GFp` -> `EC_GROUP_set_generator`. Since, 
we're now making the curves mutable, the reference call in
`EC_GROUP` that was used for custom curves is no longer usable. We
actually allocate and duplicate the custom curve structure instead.
`EC_GROUP` comparisons also had to be tweaked to account for
unfinished custom curves.

### Testing:
Test using a custom generator identical to P-256.

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.
  • Loading branch information
@samuel40791765
samuel40791765 authored Oct 14, 2024
1 parent cf969b5 commit b171716
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 66 deletions.
48 changes: 18 additions & 30 deletions crypto/fipsmodule/ec/ec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,13 +309,7 @@ EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a,
if (ret == NULL) {
return NULL;
}
ret->references = 1;
// TODO: Treat custom curves as "immutable" for now. There's the possibility
// that we'll need dynamically allocated custom curve |EC_GROUP|s. But there
// 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;
ret->mutable_ec_group = 1;
ret->conv_form = POINT_CONVERSION_UNCOMPRESSED;
ret->meth = EC_GFp_mont_method();
bn_mont_ctx_init(&ret->field);
Expand All @@ -336,11 +330,10 @@ EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a,
int EC_GROUP_set_generator(EC_GROUP *group, const EC_POINT *generator,
const BIGNUM *order, const BIGNUM *cofactor) {
if (group->curve_name != NID_undef || group->has_order ||
generator->group != group) {
EC_GROUP_cmp(generator->group, group, NULL)) {
// |EC_GROUP_set_generator| may only be used with |EC_GROUP|s returned by
// |EC_GROUP_new_curve_GFp| and may only used once on each group.
// |generator| must have been created from |EC_GROUP_new_curve_GFp|, not a
// copy, so that |generator->group->generator| is set correctly.
// |generator| must be of the same |EC_GROUP| as |group|.
OPENSSL_PUT_ERROR(EC, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
return 0;
}
Expand Down Expand Up @@ -443,8 +436,7 @@ void EC_GROUP_free(EC_GROUP *group) {
}

if (!group->mutable_ec_group) {
if (group->curve_name != NID_undef ||
!CRYPTO_refcount_dec_and_test_zero(&group->references)) {
if (group->curve_name != NID_undef) {
// Built-in curves are static.
return;
}
Expand All @@ -465,12 +457,6 @@ EC_GROUP *EC_GROUP_dup(const EC_GROUP *a) {
// Built-in curves are static.
return (EC_GROUP *)a;
}

// Groups are logically immutable (but for |EC_GROUP_set_generator| which
// must be called early on), so we simply take a reference.
EC_GROUP *group = (EC_GROUP *)a;
CRYPTO_refcount_inc(&group->references);
return group;
}

// Directly duplicate the |EC_GROUP| if it was dynamically allocated. We do a
Expand Down Expand Up @@ -506,17 +492,19 @@ int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ignored) {
return 0;
}

// |a| and |b| are both custom curves. We compare the entire curve
// structure. If |a| or |b| is incomplete (due to legacy OpenSSL mistakes,
// custom curve construction is sadly done in two parts) but otherwise not the
// same object, we consider them always unequal.
return a->meth != b->meth || //
!a->has_order || !b->has_order ||
BN_cmp(&a->order.N, &b->order.N) != 0 ||
// |a| and |b| are both custom curves. If both are incomplete (due to legacy
// OpenSSL mistakes, custom curve construction is sadly done in two parts
// |EC_GROUP_new_curve_GFp| -> |EC_GROUP_set_generator|), we only compare
// the parts that are available.
return a->meth != b->meth || a->has_order != b->has_order ||
BN_cmp(&a->field.N, &b->field.N) != 0 ||
!ec_felem_equal(a, &a->a, &b->a) || //
!ec_felem_equal(a, &a->b, &b->b) ||
!ec_GFp_simple_points_equal(a, &a->generator.raw, &b->generator.raw);
!ec_felem_equal(a, &a->a, &b->a) || !ec_felem_equal(a, &a->b, &b->b) ||
// We compare the rest of the entire curve structure if both |a| and
// |b| are complete.
(a->has_order && b->has_order &&
(BN_cmp(&a->order.N, &b->order.N) != 0 ||
!ec_GFp_simple_points_equal(a, &a->generator.raw,
&b->generator.raw)));
}


Expand Down Expand Up @@ -1177,8 +1165,8 @@ void EC_GROUP_set_point_conversion_form(EC_GROUP *group,
}
}

point_conversion_form_t EC_GROUP_get_point_conversion_form(const EC_GROUP
*group) {
point_conversion_form_t EC_GROUP_get_point_conversion_form(
const EC_GROUP *group) {
return group->conv_form;
}

Expand Down
159 changes: 132 additions & 27 deletions crypto/fipsmodule/ec/ec_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,37 +684,87 @@ class ECPublicKeyTest : public testing::TestWithParam<ECPublicKeyTestInput> {};
// |i2o_ECPublicKey|.
TEST_P(ECPublicKeyTest, DecodeAndEncode) {
const auto &param = GetParam();
const auto input_key = param.input_key;
const auto input_key_len = param.input_key_len;
const auto encode_conv_form = param.encode_conv_form;
const auto expected_output_key = param.expected_output_key;
const auto expected_output_key_len = param.expected_output_key_len;
const auto nid = param.nid;

// Generate |ec_key|.
EC_KEY *ec_key = EC_KEY_new();
ASSERT_TRUE(ec_key);
bssl::UniquePtr<EC_KEY> ec_key_ptr(ec_key);
EC_GROUP *group = EC_GROUP_new_by_curve_name(nid);
EC_GROUP *group = EC_GROUP_new_by_curve_name(param.nid);
ASSERT_TRUE(group);
ASSERT_TRUE(EC_KEY_set_group(ec_key, group));
const uint8_t *inp = &input_key[0];
const uint8_t *inp = &param.input_key[0];
// Decoding an EC point.
o2i_ECPublicKey(&ec_key, &inp, input_key_len);
o2i_ECPublicKey(&ec_key, &inp, param.input_key_len);
// On successful exit of |o2i_ECPublicKey|, |*inp| is advanced by |len| bytes.
ASSERT_EQ(&input_key[0] + input_key_len, inp);
ASSERT_EQ(&param.input_key[0] + param.input_key_len, inp);
// Set |conv_form| of |ec_key|.
EC_KEY_set_conv_form(ec_key, encode_conv_form);
EC_KEY_set_conv_form(ec_key, param.encode_conv_form);
// Encoding |ec_key| to bytes.
// The 1st call of |i2o_ECPublicKey| is to tell the number of bytes in the
// result, whether written or not.
size_t len1 = i2o_ECPublicKey(ec_key, nullptr);
ASSERT_EQ(len1, expected_output_key_len);
uint8_t* p = nullptr;
ASSERT_EQ(len1, param.expected_output_key_len);
uint8_t *p = nullptr;
// The 2nd call of |i2o_ECPublicKey| is to write the number of bytes specified
// by |len1|.
size_t len2 = i2o_ECPublicKey(ec_key, &p);
EXPECT_EQ(len2, expected_output_key_len);
EXPECT_EQ(Bytes(expected_output_key, expected_output_key_len), Bytes(p, len2));
EXPECT_EQ(len2, param.expected_output_key_len);
EXPECT_EQ(Bytes(param.expected_output_key, param.expected_output_key_len),
Bytes(p, len2));

// All the above should succeed, but |ec_key|'s assigned reference to the
// |EC_GROUP| is one of the default static methods. Since these are static,
// both references to |group| should retain the default
// |POINT_CONVERSION_UNCOMPRESSED|. We don't encourage relying on |EC_GROUP|
// to retain any information regarding the |conv_form|, but
// |EC_GROUP_new_by_curve_name_mutable| is available for this specific
// use-case.
EXPECT_EQ(EC_KEY_get_conv_form(ec_key), param.encode_conv_form);
EXPECT_EQ(EC_GROUP_get_point_conversion_form(EC_KEY_get0_group(ec_key)),
POINT_CONVERSION_UNCOMPRESSED);
EXPECT_EQ(EC_GROUP_get_point_conversion_form(group),
POINT_CONVERSION_UNCOMPRESSED);

OPENSSL_free(p);
}

TEST_P(ECPublicKeyTest, DecodeAndEncodeMutable) {
const auto &param = GetParam();

EC_KEY *ec_key = EC_KEY_new();
ASSERT_TRUE(ec_key);
bssl::UniquePtr<EC_KEY> ec_key_ptr(ec_key);
bssl::UniquePtr<EC_GROUP> group(
EC_GROUP_new_by_curve_name_mutable(param.nid));
ASSERT_TRUE(group);

ASSERT_TRUE(EC_KEY_set_group(ec_key, group.get()));
const uint8_t *inp = &param.input_key[0];
o2i_ECPublicKey(&ec_key, &inp, param.input_key_len);
ASSERT_EQ(&param.input_key[0] + param.input_key_len, inp);

// Set |conv_form| of |ec_key|.
EC_KEY_set_conv_form(ec_key, param.encode_conv_form);

size_t len1 = i2o_ECPublicKey(ec_key, nullptr);
ASSERT_EQ(len1, param.expected_output_key_len);
uint8_t *p = nullptr;
size_t len2 = i2o_ECPublicKey(ec_key, &p);
EXPECT_EQ(len2, param.expected_output_key_len);
EXPECT_EQ(Bytes(param.expected_output_key, param.expected_output_key_len),
Bytes(p, len2));

// All the above should succeed, but the original |conv_form| for |group|
// should not be changed with |EC_KEY_set_conv_form|. The |group| reference
// assigned to |EC_KEY| was duplicated with |EC_GROUP_dup|, and is a different
// pointer reference from |group|.
// |group| should retain the default |POINT_CONVERSION_UNCOMPRESSED|.
EXPECT_EQ(EC_KEY_get_conv_form(ec_key), param.encode_conv_form);
EXPECT_EQ(EC_GROUP_get_point_conversion_form(EC_KEY_get0_group(ec_key)),
param.encode_conv_form);
EXPECT_EQ(EC_GROUP_get_point_conversion_form(group.get()),
POINT_CONVERSION_UNCOMPRESSED);

OPENSSL_free(p);
}

Expand Down Expand Up @@ -1160,11 +1210,11 @@ TEST(ECTest, BIGNUMConvert) {
// Convert |EC_POINT| to |BIGNUM| in uncompressed format with
// |EC_POINT_point2bn| and ensure results are the same.
bssl::UniquePtr<BIGNUM> converted_bignum(
EC_POINT_point2bn(group.get(), generator.get(),
EC_POINT_point2bn(group.get(), EC_GROUP_get0_generator(group.get()),
POINT_CONVERSION_UNCOMPRESSED, nullptr, nullptr));
ASSERT_TRUE(converted_bignum);
bssl::UniquePtr<BIGNUM> converted_bignum2(
EC_POINT_point2bn(group2.get(), generator2.get(),
EC_POINT_point2bn(group2.get(), EC_GROUP_get0_generator(group2.get()),
POINT_CONVERSION_UNCOMPRESSED, nullptr, nullptr));
ASSERT_TRUE(converted_bignum2);
EXPECT_EQ(0, BN_cmp(converted_bignum.get(), converted_bignum2.get()));
Expand All @@ -1174,23 +1224,23 @@ TEST(ECTest, BIGNUMConvert) {
bssl::UniquePtr<EC_POINT> converted_generator(
EC_POINT_bn2point(group.get(), converted_bignum.get(), nullptr, nullptr));
ASSERT_TRUE(converted_generator);
EXPECT_EQ(0, EC_POINT_cmp(group.get(), generator.get(),
EXPECT_EQ(0, EC_POINT_cmp(group.get(), EC_GROUP_get0_generator(group.get()),
converted_generator.get(), nullptr));
bssl::UniquePtr<EC_POINT> converted_generator2(EC_POINT_bn2point(
group2.get(), converted_bignum2.get(), nullptr, nullptr));
ASSERT_TRUE(converted_generator2);
EXPECT_EQ(0, EC_POINT_cmp(group2.get(), generator2.get(),
EXPECT_EQ(0, EC_POINT_cmp(group2.get(), EC_GROUP_get0_generator(group2.get()),
converted_generator2.get(), nullptr));

// Convert |EC_POINT|s in compressed format with |EC_POINT_point2bn| and
// ensure results are the same.
converted_bignum.reset(EC_POINT_point2bn(group.get(), generator.get(),
POINT_CONVERSION_COMPRESSED, nullptr,
nullptr));
converted_bignum.reset(
EC_POINT_point2bn(group.get(), EC_GROUP_get0_generator(group.get()),
POINT_CONVERSION_COMPRESSED, nullptr, nullptr));
ASSERT_TRUE(converted_bignum);
converted_bignum2.reset(EC_POINT_point2bn(group2.get(), generator2.get(),
POINT_CONVERSION_COMPRESSED,
nullptr, nullptr));
converted_bignum2.reset(
EC_POINT_point2bn(group2.get(), EC_GROUP_get0_generator(group2.get()),
POINT_CONVERSION_COMPRESSED, nullptr, nullptr));
ASSERT_TRUE(converted_bignum2);
EXPECT_EQ(0, BN_cmp(converted_bignum.get(), converted_bignum2.get()));

Expand All @@ -1199,12 +1249,12 @@ TEST(ECTest, BIGNUMConvert) {
converted_generator.reset(
EC_POINT_bn2point(group.get(), converted_bignum.get(), nullptr, nullptr));
ASSERT_TRUE(converted_generator);
EXPECT_EQ(0, EC_POINT_cmp(group.get(), generator.get(),
EXPECT_EQ(0, EC_POINT_cmp(group.get(), EC_GROUP_get0_generator(group.get()),
converted_generator.get(), nullptr));
converted_generator2.reset(EC_POINT_bn2point(
group2.get(), converted_bignum2.get(), nullptr, nullptr));
ASSERT_TRUE(converted_generator2);
EXPECT_EQ(0, EC_POINT_cmp(group2.get(), generator2.get(),
EXPECT_EQ(0, EC_POINT_cmp(group2.get(), EC_GROUP_get0_generator(group2.get()),
converted_generator2.get(), nullptr));

// Test specific openssl/openssl#10258 case for |BN_zero|.
Expand Down Expand Up @@ -2636,3 +2686,58 @@ TEST(ECTest, ECPKParmatersBio) {
EXPECT_TRUE(i2d_ECPKParameters_bio(bio.get(), EC_group_secp256k1()));
EXPECT_EQ(d2i_ECPKParameters_bio(bio.get(), nullptr), EC_group_secp256k1());
}

TEST(ECTest, MutableCustomECGroup) {
bssl::UniquePtr<BN_CTX> ctx(BN_CTX_new());
ASSERT_TRUE(ctx);
bssl::UniquePtr<BIGNUM> p(BN_bin2bn(kP256P, sizeof(kP256P), nullptr));
ASSERT_TRUE(p);
bssl::UniquePtr<BIGNUM> a(BN_bin2bn(kP256A, sizeof(kP256A), nullptr));
ASSERT_TRUE(a);
bssl::UniquePtr<BIGNUM> b(BN_bin2bn(kP256B, sizeof(kP256B), nullptr));
ASSERT_TRUE(b);
bssl::UniquePtr<BIGNUM> gx(BN_bin2bn(kP256X, sizeof(kP256X), nullptr));
ASSERT_TRUE(gx);
bssl::UniquePtr<BIGNUM> gy(BN_bin2bn(kP256Y, sizeof(kP256Y), nullptr));
ASSERT_TRUE(gy);
bssl::UniquePtr<BIGNUM> order(
BN_bin2bn(kP256Order, sizeof(kP256Order), nullptr));
ASSERT_TRUE(order);

bssl::UniquePtr<EC_GROUP> group(
EC_GROUP_new_curve_GFp(p.get(), a.get(), b.get(), ctx.get()));
ASSERT_TRUE(group);
bssl::UniquePtr<EC_POINT> generator(EC_POINT_new(group.get()));
ASSERT_TRUE(generator);
ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(
group.get(), generator.get(), gx.get(), gy.get(), ctx.get()));
ASSERT_TRUE(EC_GROUP_set_generator(group.get(), generator.get(), order.get(),
BN_value_one()));


// Initialize an |EC_POINT| on the corresponding curve.
bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group.get()));
ASSERT_TRUE(EC_POINT_oct2point(
group.get(), point.get(), kP256PublicKey_uncompressed_0x02,
sizeof(kP256PublicKey_uncompressed_0x02), nullptr));

EC_GROUP_set_point_conversion_form(group.get(), POINT_CONVERSION_COMPRESSED);

// Use the saved conversion form in |group|. This should only work with
// |EC_GROUP_new_by_curve_name_mutable|.
std::vector<uint8_t> serialized;
ASSERT_TRUE(EncodeECPoint(&serialized, group.get(), point.get(),
EC_GROUP_get_point_conversion_form(group.get())));
EXPECT_EQ(Bytes(kP256PublicKey_compressed_0x02,
sizeof(kP256PublicKey_compressed_0x02)),
Bytes(serialized));

serialized.clear();
EC_GROUP_set_point_conversion_form(group.get(),
POINT_CONVERSION_UNCOMPRESSED);
ASSERT_TRUE(EncodeECPoint(&serialized, group.get(), point.get(),
EC_GROUP_get_point_conversion_form(group.get())));
EXPECT_EQ(Bytes(kP256PublicKey_uncompressed_0x02,
sizeof(kP256PublicKey_uncompressed_0x02)),
Bytes(serialized));
}
8 changes: 5 additions & 3 deletions crypto/fipsmodule/ec/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,11 @@ struct ec_group_st {
// comment is a human-readable string describing the curve.
const char *comment;

int curve_name; // optional NID for named curve
// curve_name is the optional NID for named curves. |oid| and |oid_len| are
// populated with values corresponding to the named curve's NID.
// |NID_undef| is used to imply that the curve is a custom explicit curve and
// the oid values are empty if so.
int curve_name;
uint8_t oid[9];
uint8_t oid_len;

Expand All @@ -660,8 +664,6 @@ struct ec_group_st {
// with |EC_GROUP_new_by_curve_name_mutable|. The default is zero to indicate
// our built-in static curves.
int mutable_ec_group;

CRYPTO_refcount_t references;
} /* EC_GROUP */;

EC_GROUP *ec_group_new(const EC_METHOD *meth, const BIGNUM *p, const BIGNUM *a,
Expand Down
24 changes: 18 additions & 6 deletions include/openssl/ec.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,6 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED EC_POINT *EC_POINT_bn2point(
OPENSSL_EXPORT int EC_GROUP_get_order(const EC_GROUP *group, BIGNUM *order,
BN_CTX *ctx);

#define OPENSSL_EC_EXPLICIT_CURVE 0
#define OPENSSL_EC_NAMED_CURVE 1

// EC_builtin_curve describes a supported elliptic curve.
typedef struct {
int nid;
Expand Down Expand Up @@ -509,7 +506,22 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED int ECPKParameters_print(
// functions undermines the assumption that our curves are static. Consider
// using the listed alternatives.

// EC_GROUP_set_asn1_flag does nothing.
// OPENSSL_EC_EXPLICIT_CURVE lets OpenSSL encode the curve as explicitly
// encoded curve parameters. AWS-LC does not support this.
//
// Note: Sadly, this was the default prior to OpenSSL 1.1.0.
#define OPENSSL_EC_EXPLICIT_CURVE 0

// OPENSSL_EC_NAMED_CURVE lets OpenSSL encode a named curve form with its
// corresponding NID. This is the only ASN1 encoding method for |EC_GROUP| that
// AWS-LC supports.
#define OPENSSL_EC_NAMED_CURVE 1

// EC_GROUP_set_asn1_flag does nothing. In OpenSSL, |flag| is used to determine
// whether the curve encoding uses explicit parameters or a named curve using an
// ASN1 OID. AWS-LC does not support serialization of explicit curve parameters.
// This behavior is only intended for custom curves. We encourage the use of
// named curves instead.
OPENSSL_EXPORT OPENSSL_DEPRECATED void EC_GROUP_set_asn1_flag(EC_GROUP *group,
int flag);

Expand All @@ -524,15 +536,15 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED int EC_GROUP_get_asn1_flag(
// |EC_GROUP_new_by_curve_name_mutable| for the encoding format to change.
//
// Note: Use |EC_KEY_set_conv_form| / |EC_KEY_get_conv_form| to set and return
// the desired compression format.
// the desired compression format.
OPENSSL_EXPORT OPENSSL_DEPRECATED void EC_GROUP_set_point_conversion_form(
EC_GROUP *group, point_conversion_form_t form);

// EC_GROUP_get_point_conversion_form returns |POINT_CONVERSION_UNCOMPRESSED|
// (the default compression format).
//
// Note: Use |EC_KEY_set_conv_form| / |EC_KEY_get_conv_form| to set and return
// the desired compression format.
// the desired compression format.
OPENSSL_EXPORT OPENSSL_DEPRECATED point_conversion_form_t
EC_GROUP_get_point_conversion_form(const EC_GROUP *group);

Expand Down

0 comments on commit b171716

Please sign in to comment.