Restore EC_GROUP_new_by_curve_name and EC_GROUP_set_generator.

Having a different API for this case than upstream is more trouble than is
worth it. This is sad since the new API avoids incomplete EC_GROUPs at least,
but I don't believe supporting this pair of functions will be significantly
more complex than supporting EC_GROUP_new_arbitrary even when we have static
EC_GROUPs.

For now, keep both sets of APIs around, but we'll be able to remove the scar
tissue once Conscrypt's complex dependencies are resolved.

Make the restored EC_GROUP_set_generator somewhat simpler than before by
removing the ability to call it multiple times and with some parameters set to
NULL. Keep the test.

Change-Id: I64e3f6a742678411904cb15c0ad15d56cdae4a73
Reviewed-on: https://boringssl-review.googlesource.com/7432
Reviewed-by: David Benjamin <davidben@google.com>

crypto/ec/ec.c

@@ -350,8 +350,8 @@ EC_GROUP *ec_group_new(const EC_METHOD *meth) {
   return ret;
 }
 
-static EC_GROUP *ec_group_new_curve_GFp(const BIGNUM *p, const BIGNUM *a,
-                                        const BIGNUM *b, BN_CTX *ctx) {
+EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a,
+                                 const BIGNUM *b, BN_CTX *ctx) {
   const EC_METHOD *meth = EC_GFp_mont_method();
   EC_GROUP *ret;
 
@@ -371,35 +371,49 @@ static EC_GROUP *ec_group_new_curve_GFp(const BIGNUM *p, const BIGNUM *a,
   return ret;
 }
 
+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->generator != 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. */
+    return 0;
+  }
+
+  group->generator = EC_POINT_new(group);
+  return group->generator != NULL &&
+         EC_POINT_copy(group->generator, generator) &&
+         BN_copy(&group->order, order) &&
+         BN_copy(&group->cofactor, cofactor);
+}
+
 EC_GROUP *EC_GROUP_new_arbitrary(const BIGNUM *p, const BIGNUM *a,
                                  const BIGNUM *b, const BIGNUM *gx,
                                  const BIGNUM *gy, const BIGNUM *order,
                                  const BIGNUM *cofactor) {
-  EC_GROUP *ret = NULL;
-  BN_CTX *ctx;
-
-  ctx = BN_CTX_new();
+  BN_CTX *ctx = BN_CTX_new();
   if (ctx == NULL) {
-    goto err;
+    return NULL;
   }
 
-  ret = ec_group_new_curve_GFp(p, a, b, ctx);
+  EC_POINT *generator = NULL;
+  EC_GROUP *ret = EC_GROUP_new_curve_GFp(p, a, b, ctx);
   if (ret == NULL) {
     goto err;
   }
 
-  ret->generator = EC_POINT_new(ret);
-  if (ret->generator == NULL ||
-      !EC_POINT_set_affine_coordinates_GFp(ret, ret->generator, gx, gy, ctx) ||
-      !BN_copy(&ret->order, order) ||
-      !BN_copy(&ret->cofactor, cofactor)) {
+  generator = EC_POINT_new(ret);
+  if (generator == NULL ||
+      !EC_POINT_set_affine_coordinates_GFp(ret, generator, gx, gy, ctx) ||
+      !EC_GROUP_set_generator(ret, generator, order, cofactor)) {
     goto err;
   }
 
+  EC_POINT_free(generator);
   BN_CTX_free(ctx);
   return ret;
 
 err:
+  EC_POINT_free(generator);
   EC_GROUP_free(ret);
   BN_CTX_free(ctx);
   return NULL;
@@ -438,7 +452,7 @@ static EC_GROUP *ec_group_new_from_data(unsigned built_in_index) {
       goto err;
     }
   } else {
-    if ((group = ec_group_new_curve_GFp(p, a, b, ctx)) == NULL) {
+    if ((group = EC_GROUP_new_curve_GFp(p, a, b, ctx)) == NULL) {
       OPENSSL_PUT_ERROR(EC, ERR_R_EC_LIB);
       goto err;
     }

crypto/ec/ec_test.cc

@@ -349,23 +349,32 @@ static bool TestArbitraryCurve() {
       0xff, 0xff, 0xff, 0xff, 0xff, 0xbc, 0xe6, 0xfa, 0xad, 0xa7, 0x17,
       0x9e, 0x84, 0xf3, 0xb9, 0xca, 0xc2, 0xfc, 0x63, 0x25, 0x51,
   };
+  ScopedBN_CTX ctx(BN_CTX_new());
   ScopedBIGNUM p(BN_bin2bn(kP, sizeof(kP), nullptr));
   ScopedBIGNUM a(BN_bin2bn(kA, sizeof(kA), nullptr));
   ScopedBIGNUM b(BN_bin2bn(kB, sizeof(kB), nullptr));
-  ScopedBIGNUM x(BN_bin2bn(kX, sizeof(kX), nullptr));
-  ScopedBIGNUM y(BN_bin2bn(kY, sizeof(kY), nullptr));
+  ScopedBIGNUM gx(BN_bin2bn(kX, sizeof(kX), nullptr));
+  ScopedBIGNUM gy(BN_bin2bn(kY, sizeof(kY), nullptr));
   ScopedBIGNUM order(BN_bin2bn(kOrder, sizeof(kOrder), nullptr));
   ScopedBIGNUM cofactor(BN_new());
-  if (!p || !a || !b || !x || !y || !order || !cofactor ||
+  if (!ctx || !p || !a || !b || !gx || !gy || !order || !cofactor ||
       !BN_set_word(cofactor.get(), 1)) {
     return false;
   }
-  ScopedEC_GROUP group(EC_GROUP_new_arbitrary(p.get(), a.get(), b.get(),
-                                              x.get(), y.get(), order.get(),
-                                              cofactor.get()));
+
+  ScopedEC_GROUP group(
+      EC_GROUP_new_curve_GFp(p.get(), a.get(), b.get(), ctx.get()));
   if (!group) {
     return false;
   }
+  ScopedEC_POINT generator(EC_POINT_new(group.get()));
+  if (!generator ||
+      !EC_POINT_set_affine_coordinates_GFp(group.get(), generator.get(),
+                                           gx.get(), gy.get(), ctx.get()) ||
+      !EC_GROUP_set_generator(group.get(), generator.get(), order.get(),
+                              cofactor.get())) {
+    return false;
+  }
 
   // |group| should not have a curve name.
   if (EC_GROUP_get_curve_name(group.get()) != NID_undef) {
@@ -375,7 +384,8 @@ static bool TestArbitraryCurve() {
   // Copy |key| to |key2| using |group|.
   ScopedEC_KEY key2(EC_KEY_new());
   ScopedEC_POINT point(EC_POINT_new(group.get()));
-  if (!key2 || !point ||
+  ScopedBIGNUM x(BN_new()), y(BN_new());
+  if (!key2 || !point || !x || !y ||
       !EC_KEY_set_group(key2.get(), group.get()) ||
       !EC_KEY_set_private_key(key2.get(), EC_KEY_get0_private_key(key.get())) ||
       !EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(key.get()),
@@ -394,6 +404,37 @@ static bool TestArbitraryCurve() {
     return false;
   }
 
+  // Repeat the process for |EC_GROUP_new_arbitrary|.
+  group.reset(EC_GROUP_new_arbitrary(p.get(), a.get(), b.get(), gx.get(),
+                                     gy.get(), order.get(), cofactor.get()));
+  if (!group) {
+    return false;
+  }
+
+  // |group| should not have a curve name.
+  if (EC_GROUP_get_curve_name(group.get()) != NID_undef) {
+    return false;
+  }
+
+  // Copy |key| to |key2| using |group|.
+  key2.reset(EC_KEY_new());
+  point.reset(EC_POINT_new(group.get()));
+  if (!key2 || !point ||
+      !EC_KEY_set_group(key2.get(), group.get()) ||
+      !EC_KEY_set_private_key(key2.get(), EC_KEY_get0_private_key(key.get())) ||
+      !EC_POINT_set_affine_coordinates_GFp(group.get(), point.get(), x.get(),
+                                           y.get(), nullptr) ||
+      !EC_KEY_set_public_key(key2.get(), point.get())) {
+    fprintf(stderr, "Could not copy key.\n");
+    return false;
+  }
+
+  // The key must be valid according to the new group too.
+  if (!EC_KEY_check_key(key2.get())) {
+    fprintf(stderr, "Copied key is not valid.\n");
+    return false;
+  }
+
   return true;
 }
 

include/openssl/ec.h

@@ -289,13 +289,37 @@ OPENSSL_EXPORT int EC_POINT_mul(const EC_GROUP *group, EC_POINT *r,
 
 /* Deprecated functions. */
 
-/* EC_GROUP_new_arbitrary creates a new, arbitrary elliptic curve group based on
- * the equation y² = x³ + a·x + b. The generator is set to (gx, gy) which must
- * have the given order and cofactor. It returns the new group or NULL on error.
+/* EC_GROUP_new_curve_GFp creates a new, arbitrary elliptic curve group based
+ * on the equation y² = x³ + a·x + b. It returns the new group or NULL on
+ * error.
+ *
+ * This new group has no generator. It is an error to use a generator-less group
+ * with any functions except for |EC_GROUP_free|, |EC_POINT_new|,
+ * |EC_POINT_set_affine_coordinates_GFp|, and |EC_GROUP_set_generator|.
  *
  * |EC_GROUP|s returned by this function will always compare as unequal via
  * |EC_GROUP_cmp| (even to themselves). |EC_GROUP_get_curve_name| will always
- * return |NID_undef|. */
+ * return |NID_undef|.
+ *
+ * Avoid using arbitrary curves and use |EC_GROUP_new_by_curve_name| instead. */
+OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p,
+                                                const BIGNUM *a,
+                                                const BIGNUM *b, BN_CTX *ctx);
+
+/* EC_GROUP_set_generator sets the generator for |group| to |generator|, which
+ * must have the given order and cofactor. It may only be used with |EC_GROUP|
+ * objects returned by |EC_GROUP_new_curve_GFp| and may only be used once on
+ * each group. */
+OPENSSL_EXPORT int EC_GROUP_set_generator(EC_GROUP *group,
+                                          const EC_POINT *generator,
+                                          const BIGNUM *order,
+                                          const BIGNUM *cofactor);
+
+/* EC_GROUP_new_arbitrary calls |EC_GROUP_new_curve_GFp| and
+ * |EC_GROUP_set_generator|.
+ *
+ * TODO(davidben): Remove this once
+ * https://android-review.googlesource.com/#/c/207990/ has cycled in. */
 OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_arbitrary(
     const BIGNUM *p, const BIGNUM *a, const BIGNUM *b, const BIGNUM *gx,
     const BIGNUM *gy, const BIGNUM *order, const BIGNUM *cofactor);