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>
diff --git a/crypto/ec/ec.c b/crypto/ec/ec.c
index c8f8dbc..4891daa 100644
--- a/crypto/ec/ec.c
+++ b/crypto/ec/ec.c
@@ -814,7 +814,16 @@
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,
diff --git a/crypto/ec/ec_test.cc b/crypto/ec/ec_test.cc
index 5af42d5..3f45bd0 100644
--- a/crypto/ec/ec_test.cc
+++ b/crypto/ec/ec_test.cc
@@ -173,12 +173,84 @@
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;
}
diff --git a/include/openssl/ec.h b/include/openssl/ec.h
index fe1c89e..ac36a32 100644
--- a/include/openssl/ec.h
+++ b/include/openssl/ec.h
@@ -220,8 +220,10 @@
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,
