Move BN_nnmod calls out of low-level group_set_curve.
group_set_curve is called when instantiating the built-in curves and
when creating arbitrary curves. The former has non-NULL BN_CTXs and
fully reduced inputs. Move the logic for this to the deprecated
EC_GROUP_new_curve_GFp function so it can be dropped from most binaries.
Change-Id: I5ff60d6d51acb79fbcb32588e6324a5b2627b6d2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40544
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c
index 158d66c..d32ac7e 100644
--- a/crypto/fipsmodule/ec/ec.c
+++ b/crypto/fipsmodule/ec/ec.c
@@ -321,20 +321,37 @@
return NULL;
}
- EC_GROUP *ret = ec_group_new(EC_GFp_mont_method());
- if (ret == NULL) {
- return NULL;
+ BN_CTX *new_ctx = NULL;
+ if (ctx == NULL) {
+ ctx = new_ctx = BN_CTX_new();
+ if (ctx == NULL) {
+ return NULL;
+ }
}
- if (ret->meth->group_set_curve == NULL) {
- OPENSSL_PUT_ERROR(EC, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
- EC_GROUP_free(ret);
- return NULL;
+ // Historically, |a| and |b| were not required to be fully reduced.
+ // TODO(davidben): Can this be removed?
+ EC_GROUP *ret = NULL;
+ BN_CTX_start(ctx);
+ BIGNUM *a_reduced = BN_CTX_get(ctx);
+ BIGNUM *b_reduced = BN_CTX_get(ctx);
+ if (a_reduced == NULL || b_reduced == NULL ||
+ !BN_nnmod(a_reduced, a, p, ctx) ||
+ !BN_nnmod(b_reduced, b, p, ctx)) {
+ goto err;
}
- if (!ret->meth->group_set_curve(ret, p, a, b, ctx)) {
+
+ ret = ec_group_new(EC_GFp_mont_method());
+ if (ret == NULL ||
+ !ret->meth->group_set_curve(ret, p, a_reduced, b_reduced, ctx)) {
EC_GROUP_free(ret);
- return NULL;
+ ret = NULL;
+ goto err;
}
+
+err:
+ BN_CTX_end(ctx);
+ BN_CTX_free(new_ctx);
return ret;
}
diff --git a/crypto/fipsmodule/ec/ec_montgomery.c b/crypto/fipsmodule/ec/ec_montgomery.c
index 5553f05..22251e0 100644
--- a/crypto/fipsmodule/ec/ec_montgomery.c
+++ b/crypto/fipsmodule/ec/ec_montgomery.c
@@ -92,35 +92,20 @@
int ec_GFp_mont_group_set_curve(EC_GROUP *group, const BIGNUM *p,
const BIGNUM *a, const BIGNUM *b, BN_CTX *ctx) {
- BN_CTX *new_ctx = NULL;
- int ret = 0;
-
BN_MONT_CTX_free(group->mont);
- group->mont = NULL;
-
- if (ctx == NULL) {
- ctx = new_ctx = BN_CTX_new();
- if (ctx == NULL) {
- return 0;
- }
- }
-
group->mont = BN_MONT_CTX_new_for_modulus(p, ctx);
if (group->mont == NULL) {
OPENSSL_PUT_ERROR(EC, ERR_R_BN_LIB);
- goto err;
+ return 0;
}
- ret = ec_GFp_simple_group_set_curve(group, p, a, b, ctx);
-
- if (!ret) {
+ if (!ec_GFp_simple_group_set_curve(group, p, a, b, ctx)) {
BN_MONT_CTX_free(group->mont);
group->mont = NULL;
+ return 0;
}
-err:
- BN_CTX_free(new_ctx);
- return ret;
+ return 1;
}
static void ec_GFp_mont_felem_to_montgomery(const EC_GROUP *group,
diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc
index 1c8a9ce..6a6a71b 100644
--- a/crypto/fipsmodule/ec/ec_test.cc
+++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -330,6 +330,23 @@
EXPECT_EQ(0, EC_GROUP_cmp(group.get(), group4.get(), NULL));
#endif
+
+ // group5 is the same group, but the curve coefficients are passed in
+ // unreduced and the caller does not pass in a |BN_CTX|.
+ ASSERT_TRUE(BN_sub(a.get(), a.get(), p.get()));
+ ASSERT_TRUE(BN_add(b.get(), b.get(), p.get()));
+ bssl::UniquePtr<EC_GROUP> group5(
+ EC_GROUP_new_curve_GFp(p.get(), a.get(), b.get(), NULL));
+ ASSERT_TRUE(group5);
+ bssl::UniquePtr<EC_POINT> generator5(EC_POINT_new(group5.get()));
+ ASSERT_TRUE(generator5);
+ ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(
+ group5.get(), generator5.get(), gx.get(), gy.get(), ctx.get()));
+ ASSERT_TRUE(EC_GROUP_set_generator(group5.get(), generator5.get(),
+ order.get(), BN_value_one()));
+
+ EXPECT_EQ(0, EC_GROUP_cmp(group.get(), group.get(), NULL));
+ EXPECT_EQ(0, EC_GROUP_cmp(group5.get(), group.get(), NULL));
}
TEST(ECTest, SetKeyWithoutGroup) {
diff --git a/crypto/fipsmodule/ec/simple.c b/crypto/fipsmodule/ec/simple.c
index 8536b80..af91da9 100644
--- a/crypto/fipsmodule/ec/simple.c
+++ b/crypto/fipsmodule/ec/simple.c
@@ -101,22 +101,13 @@
int ec_GFp_simple_group_set_curve(EC_GROUP *group, const BIGNUM *p,
const BIGNUM *a, const BIGNUM *b,
BN_CTX *ctx) {
- int ret = 0;
- BN_CTX *new_ctx = NULL;
-
// p must be a prime > 3
if (BN_num_bits(p) <= 2 || !BN_is_odd(p)) {
OPENSSL_PUT_ERROR(EC, EC_R_INVALID_FIELD);
return 0;
}
- if (ctx == NULL) {
- ctx = new_ctx = BN_CTX_new();
- if (ctx == NULL) {
- return 0;
- }
- }
-
+ int ret = 0;
BN_CTX_start(ctx);
BIGNUM *tmp = BN_CTX_get(ctx);
if (tmp == NULL) {
@@ -131,33 +122,23 @@
// Store the field in minimal form, so it can be used with |BN_ULONG| arrays.
bn_set_minimal_width(&group->field);
- // group->a
- if (!BN_nnmod(tmp, a, &group->field, ctx) ||
- !ec_bignum_to_felem(group, &group->a, tmp)) {
+ if (!ec_bignum_to_felem(group, &group->a, a) ||
+ !ec_bignum_to_felem(group, &group->b, b) ||
+ !ec_bignum_to_felem(group, &group->one, BN_value_one())) {
goto err;
}
// group->a_is_minus3
- if (!BN_add_word(tmp, 3)) {
+ if (!BN_copy(tmp, a) ||
+ !BN_add_word(tmp, 3)) {
goto err;
}
group->a_is_minus3 = (0 == BN_cmp(tmp, &group->field));
- // group->b
- if (!BN_nnmod(tmp, b, &group->field, ctx) ||
- !ec_bignum_to_felem(group, &group->b, tmp)) {
- goto err;
- }
-
- if (!ec_bignum_to_felem(group, &group->one, BN_value_one())) {
- goto err;
- }
-
ret = 1;
err:
BN_CTX_end(ctx);
- BN_CTX_free(new_ctx);
return ret;
}
