Move point-on-curve check to set_affine_coordinates

Bad API design makes it possible to set an EC_KEY public key to
a point not on the curve. As a consequence, it was possible to
have bogus ECDSA signatures validated. In practice, all software
uses either EC_POINT_oct2point*() to unmarshal public keys or
issues a call to EC_KEY_check_key() after setting it. This way,
a point on curve check is performed and the problem is mitigated.

In OpenSSL commit 1e2012b7ff4a5f12273446b281775faa5c8a1858, Emilia
Kasper moved the point-on-curve check from EC_POINT_oct2point to
EC_POINT_set_affine_coordinates_*, which results in more checking.
In addition to this commit, we also check in the currently unused
codepath of a user set callback for setting compressed coordinates,
just in case this will be used at some point in the future.

The documentation of EC_KEY_check_key() is very vague on what it
checks and when checks are needed.  It could certainly be improved
a lot. It's also strange that EC_KEY_set_key() performs no checks,
while EC_KEY_set_public_key_affine_coordinates() implicitly calls
EC_KEY_check_key().

It's a mess.

Issue found and reported by Guido Vranken who also tested an earlier
version of this fix.

ok jsing

4 files changed, 50 insertions, 18 deletions

diff --git a/src/lib/libcrypto/ec/ec2_oct.c b/src/lib/libcrypto/ec/ec2_oct.c
index 268eccf471..5f7f7e3c99 100644
--- a/src/lib/libcrypto/ec/ec2_oct.c
+++ b/src/lib/libcrypto/ec/ec2_oct.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec2_oct.c,v 1.11 2018/07/15 16:27:39 tb Exp $ */
+/* $OpenBSD: ec2_oct.c,v 1.12 2020/12/04 08:55:30 tb Exp $ */
 /* ====================================================================
  * Copyright 2002 Sun Microsystems, Inc. ALL RIGHTS RESERVED.
  *
@@ -346,6 +346,10 @@ ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
                 goto err;
         }
         if (form == POINT_CONVERSION_COMPRESSED) {
+                /*
+                 * EC_POINT_set_compressed_coordinates_GF2m checks that the
+                 * point is on the curve as required by X9.62.
+                 */
                 if (!EC_POINT_set_compressed_coordinates_GF2m(group, point, x, y_bit, ctx))
                         goto err;
         } else {
@@ -363,15 +367,14 @@ ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
                                 goto err;
                         }
                 }
+                /*
+                 * EC_POINT_set_affine_coordinates_GF2m checks that the
+                 * point is on the curve as required by X9.62.
+                 */
                 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) {
-                ECerror(EC_R_POINT_IS_NOT_ON_CURVE);
-                goto err;
-        }
         ret = 1;
 
  err:
diff --git a/src/lib/libcrypto/ec/ec_lib.c b/src/lib/libcrypto/ec/ec_lib.c
index df9061627e..3442c7a324 100644
--- a/src/lib/libcrypto/ec/ec_lib.c
+++ b/src/lib/libcrypto/ec/ec_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec_lib.c,v 1.32 2019/09/29 10:09:09 tb Exp $ */
+/* $OpenBSD: ec_lib.c,v 1.33 2020/12/04 08:55:30 tb Exp $ */
 /*
  * Originally written by Bodo Moeller for the OpenSSL project.
  */
@@ -964,7 +964,13 @@ EC_POINT_set_affine_coordinates_GFp(const EC_GROUP *group, EC_POINT *point,
                 ECerror(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) {
+                ECerror(EC_R_POINT_IS_NOT_ON_CURVE);
+                return 0;
+        }
+        return 1;
 }
 
 #ifndef OPENSSL_NO_EC2M
@@ -980,7 +986,13 @@ EC_POINT_set_affine_coordinates_GF2m(const EC_GROUP *group, EC_POINT *point,
                 ECerror(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) {
+                ECerror(EC_R_POINT_IS_NOT_ON_CURVE);
+                return 0;
+        }
+        return 1;
 }
 #endif
 
diff --git a/src/lib/libcrypto/ec/ec_oct.c b/src/lib/libcrypto/ec/ec_oct.c
index f44b174fd7..a285c81459 100644
--- a/src/lib/libcrypto/ec/ec_oct.c
+++ b/src/lib/libcrypto/ec/ec_oct.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec_oct.c,v 1.5 2017/01/29 17:49:23 beck Exp $ */
+/* $OpenBSD: ec_oct.c,v 1.6 2020/12/04 08:55:30 tb Exp $ */
 /*
  * Originally written by Bodo Moeller for the OpenSSL project.
  */
@@ -98,7 +98,14 @@ EC_POINT_set_compressed_coordinates_GFp(const EC_GROUP * group, EC_POINT * point
                             group, point, x, y_bit, ctx);
 #endif
         }
-        return group->meth->point_set_compressed_coordinates(group, point, x, y_bit, ctx);
+        if (!group->meth->point_set_compressed_coordinates(group, point, x,
+            y_bit, ctx))
+                return 0;
+        if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
+                ECerror(EC_R_POINT_IS_NOT_ON_CURVE);
+                return 0;
+        }
+        return 1;
 }
 
 #ifndef OPENSSL_NO_EC2M
@@ -123,7 +130,14 @@ EC_POINT_set_compressed_coordinates_GF2m(const EC_GROUP * group, EC_POINT * poin
                         return ec_GF2m_simple_set_compressed_coordinates(
                             group, point, x, y_bit, ctx);
         }
-        return group->meth->point_set_compressed_coordinates(group, point, x, y_bit, ctx);
+        if (!group->meth->point_set_compressed_coordinates(group, point, x,
+            y_bit, ctx))
+                return 0;
+        if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
+                ECerror(EC_R_POINT_IS_NOT_ON_CURVE);
+                return 0;
+        }
+        return 1;
 }
 #endif
 
diff --git a/src/lib/libcrypto/ec/ecp_oct.c b/src/lib/libcrypto/ec/ecp_oct.c
index 90c5ca2e4e..29d9990546 100644
--- a/src/lib/libcrypto/ec/ecp_oct.c
+++ b/src/lib/libcrypto/ec/ecp_oct.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ecp_oct.c,v 1.11 2018/07/15 16:27:39 tb Exp $ */
+/* $OpenBSD: ecp_oct.c,v 1.12 2020/12/04 08:55:30 tb Exp $ */
 /* Includes code written by Lenka Fibikova <fibikova@exp-math.uni-essen.de>
  * for the OpenSSL project.
  * Includes code written by Bodo Moeller for the OpenSSL project.
@@ -362,6 +362,10 @@ ec_GFp_simple_oct2point(const EC_GROUP * group, EC_POINT * point,
                 goto err;
         }
         if (form == POINT_CONVERSION_COMPRESSED) {
+                /*
+                 * EC_POINT_set_compressed_coordinates_GFp checks that the point
+                 * is on the curve as required by X9.62.
+                 */
                 if (!EC_POINT_set_compressed_coordinates_GFp(group, point, x, y_bit, ctx))
                         goto err;
         } else {
@@ -377,15 +381,14 @@ ec_GFp_simple_oct2point(const EC_GROUP * group, EC_POINT * point,
                                 goto err;
                         }
                 }
+                /*
+                 * EC_POINT_set_affine_coordinates_GFp checks that the point is
+                 * on the curve as required by X9.62.
+                 */
                 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) {
-                ECerror(EC_R_POINT_IS_NOT_ON_CURVE);
-                goto err;
-        }
         ret = 1;
 
  err: