Use constant curve-specific groups whenever possible

Also remove unnecessary EC_GROUP_free calls. EC_GROUP_free is only
necessary in codepaths where arbitrary groups are possible.

Bug: 20
Change-Id: I3dfb7f07b890ab002ba8a302724d8bc671590cfe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60932
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>

Author: davidben

crypto/ec_extra/ec_asn1.c

@@ -94,7 +94,6 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) {
   }
 
   // Parse the optional parameters field.
-  EC_GROUP *inner_group = NULL;
   EC_KEY *ret = NULL;
   BIGNUM *priv_key = NULL;
   if (CBS_peek_asn1_tag(&ec_private_key, kParametersTag)) {
@@ -107,7 +106,7 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) {
       OPENSSL_PUT_ERROR(EC, EC_R_DECODE_ERROR);
       goto err;
     }
-    inner_group = EC_KEY_parse_parameters(&child);
+    const EC_GROUP *inner_group = EC_KEY_parse_parameters(&child);
     if (inner_group == NULL) {
       goto err;
     }
@@ -189,13 +188,11 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) {
   }
 
   BN_free(priv_key);
-  EC_GROUP_free(inner_group);
   return ret;
 
 err:
   EC_KEY_free(ret);
   BN_free(priv_key);
-  EC_GROUP_free(inner_group);
   return NULL;
 }
 
@@ -353,8 +350,6 @@ EC_GROUP *EC_KEY_parse_curve_name(CBS *cbs) {
   for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kAllGroups); i++) {
     const EC_GROUP *group = kAllGroups[i]();
     if (CBS_mem_equal(&named_curve, group->oid, group->oid_len)) {
-      // TODO(davidben): Remove unnecessary calls to |EC_GROUP_free| within the
-      // library.
       return (EC_GROUP *)group;
     }
   }
@@ -433,8 +428,6 @@ EC_GROUP *EC_KEY_parse_parameters(CBS *cbs) {
   BN_free(b);
   BN_free(x);
   BN_free(y);
-  // TODO(davidben): Remove unnecessary calls to |EC_GROUP_free| within the
-  // library.
   return (EC_GROUP *)ret;
 }
 
@@ -492,18 +485,16 @@ EC_KEY *d2i_ECParameters(EC_KEY **out_key, const uint8_t **inp, long len) {
 
   CBS cbs;
   CBS_init(&cbs, *inp, (size_t)len);
-  EC_GROUP *group = EC_KEY_parse_parameters(&cbs);
+  const EC_GROUP *group = EC_KEY_parse_parameters(&cbs);
   if (group == NULL) {
     return NULL;
   }
 
   EC_KEY *ret = EC_KEY_new();
   if (ret == NULL || !EC_KEY_set_group(ret, group)) {
-    EC_GROUP_free(group);
     EC_KEY_free(ret);
     return NULL;
   }
-  EC_GROUP_free(group);
 
   if (out_key != NULL) {
     EC_KEY_free(*out_key);

crypto/ecdh_extra/ecdh_test.cc

@@ -35,24 +35,23 @@
 #include "../test/wycheproof_util.h"
 
 
-static bssl::UniquePtr<EC_GROUP> GetCurve(FileTest *t, const char *key) {
+static const EC_GROUP *GetCurve(FileTest *t, const char *key) {
   std::string curve_name;
   if (!t->GetAttribute(&curve_name, key)) {
     return nullptr;
   }
 
   if (curve_name == "P-224") {
-    return bssl::UniquePtr<EC_GROUP>(EC_GROUP_new_by_curve_name(NID_secp224r1));
+    return EC_group_p224();
   }
   if (curve_name == "P-256") {
-    return bssl::UniquePtr<EC_GROUP>(EC_GROUP_new_by_curve_name(
-        NID_X9_62_prime256v1));
+    return EC_group_p256();
   }
   if (curve_name == "P-384") {
-    return bssl::UniquePtr<EC_GROUP>(EC_GROUP_new_by_curve_name(NID_secp384r1));
+    return EC_group_p384();
   }
   if (curve_name == "P-521") {
-    return bssl::UniquePtr<EC_GROUP>(EC_GROUP_new_by_curve_name(NID_secp521r1));
+    return EC_group_p521();
   }
 
   t->PrintLine("Unknown curve '%s'", curve_name.c_str());
@@ -70,7 +69,7 @@ static bssl::UniquePtr<BIGNUM> GetBIGNUM(FileTest *t, const char *key) {
 
 TEST(ECDHTest, TestVectors) {
   FileTestGTest("crypto/ecdh_extra/ecdh_tests.txt", [](FileTest *t) {
-    bssl::UniquePtr<EC_GROUP> group = GetCurve(t, "Curve");
+    const EC_GROUP *group = GetCurve(t, "Curve");
     ASSERT_TRUE(group);
     bssl::UniquePtr<BIGNUM> priv_key = GetBIGNUM(t, "Private");
     ASSERT_TRUE(priv_key);
@@ -87,16 +86,16 @@ TEST(ECDHTest, TestVectors) {
 
     bssl::UniquePtr<EC_KEY> key(EC_KEY_new());
     ASSERT_TRUE(key);
-    bssl::UniquePtr<EC_POINT> pub_key(EC_POINT_new(group.get()));
+    bssl::UniquePtr<EC_POINT> pub_key(EC_POINT_new(group));
     ASSERT_TRUE(pub_key);
-    bssl::UniquePtr<EC_POINT> peer_pub_key(EC_POINT_new(group.get()));
+    bssl::UniquePtr<EC_POINT> peer_pub_key(EC_POINT_new(group));
     ASSERT_TRUE(peer_pub_key);
-    ASSERT_TRUE(EC_KEY_set_group(key.get(), group.get()));
+    ASSERT_TRUE(EC_KEY_set_group(key.get(), group));
     ASSERT_TRUE(EC_KEY_set_private_key(key.get(), priv_key.get()));
-    ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(group.get(), pub_key.get(),
+    ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(group, pub_key.get(),
                                                     x.get(), y.get(), nullptr));
     ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(
-        group.get(), peer_pub_key.get(), peer_x.get(), peer_y.get(), nullptr));
+        group, peer_pub_key.get(), peer_x.get(), peer_y.get(), nullptr));
     ASSERT_TRUE(EC_KEY_set_public_key(key.get(), pub_key.get()));
     ASSERT_TRUE(EC_KEY_check_key(key.get()));
 
@@ -130,7 +129,7 @@ TEST(ECDHTest, TestVectors) {
 static void RunWycheproofTest(FileTest *t) {
   t->IgnoreInstruction("encoding");
 
-  bssl::UniquePtr<EC_GROUP> group = GetWycheproofCurve(t, "curve", true);
+  const EC_GROUP *group = GetWycheproofCurve(t, "curve", true);
   ASSERT_TRUE(group);
   bssl::UniquePtr<BIGNUM> priv_key = GetWycheproofBIGNUM(t, "private", false);
   ASSERT_TRUE(priv_key);
@@ -157,10 +156,10 @@ static void RunWycheproofTest(FileTest *t) {
 
   bssl::UniquePtr<EC_KEY> key(EC_KEY_new());
   ASSERT_TRUE(key);
-  ASSERT_TRUE(EC_KEY_set_group(key.get(), group.get()));
+  ASSERT_TRUE(EC_KEY_set_group(key.get(), group));
   ASSERT_TRUE(EC_KEY_set_private_key(key.get(), priv_key.get()));
 
-  std::vector<uint8_t> actual((EC_GROUP_get_degree(group.get()) + 7) / 8);
+  std::vector<uint8_t> actual((EC_GROUP_get_degree(group) + 7) / 8);
   int ret =
       ECDH_compute_key(actual.data(), actual.size(),
                        EC_KEY_get0_public_key(peer_ec), key.get(), nullptr);

crypto/err/ssl.errordata

@@ -121,7 +121,6 @@ SSL,253,NO_COMMON_SIGNATURE_ALGORITHMS
 SSL,178,NO_COMPRESSION_SPECIFIED
 SSL,265,NO_GROUPS_SPECIFIED
 SSL,179,NO_METHOD_SPECIFIED
-SSL,180,NO_P256_SUPPORT
 SSL,181,NO_PRIVATE_KEY_ASSIGNED
 SSL,182,NO_RENEGOTIATION
 SSL,183,NO_REQUIRED_DIGEST

crypto/evp/p_ec.c

@@ -75,7 +75,7 @@
 typedef struct {
   // message digest
   const EVP_MD *md;
-  EC_GROUP *gen_group;
+  const EC_GROUP *gen_group;
 } EC_PKEY_CTX;
 
 
@@ -111,7 +111,6 @@ static void pkey_ec_cleanup(EVP_PKEY_CTX *ctx) {
     return;
   }
 
-  EC_GROUP_free(dctx->gen_group);
   OPENSSL_free(dctx);
 }
 
@@ -195,11 +194,10 @@ static int pkey_ec_ctrl(EVP_PKEY_CTX *ctx, int type, int p1, void *p2) {
       return 1;
 
     case EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID: {
-      EC_GROUP *group = EC_GROUP_new_by_curve_name(p1);
+      const EC_GROUP *group = EC_GROUP_new_by_curve_name(p1);
       if (group == NULL) {
         return 0;
       }
-      EC_GROUP_free(dctx->gen_group);
       dctx->gen_group = group;
       return 1;
     }

crypto/evp/p_ec_asn1.c

@@ -94,7 +94,7 @@ static int eckey_pub_decode(EVP_PKEY *out, CBS *params, CBS *key) {
 
   // The parameters are a named curve.
   EC_KEY *eckey = NULL;
-  EC_GROUP *group = EC_KEY_parse_curve_name(params);
+  const EC_GROUP *group = EC_KEY_parse_curve_name(params);
   if (group == NULL || CBS_len(params) != 0) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     goto err;
@@ -107,12 +107,10 @@ static int eckey_pub_decode(EVP_PKEY *out, CBS *params, CBS *key) {
     goto err;
   }
 
-  EC_GROUP_free(group);
   EVP_PKEY_assign_EC_KEY(out, eckey);
   return 1;
 
 err:
-  EC_GROUP_free(group);
   EC_KEY_free(eckey);
   return 0;
 }
@@ -135,15 +133,13 @@ static int eckey_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) {
 
 static int eckey_priv_decode(EVP_PKEY *out, CBS *params, CBS *key) {
   // See RFC 5915.
-  EC_GROUP *group = EC_KEY_parse_parameters(params);
+  const EC_GROUP *group = EC_KEY_parse_parameters(params);
   if (group == NULL || CBS_len(params) != 0) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
-    EC_GROUP_free(group);
     return 0;
   }
 
   EC_KEY *ec_key = EC_KEY_parse_private_key(key, group);
-  EC_GROUP_free(group);
   if (ec_key == NULL || CBS_len(key) != 0) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     EC_KEY_free(ec_key);