commit 3ab6ad6abd1b30a6d5902e55a3e3a89a9b1efc7a
author David Benjamin <davidben@google.com> Mon Dec 25 14:56:21 2017 -0500
committer Adam Langley <agl@google.com> Mon Jan 08 19:50:42 2018 +0000
tree c6b8a85894c3851395118ee81a36d1c9e6163089
parent 99084cdd76d78e982b89ed1ae2522b9b5aeedf3b

Simplify EC_KEY_set_public_key_affine_coordinates.

EC_POINT_set_affine_coordinates_GFp already rejects coordinates which
are out of range. There's no need to double-check.

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

crypto/fipsmodule/ec/ec_key.c

diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c
index 4e0bcb2..084d33b 100644
--- a/crypto/fipsmodule/ec/ec_key.c
+++ b/crypto/fipsmodule/ec/ec_key.c
@@ -390,8 +390,6 @@
 
 int EC_KEY_set_public_key_affine_coordinates(EC_KEY *key, BIGNUM *x,
                                              BIGNUM *y) {
-  BN_CTX *ctx = NULL;
-  BIGNUM *tx, *ty;
   EC_POINT *point = NULL;
   int ok = 0;
 
@@ -399,51 +397,18 @@
     OPENSSL_PUT_ERROR(EC, ERR_R_PASSED_NULL_PARAMETER);
     return 0;
   }
-  ctx = BN_CTX_new();
 
-  if (ctx == NULL) {
-    return 0;
-  }
-
-  BN_CTX_start(ctx);
   point = EC_POINT_new(key->group);
-
-  if (point == NULL) {
-    goto err;
-  }
-
-  tx = BN_CTX_get(ctx);
-  ty = BN_CTX_get(ctx);
-  if (tx == NULL ||
-      ty == NULL) {
-    goto err;
-  }
-
-  if (!EC_POINT_set_affine_coordinates_GFp(key->group, point, x, y, ctx) ||
-      !EC_POINT_get_affine_coordinates_GFp(key->group, point, tx, ty, ctx)) {
-    goto err;
-  }
-
-  // Check if retrieved coordinates match originals: if not values
-  // are out of range.
-  if (BN_cmp(x, tx) || BN_cmp(y, ty)) {
-    OPENSSL_PUT_ERROR(EC, EC_R_COORDINATES_OUT_OF_RANGE);
-    goto err;
-  }
-
-  if (!EC_KEY_set_public_key(key, point)) {
-    goto err;
-  }
-
-  if (EC_KEY_check_key(key) == 0) {
+  if (point == NULL ||
+      !EC_POINT_set_affine_coordinates_GFp(key->group, point, x, y, NULL) ||
+      !EC_KEY_set_public_key(key, point) ||
+      !EC_KEY_check_key(key)) {
     goto err;
   }
 
   ok = 1;
 
 err:
-  BN_CTX_end(ctx);
-  BN_CTX_free(ctx);
   EC_POINT_free(point);
   return ok;
 }

crypto/fipsmodule/ec/ec_test.cc

diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc
index d2cd2af..e69f8d7 100644
--- a/crypto/fipsmodule/ec/ec_test.cc
+++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -352,8 +352,12 @@
   ASSERT_TRUE(x);
   bssl::UniquePtr<BIGNUM> y(BN_new());
   ASSERT_TRUE(y);
+  bssl::UniquePtr<BIGNUM> p(BN_new());
+  ASSERT_TRUE(p);
   EXPECT_TRUE(EC_POINT_get_affine_coordinates_GFp(
       group, EC_KEY_get0_public_key(key.get()), x.get(), y.get(), nullptr));
+  EXPECT_TRUE(
+      EC_GROUP_get_curve_GFp(group, p.get(), nullptr, nullptr, nullptr));
 
   // Points on the curve should be accepted.
   auto point = bssl::UniquePtr<EC_POINT>(EC_POINT_new(group));
@@ -369,6 +373,15 @@
   ASSERT_TRUE(invalid_point);
   EXPECT_FALSE(EC_POINT_set_affine_coordinates_GFp(group, invalid_point.get(),
                                                    x.get(), y.get(), nullptr));
+
+  // Coordinates out of range should be rejected.
+  EXPECT_TRUE(BN_add(y.get(), y.get(), BN_value_one()));
+  EXPECT_TRUE(BN_add(y.get(), y.get(), p.get()));
+
+  EXPECT_FALSE(EC_POINT_set_affine_coordinates_GFp(group, invalid_point.get(),
+                                                   x.get(), y.get(), nullptr));
+  EXPECT_FALSE(
+      EC_KEY_set_public_key_affine_coordinates(key.get(), x.get(), y.get()));
 }
 
 TEST_P(ECCurveTest, GenerateFIPS) {