Skip to content

Commit

Permalink
add EC_GROUP mutablility to custom curves
Browse files Browse the repository at this point in the history
  • Loading branch information
@samuel40791765
samuel40791765 committed Sep 26, 2024
1 parent da01032 commit e63eb00
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 31 deletions.
49 changes: 20 additions & 29 deletions crypto/fipsmodule/ec/ec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,13 +309,8 @@ 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);
bn_mont_ctx_init(&ret->order);
Expand All @@ -335,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 @@ -442,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 @@ -464,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 @@ -504,17 +491,21 @@ 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);

// 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);
}
}


Expand Down
55 changes: 55 additions & 0 deletions crypto/fipsmodule/ec/ec_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2636,3 +2636,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));
}
2 changes: 0 additions & 2 deletions crypto/fipsmodule/ec/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit e63eb00

Please sign in to comment.