Require that EC points are on the curve.

This removes a sharp corner in the API where |ECDH_compute_key| assumed
that callers were either using ephemeral keys, or else had already
checked that the public key was on the curve.

A public key that's not on the curve can be in a small subgroup and thus
the result can leak information about the private key.

This change causes |EC_POINT_set_affine_coordinates_GFp| to require that
points are on the curve. |EC_POINT_oct2point| already does this.

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

crypto/ec/ec.c

@@ -814,7 +814,16 @@ int EC_POINT_set_affine_coordinates_GFp(const EC_GROUP *group, EC_POINT *point,
     OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS);
     return 0;
   }
-  return ec_GFp_simple_point_set_affine_coordinates(group, point, x, y, ctx);
+  if (!ec_GFp_simple_point_set_affine_coordinates(group, point, x, y, ctx)) {
+    return 0;
+  }
+
+  if (!EC_POINT_is_on_curve(group, point, ctx)) {
+    OPENSSL_PUT_ERROR(EC, EC_R_POINT_IS_NOT_ON_CURVE);
+    return 0;
+  }
+
+  return 1;
 }
 
 int EC_POINT_add(const EC_GROUP *group, EC_POINT *r, const EC_POINT *a,

crypto/ec/ec_test.cc

@@ -173,12 +173,84 @@ static bool TestZeroPadding() {
   return true;
 }
 
+bool TestSetAffine(const int nid) {
+  ScopedEC_KEY key(EC_KEY_new_by_curve_name(nid));
+  if (!key) {
+    return false;
+  }
+
+  const EC_GROUP *const group = EC_KEY_get0_group(key.get());
+
+  if (!EC_KEY_generate_key(key.get())) {
+    fprintf(stderr, "EC_KEY_generate_key failed with nid %d\n", nid);
+    ERR_print_errors_fp(stderr);
+    return false;
+  }
+
+  if (!EC_POINT_is_on_curve(group, EC_KEY_get0_public_key(key.get()),
+                            nullptr)) {
+    fprintf(stderr, "generated point is not on curve with nid %d", nid);
+    ERR_print_errors_fp(stderr);
+    return false;
+  }
+
+  ScopedBIGNUM x(BN_new());
+  ScopedBIGNUM y(BN_new());
+  if (!EC_POINT_get_affine_coordinates_GFp(group,
+                                           EC_KEY_get0_public_key(key.get()),
+                                           x.get(), y.get(), nullptr)) {
+    fprintf(stderr, "EC_POINT_get_affine_coordinates_GFp failed with nid %d\n",
+            nid);
+    ERR_print_errors_fp(stderr);
+    return false;
+  }
+
+  ScopedEC_POINT point(EC_POINT_new(group));
+  if (!point) {
+    return false;
+  }
+
+  if (!EC_POINT_set_affine_coordinates_GFp(group, point.get(), x.get(), y.get(),
+                                           nullptr)) {
+    fprintf(stderr, "EC_POINT_set_affine_coordinates_GFp failed with nid %d\n",
+            nid);
+    ERR_print_errors_fp(stderr);
+    return false;
+  }
+
+  // Subtract one from |y| to make the point no longer on the curve.
+  if (!BN_sub(y.get(), y.get(), BN_value_one())) {
+    return false;
+  }
+
+  ScopedEC_POINT invalid_point(EC_POINT_new(group));
+  if (!invalid_point) {
+    return false;
+  }
+
+  if (EC_POINT_set_affine_coordinates_GFp(group, invalid_point.get(), x.get(),
+                                          y.get(), nullptr)) {
+    fprintf(stderr,
+            "EC_POINT_set_affine_coordinates_GFp succeeded with invalid "
+            "coordinates with nid %d\n",
+            nid);
+    ERR_print_errors_fp(stderr);
+    return false;
+  }
+
+  return true;
+}
+
 int main(void) {
   CRYPTO_library_init();
   ERR_load_crypto_strings();
 
   if (!Testd2i_ECPrivateKey() ||
-      !TestZeroPadding()) {
+      !TestZeroPadding() ||
+      !TestSetAffine(NID_secp224r1) ||
+      !TestSetAffine(NID_X9_62_prime256v1) ||
+      !TestSetAffine(NID_secp384r1) ||
+      !TestSetAffine(NID_secp521r1)) {
     fprintf(stderr, "failed\n");
     return 1;
   }

include/openssl/ec.h

@@ -220,8 +220,10 @@ OPENSSL_EXPORT int EC_POINT_get_affine_coordinates_GFp(const EC_GROUP *group,
                                                        BIGNUM *x, BIGNUM *y,
                                                        BN_CTX *ctx);
 
-/* EC_POINT_set_affine_coordinates_GFp sets the value of |p| to be (|x|, |y|). The
- * |ctx| argument may be used if not NULL. */
+/* EC_POINT_set_affine_coordinates_GFp sets the value of |p| to be (|x|, |y|).
+ * The |ctx| argument may be used if not NULL. It returns one on success or
+ * zero on error. Note that, unlike with OpenSSL, it's considered an error if
+ * the point is not on the curve. */
 OPENSSL_EXPORT int EC_POINT_set_affine_coordinates_GFp(const EC_GROUP *group,
                                                        EC_POINT *point,
                                                        const BIGNUM *x,