RT 4242: reject invalid EC point coordinates

We already test in EC_POINT_oct2point that points are on the curve. To
be on the safe side, move this check to
EC_POINT_set_affine_coordinates_* so as to also check point coordinates
received through some other method.

We do not check projective coordinates, though, as
- it's unlikely that applications would be receiving this primarily
  internal representation from untrusted sources, and
- it's possible that the projective setters are used in a setting where
  performance matters.

Reviewed-by: Rich Salz <rsalz@openssl.org>

3 files changed, 26 insertions, 14 deletions

diff --git a/crypto/ec/ec2_oct.c b/crypto/ec/ec2_oct.c
index 4ac96fd18a..ea88ce860a 100644
--- a/crypto/ec/ec2_oct.c
+++ b/crypto/ec/ec2_oct.c
@@ -334,16 +334,14 @@ int ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
             }
         }
 
+        /*
+         * EC_POINT_set_affine_coordinates_GF2m is responsible for checking that
+         * the point is on the curve.
+         */
         if (!EC_POINT_set_affine_coordinates_GF2m(group, point, x, y, ctx))
             goto err;
     }
 
-    /* test required by X9.62 */
-    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
-        ECerr(EC_F_EC_GF2M_SIMPLE_OCT2POINT, EC_R_POINT_IS_NOT_ON_CURVE);
-        goto err;
-    }
-
     ret = 1;
 
  err:
diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c
index d7383d65c1..fa74ee7644 100644
--- a/crypto/ec/ec_lib.c
+++ b/crypto/ec/ec_lib.c
@@ -700,7 +700,15 @@ int EC_POINT_set_affine_coordinates_GFp(const EC_GROUP *group,
               EC_R_INCOMPATIBLE_OBJECTS);
         return 0;
     }
-    return group->meth->point_set_affine_coordinates(group, point, x, y, ctx);
+    if (!group->meth->point_set_affine_coordinates(group, point, x, y, ctx))
+        return 0;
+
+    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
+        ECerr(EC_F_EC_POINT_SET_AFFINE_COORDINATES_GFP,
+              EC_R_POINT_IS_NOT_ON_CURVE);
+        return 0;
+    }
+    return 1;
 }
 
 #ifndef OPENSSL_NO_EC2M
@@ -718,7 +726,15 @@ int EC_POINT_set_affine_coordinates_GF2m(const EC_GROUP *group,
               EC_R_INCOMPATIBLE_OBJECTS);
         return 0;
     }
-    return group->meth->point_set_affine_coordinates(group, point, x, y, ctx);
+    if (!group->meth->point_set_affine_coordinates(group, point, x, y, ctx))
+        return 0;
+
+    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
+        ECerr(EC_F_EC_POINT_SET_AFFINE_COORDINATES_GF2M,
+              EC_R_POINT_IS_NOT_ON_CURVE);
+        return 0;
+    }
+    return 1;
 }
 #endif
 
diff --git a/crypto/ec/ecp_oct.c b/crypto/ec/ecp_oct.c
index 299f8c5a49..4d142a4ab9 100644
--- a/crypto/ec/ecp_oct.c
+++ b/crypto/ec/ecp_oct.c
@@ -355,16 +355,14 @@ int ec_GFp_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
             }
         }
 
+        /*
+         * EC_POINT_set_affine_coordinates_GFp is responsible for checking that
+         * the point is on the curve.
+         */
         if (!EC_POINT_set_affine_coordinates_GFp(group, point, x, y, ctx))
             goto err;
     }
 
-    /* test required by X9.62 */
-    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
-        ECerr(EC_F_EC_GFP_SIMPLE_OCT2POINT, EC_R_POINT_IS_NOT_ON_CURVE);
-        goto err;
-    }
-
     ret = 1;
 
  err: