Refcount EC_GROUP.

I really need to resurrect the CL to make them entirely static (https://crbug.com/boringssl/20), but, in the meantime, to make replacing the EC_METHOD pointer in EC_POINT with EC_GROUP not *completely* insane, make them refcounted. OpenSSL did not do this because their EC_GROUPs are mutable (EC_GROUP_set_asn1_flag and EC_GROUP_set_point_conversion_form). Ours are immutable but for the two-function dance around custom curves (more of OpenSSL's habit of making their objects too complex), which is good enough to refcount. Change-Id: I3650993737a97da0ddcf0e5fb7a15876e724cadc Reviewed-on: https://boringssl-review.googlesource.com/22244 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> CQ-Verified: CQ bot account: [email protected] <[email protected]>
aws · Oct 27, 2017 · 51073ce · 51073ce
1 parent d24fd47
commit 51073ce
Show file tree

Hide file tree

Showing 7 changed files with 11 additions and 78 deletions.
diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c
@@ -354,6 +354,7 @@ EC_GROUP *ec_group_new(const EC_METHOD *meth) {
   }
   OPENSSL_memset(ret, 0, sizeof(EC_GROUP));
 
+  ret->references = 1;
   ret->meth = meth;
   BN_init(&ret->order);
 
@@ -500,11 +501,12 @@ EC_GROUP *EC_GROUP_new_by_curve_name(int nid) {
 }
 
 void EC_GROUP_free(EC_GROUP *group) {
-  if (!group) {
+  if (!group ||
+      !CRYPTO_refcount_dec_and_test_zero(&group->references)) {
     return;
   }
 
-  if (group->meth->group_finish != 0) {
+  if (group->meth->group_finish != NULL) {
     group->meth->group_finish(group);
   }
 
@@ -523,36 +525,11 @@ EC_GROUP *EC_GROUP_dup(const EC_GROUP *a) {
     return NULL;
   }
 
-  if (a->meth->group_copy == NULL) {
-    OPENSSL_PUT_ERROR(EC, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
-    return NULL;
-  }
-
-  EC_GROUP *ret = ec_group_new(a->meth);
-  if (ret == NULL) {
-    return NULL;
-  }
-
-  ret->order_mont = a->order_mont;
-  ret->curve_name = a->curve_name;
-
-  if (a->generator != NULL) {
-    ret->generator = EC_POINT_dup(a->generator, ret);
-    if (ret->generator == NULL) {
-      goto err;
-    }
-  }
-
-  if (!BN_copy(&ret->order, &a->order) ||
-      !ret->meth->group_copy(ret, a)) {
-    goto err;
-  }
-
-  return ret;
-
-err:
-  EC_GROUP_free(ret);
-  return NULL;
+  // 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;
 }
 
 int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ignored) {

diff --git a/crypto/fipsmodule/ec/ec_montgomery.c b/crypto/fipsmodule/ec/ec_montgomery.c
@@ -90,32 +90,6 @@ void ec_GFp_mont_group_finish(EC_GROUP *group) {
   ec_GFp_simple_group_finish(group);
 }
 
-int ec_GFp_mont_group_copy(EC_GROUP *dest, const EC_GROUP *src) {
-  BN_MONT_CTX_free(dest->mont);
-  dest->mont = NULL;
-
-  if (!ec_GFp_simple_group_copy(dest, src)) {
-    return 0;
-  }
-
-  if (src->mont != NULL) {
-    dest->mont = BN_MONT_CTX_new();
-    if (dest->mont == NULL) {
-      return 0;
-    }
-    if (!BN_MONT_CTX_copy(dest->mont, src->mont)) {
-      goto err;
-    }
-  }
-
-  return 1;
-
-err:
-  BN_MONT_CTX_free(dest->mont);
-  dest->mont = NULL;
-  return 0;
-}
-
 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;
@@ -293,7 +267,6 @@ static int ec_GFp_mont_point_get_affine_coordinates(const EC_GROUP *group,
 DEFINE_METHOD_FUNCTION(EC_METHOD, EC_GFp_mont_method) {
   out->group_init = ec_GFp_mont_group_init;
   out->group_finish = ec_GFp_mont_group_finish;
-  out->group_copy = ec_GFp_mont_group_copy;
   out->group_set_curve = ec_GFp_mont_group_set_curve;
   out->point_get_affine_coordinates = ec_GFp_mont_point_get_affine_coordinates;
   out->mul = ec_wNAF_mul /* XXX: Not constant time. */;

diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h
@@ -82,7 +82,6 @@ extern "C" {
 struct ec_method_st {
   int (*group_init)(EC_GROUP *);
   void (*group_finish)(EC_GROUP *);
-  int (*group_copy)(EC_GROUP *, const EC_GROUP *);
   int (*group_set_curve)(EC_GROUP *, const BIGNUM *p, const BIGNUM *a,
                          const BIGNUM *b, BN_CTX *);
   int (*point_get_affine_coordinates)(const EC_GROUP *, const EC_POINT *,
@@ -130,6 +129,8 @@ struct ec_group_st {
 
   int a_is_minus3;  // enable optimized point arithmetics for special case
 
+  CRYPTO_refcount_t references;
+
   BN_MONT_CTX *mont;  // Montgomery structure.
 
   BIGNUM one;  // The value one.
@@ -145,7 +146,6 @@ struct ec_point_st {
 } /* EC_POINT */;
 
 EC_GROUP *ec_group_new(const EC_METHOD *meth);
-int ec_group_copy(EC_GROUP *dest, const EC_GROUP *src);
 
 // ec_group_get_order_mont returns a Montgomery context for operations modulo
 // |group|'s order. It may return NULL in the case that |group| is not a
@@ -158,7 +158,6 @@ int ec_wNAF_mul(const EC_GROUP *group, EC_POINT *r, const BIGNUM *g_scalar,
 // method functions in simple.c
 int ec_GFp_simple_group_init(EC_GROUP *);
 void ec_GFp_simple_group_finish(EC_GROUP *);
-int ec_GFp_simple_group_copy(EC_GROUP *, const EC_GROUP *);
 int ec_GFp_simple_group_set_curve(EC_GROUP *, const BIGNUM *p, const BIGNUM *a,
                                   const BIGNUM *b, BN_CTX *);
 int ec_GFp_simple_group_get_curve(const EC_GROUP *, BIGNUM *p, BIGNUM *a,
@@ -204,7 +203,6 @@ int ec_GFp_mont_group_init(EC_GROUP *);
 int ec_GFp_mont_group_set_curve(EC_GROUP *, const BIGNUM *p, const BIGNUM *a,
                                 const BIGNUM *b, BN_CTX *);
 void ec_GFp_mont_group_finish(EC_GROUP *);
-int ec_GFp_mont_group_copy(EC_GROUP *, const EC_GROUP *);
 int ec_GFp_mont_field_mul(const EC_GROUP *, BIGNUM *r, const BIGNUM *a,
                           const BIGNUM *b, BN_CTX *);
 int ec_GFp_mont_field_sqr(const EC_GROUP *, BIGNUM *r, const BIGNUM *a,

diff --git a/crypto/fipsmodule/ec/p224-64.c b/crypto/fipsmodule/ec/p224-64.c
@@ -1151,7 +1151,6 @@ static int ec_GFp_nistp224_points_mul(const EC_GROUP *group, EC_POINT *r,
 DEFINE_METHOD_FUNCTION(EC_METHOD, EC_GFp_nistp224_method) {
   out->group_init = ec_GFp_simple_group_init;
   out->group_finish = ec_GFp_simple_group_finish;
-  out->group_copy = ec_GFp_simple_group_copy;
   out->group_set_curve = ec_GFp_simple_group_set_curve;
   out->point_get_affine_coordinates =
       ec_GFp_nistp224_point_get_affine_coordinates;

diff --git a/crypto/fipsmodule/ec/p256-64.c b/crypto/fipsmodule/ec/p256-64.c
@@ -1694,7 +1694,6 @@ static int ec_GFp_nistp256_points_mul(const EC_GROUP *group, EC_POINT *r,
 DEFINE_METHOD_FUNCTION(EC_METHOD, EC_GFp_nistp256_method) {
   out->group_init = ec_GFp_simple_group_init;
   out->group_finish = ec_GFp_simple_group_finish;
-  out->group_copy = ec_GFp_simple_group_copy;
   out->group_set_curve = ec_GFp_simple_group_set_curve;
   out->point_get_affine_coordinates =
       ec_GFp_nistp256_point_get_affine_coordinates;

diff --git a/crypto/fipsmodule/ec/p256-x86_64.c b/crypto/fipsmodule/ec/p256-x86_64.c
@@ -547,7 +547,6 @@ static int ecp_nistz256_get_affine(const EC_GROUP *group, const EC_POINT *point,
 DEFINE_METHOD_FUNCTION(EC_METHOD, EC_GFp_nistz256_method) {
   out->group_init = ec_GFp_mont_group_init;
   out->group_finish = ec_GFp_mont_group_finish;
-  out->group_copy = ec_GFp_mont_group_copy;
   out->group_set_curve = ec_GFp_mont_group_set_curve;
   out->point_get_affine_coordinates = ecp_nistz256_get_affine;
   out->mul = ecp_nistz256_points_mul;

diff --git a/crypto/fipsmodule/ec/simple.c b/crypto/fipsmodule/ec/simple.c
@@ -104,18 +104,6 @@ void ec_GFp_simple_group_finish(EC_GROUP *group) {
   BN_free(&group->one);
 }
 
-int ec_GFp_simple_group_copy(EC_GROUP *dest, const EC_GROUP *src) {
-  if (!BN_copy(&dest->field, &src->field) ||
-      !BN_copy(&dest->a, &src->a) ||
-      !BN_copy(&dest->b, &src->b) ||
-      !BN_copy(&dest->one, &src->one)) {
-    return 0;
-  }
-
-  dest->a_is_minus3 = src->a_is_minus3;
-  return 1;
-}
-
 int ec_GFp_simple_group_set_curve(EC_GROUP *group, const BIGNUM *p,
                                   const BIGNUM *a, const BIGNUM *b,
                                   BN_CTX *ctx) {