From b171716a31fedf65e560fc0f79860cd9ac458552 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Mon, 14 Oct 2024 11:35:41 -0700 Subject: [PATCH] Add EC_GROUP mutablility to custom curves (#1881) 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. --- crypto/fipsmodule/ec/ec.c | 48 ++++------ crypto/fipsmodule/ec/ec_test.cc | 159 ++++++++++++++++++++++++++------ crypto/fipsmodule/ec/internal.h | 8 +- include/openssl/ec.h | 24 +++-- 4 files changed, 173 insertions(+), 66 deletions(-) diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c index 3835ce9dd0..1865274995 100644 --- a/crypto/fipsmodule/ec/ec.c +++ b/crypto/fipsmodule/ec/ec.c @@ -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); @@ -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; } @@ -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; } @@ -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 @@ -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))); } @@ -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; } diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index f638a12659..9438f13796 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -684,37 +684,87 @@ class ECPublicKeyTest : public testing::TestWithParam {}; // |i2o_ECPublicKey|. TEST_P(ECPublicKeyTest, DecodeAndEncode) { const auto ¶m = 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_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 = ¶m.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(¶m.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 ¶m = GetParam(); + + EC_KEY *ec_key = EC_KEY_new(); + ASSERT_TRUE(ec_key); + bssl::UniquePtr ec_key_ptr(ec_key); + bssl::UniquePtr 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 = ¶m.input_key[0]; + o2i_ECPublicKey(&ec_key, &inp, param.input_key_len); + ASSERT_EQ(¶m.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); } @@ -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 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 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())); @@ -1174,23 +1224,23 @@ TEST(ECTest, BIGNUMConvert) { bssl::UniquePtr 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 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())); @@ -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|. @@ -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 ctx(BN_CTX_new()); + ASSERT_TRUE(ctx); + bssl::UniquePtr p(BN_bin2bn(kP256P, sizeof(kP256P), nullptr)); + ASSERT_TRUE(p); + bssl::UniquePtr a(BN_bin2bn(kP256A, sizeof(kP256A), nullptr)); + ASSERT_TRUE(a); + bssl::UniquePtr b(BN_bin2bn(kP256B, sizeof(kP256B), nullptr)); + ASSERT_TRUE(b); + bssl::UniquePtr gx(BN_bin2bn(kP256X, sizeof(kP256X), nullptr)); + ASSERT_TRUE(gx); + bssl::UniquePtr gy(BN_bin2bn(kP256Y, sizeof(kP256Y), nullptr)); + ASSERT_TRUE(gy); + bssl::UniquePtr order( + BN_bin2bn(kP256Order, sizeof(kP256Order), nullptr)); + ASSERT_TRUE(order); + + bssl::UniquePtr group( + EC_GROUP_new_curve_GFp(p.get(), a.get(), b.get(), ctx.get())); + ASSERT_TRUE(group); + bssl::UniquePtr 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 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 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)); +} diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index f849ed5cc3..bc2d5897f1 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h @@ -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; @@ -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, diff --git a/include/openssl/ec.h b/include/openssl/ec.h index 8f4161565d..348250890a 100644 --- a/include/openssl/ec.h +++ b/include/openssl/ec.h @@ -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; @@ -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); @@ -524,7 +536,7 @@ 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); @@ -532,7 +544,7 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED void EC_GROUP_set_point_conversion_form( // (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);