Fold EC_GROUP_new_curve_GFp and EC_GROUP_set_generator into a EC_GROUP_new_arbitrary.

This is only for Conscrypt which always calls the pair in succession. (Indeed
it wouldn't make any sense to not call it.) Remove those two APIs and replace
with a single merged API. This way incomplete EC_GROUPs never escape outside
our API boundary and EC_GROUPs may *finally* be made immutable.

Also add a test for this to make sure I didn't mess it up.

Add a temporary BORINGSSL_201512 define to ease the transition for Conscrypt.
Conscrypt requires https://android-review.googlesource.com/#/c/187801/ before
picking up this change.

Change-Id: I3706c2ceac31ed2313175ba5ee724bd5c74ef6e1
Reviewed-on: https://boringssl-review.googlesource.com/6550
Reviewed-by: Adam Langley <agl@google.com>

crypto/ec/ec.c

@@ -350,8 +350,8 @@ EC_GROUP *ec_group_new(const EC_METHOD *meth) {
   return ret;
 }
 
-EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a,
-                                 const BIGNUM *b, BN_CTX *ctx) {
+static 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,42 +371,38 @@ 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) {
-    /* |EC_GROUP_set_generator| should only be used with |EC_GROUP|s returned
-     * by |EC_GROUP_new_curve_GFp|. */
-    return 0;
-  }
+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;
 
