From eba5b622a3ad6c48a28a09d15ca32cdfac91f91b Mon Sep 17 00:00:00 2001 From: tb <> Date: Fri, 4 Dec 2020 08:55:30 +0000 Subject: 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 --- src/lib/libcrypto/ec/ec2_oct.c | 15 +++++++++------ src/lib/libcrypto/ec/ec_lib.c | 18 +++++++++++++++--- src/lib/libcrypto/ec/ec_oct.c | 20 +++++++++++++++++--- src/lib/libcrypto/ec/ecp_oct.c | 15 +++++++++------ 4 files changed, 50 insertions(+), 18 deletions(-) (limited to 'src/lib') 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 * 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: -- cgit v1.2.3-55-g6feb