diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c index 3835ce9dd0..8daaac642c 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 @@ -492,10 +478,6 @@ EC_GROUP *EC_GROUP_dup(const EC_GROUP *a) { } int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ignored) { - // Note this function returns 0 if equal and non-zero otherwise. - if (a == b) { - return 0; - } // Built-in static curves may be compared by curve name alone. if (a->curve_name != b->curve_name) { return 1; @@ -506,17 +488,20 @@ 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 || - 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); + // |a| and |b| are both custom curves. We compare the entire curve structure + // if both |a| and |b| are complete. If either are incomplete (due to legacy + // OpenSSL mistakes, custom curve construction is sadly done in two parts), we + // only compare the parts that are available. + if (a->has_order && b->has_order) { + return a->meth != b->meth || BN_cmp(&a->order.N, &b->order.N) != 0 || + 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); + } else { + return a->meth != b->meth || 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); + } } diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index d8a690dc65..5d821dc624 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -2633,3 +2633,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..ee6a304184 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h @@ -660,8 +660,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);