-  if (group->generator == NULL) {
-    group->generator = EC_POINT_new(group);
-    if (group->generator == NULL) {
-      return 0;
-    }
+  ctx = BN_CTX_new();
+  if (ctx == NULL) {
+    goto err;
   }
 
-  if (!EC_POINT_copy(group->generator, generator)) {
-    return 0;
+  ret = ec_group_new_curve_GFp(p, a, b, ctx);
+  if (ret == NULL) {
+    goto err;
   }
 
-  if (order != NULL) {
-    if (!BN_copy(&group->order, order)) {
-      return 0;
-    }
-  } else {
-    BN_zero(&group->order);
+  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)) {
+    goto err;
   }
 
-  if (cofactor != NULL) {
-    if (!BN_copy(&group->cofactor, cofactor)) {
-      return 0;
-    }
-  } else {
-    BN_zero(&group->cofactor);
-  }
+  BN_CTX_free(ctx);
+  return ret;
 
-  return 1;
+err:
+  EC_GROUP_free(ret);
+  BN_CTX_free(ctx);
+  return NULL;
 }
 
 static EC_GROUP *ec_group_new_from_data(unsigned built_in_index) {
@@ -442,7 +438,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

@@ -240,6 +240,92 @@ bool TestSetAffine(const int nid) {
   return true;
 }
 
+static bool TestArbitraryCurve() {
+  // Make a P-256 key and extract the affine coordinates.
+  ScopedEC_KEY key(EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
+  if (!key || !EC_KEY_generate_key(key.get())) {
+    return false;
+  }
+
+  // Make an arbitrary curve which is identical to P-256.
+  static const uint8_t kP[] = {
+      0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff,
+      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+  };
+  static const uint8_t kA[] = {
+      0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff,
+      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfc,
+  };
+  static const uint8_t kB[] = {
+      0x5a, 0xc6, 0x35, 0xd8, 0xaa, 0x3a, 0x93, 0xe7, 0xb3, 0xeb, 0xbd,
+      0x55, 0x76, 0x98, 0x86, 0xbc, 0x65, 0x1d, 0x06, 0xb0, 0xcc, 0x53,
+      0xb0, 0xf6, 0x3b, 0xce, 0x3c, 0x3e, 0x27, 0xd2, 0x60, 0x4b,
+  };
+  static const uint8_t kX[] = {
+      0x6b, 0x17, 0xd1, 0xf2, 0xe1, 0x2c, 0x42, 0x47, 0xf8, 0xbc, 0xe6,
+      0xe5, 0x63, 0xa4, 0x40, 0xf2, 0x77, 0x03, 0x7d, 0x81, 0x2d, 0xeb,
+      0x33, 0xa0, 0xf4, 0xa1, 0x39, 0x45, 0xd8, 0x98, 0xc2, 0x96,
+  };
+  static const uint8_t kY[] = {
+      0x4f, 0xe3, 0x42, 0xe2, 0xfe, 0x1a, 0x7f, 0x9b, 0x8e, 0xe7, 0xeb,
+      0x4a, 0x7c, 0x0f, 0x9e, 0x16, 0x2b, 0xce, 0x33, 0x57, 0x6b, 0x31,
+      0x5e, 0xce, 0xcb, 0xb6, 0x40, 0x68, 0x37, 0xbf, 0x51, 0xf5,
+  };
+  static const uint8_t kOrder[] = {
+      0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff,
+      0xff, 0xff, 0xff, 0xff, 0xff, 0xbc, 0xe6, 0xfa, 0xad, 0xa7, 0x17,
+      0x9e, 0x84, 0xf3, 0xb9, 0xca, 0xc2, 0xfc, 0x63, 0x25, 0x51,
+  };
+  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 order(BN_bin2bn(kOrder, sizeof(kOrder), nullptr));
+  ScopedBIGNUM cofactor(BN_new());
+  if (!p || !a || !b || !x || !y || !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()));
+  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|.
+  ScopedEC_KEY key2(EC_KEY_new());
+  ScopedEC_POINT point(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_get_affine_coordinates_GFp(EC_KEY_get0_group(key.get()),
+                                           EC_KEY_get0_public_key(key.get()),
+                                           x.get(), y.get(), nullptr) ||
+      !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;
+}
+
 int main(void) {
   CRYPTO_library_init();
   ERR_load_crypto_strings();
@@ -249,7 +335,8 @@ int main(void) {
       !TestSetAffine(NID_secp224r1) ||
       !TestSetAffine(NID_X9_62_prime256v1) ||
       !TestSetAffine(NID_secp384r1) ||
-      !TestSetAffine(NID_secp521r1)) {
+      !TestSetAffine(NID_secp521r1) ||
+      !TestArbitraryCurve()) {
     fprintf(stderr, "failed\n");
     return 1;
   }

include/openssl/base.h

@@ -108,7 +108,7 @@ extern "C" {
 #endif
 
 #define OPENSSL_IS_BORINGSSL
-#define BORINGSSL_201510
+#define BORINGSSL_201512
 #define OPENSSL_VERSION_NUMBER 0x10002000
 #define SSLEAY_VERSION_NUMBER OPENSSL_VERSION_NUMBER
 

include/openssl/ec.h

@@ -271,16 +271,16 @@ OPENSSL_EXPORT int EC_POINT_mul(const EC_GROUP *group, EC_POINT *r,
 
 /* Deprecated functions. */
 
-/* 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.
+/* 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|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|. */
-OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p,
-                                                const BIGNUM *a,
-                                                const BIGNUM *b, BN_CTX *ctx);
+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);
 
 /* EC_GROUP_get_order sets |*order| to the order of |group|, if it's not
  * NULL. It returns one on success and zero otherwise. |ctx| is ignored. Use
@@ -288,14 +288,6 @@ OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p,
 OPENSSL_EXPORT int EC_GROUP_get_order(const EC_GROUP *group, BIGNUM *order,
                                       BN_CTX *ctx);
 
-/* EC_GROUP_set_generator sets the generator for |group| to |generator|, which
- * must have the given order and cofactor. This should only be used with
- * |EC_GROUP| objects returned by |EC_GROUP_new_curve_GFp|. */
-OPENSSL_EXPORT int EC_GROUP_set_generator(EC_GROUP *group,
-                                          const EC_POINT *generator,
-                                          const BIGNUM *order,
-                                          const BIGNUM *cofactor);
-
 /* EC_GROUP_set_asn1_flag does nothing. */
 OPENSSL_EXPORT void EC_GROUP_set_asn1_flag(EC_GROUP *group, int flag);