From 19619287f1f0d0217f654c44bd972f72836ef04e Mon Sep 17 00:00:00 2001 From: tb Date: Fri, 16 Feb 2024 06:09:36 +0000 Subject: [PATCH] Make it explicit that the EC_KEY setters don't check things While EC_POINT_set_affine_coordinates() checks that the resulting point is on the elliptic curve, this is only necessary, but not sufficient, to ensure that the point can serve as a valid public key. For example, this does not check for normalized coordinates or exclude that it is zero (the point at infinity). Such checks, and more, are performed by the similarly named EC_KEY_set_public_key_affine_coordinates(). This kind of makes sense from the mathematical standpoint as an elliptic curve point isn't a priori a public key, even if you are not going to use libcrypto for actual mathematics (or anything really) unless you like pain. In a cryptographic library such differences are more of a hazard than a help. This is exacerbated by the fact that EC_KEY_set_public_key() does almost no checking (it only checks that the point's EC_POINT method matches the one of group set of the EC_KEY, which is far from enough). The API expects that you call EC_KEY_check_key() on your own. This is kind of confusing since EC_KEY_set_public_key_affine_coordinates() does that for you. Unfortunately, adding sanity checks to EC_KEY_set_public_key() isn't easy since it's going to penalize those who already check. Caching the result of a check is dangerous and fragile if there are a million ways of fiddling with an EC_KEY. While the elliptic curve code is really bad, its documentation is worse (another thing that applies to OpenSSL in general). Try to help that a little bit by making it more explicit that you are supposed to call EC_KEY_check_key() after using lower-level EC_KEY setters. Also make it clearer that the setters copy the data, they don't take ownership (which isn't obvious from the naming). If OpenSSL 3 got one thing kind of right, it was to deprecate the EC_KEY and EC_POINT APIs. But if you are going to deprecate something, you should either be prepared to remove it or have a reasonable replacement... Found by Guido Vranken using cryptofuzz https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=66667 ok jsing --- lib/libcrypto/man/EC_KEY_new.3 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/libcrypto/man/EC_KEY_new.3 b/lib/libcrypto/man/EC_KEY_new.3 index 06afdd537ce..f415b91d6fc 100644 --- a/lib/libcrypto/man/EC_KEY_new.3 +++ b/lib/libcrypto/man/EC_KEY_new.3 @@ -1,4 +1,4 @@ -.\" $OpenBSD: EC_KEY_new.3,v 1.18 2023/08/29 10:07:42 tb Exp $ +.\" $OpenBSD: EC_KEY_new.3,v 1.19 2024/02/16 06:09:36 tb Exp $ .\" full merge up to: OpenSSL 3aef36ff Jan 5 13:06:03 2016 -0500 .\" partial merge up to: OpenSSL e9b77246 Jan 20 19:58:49 2017 +0100 .\" @@ -49,7 +49,7 @@ .\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED .\" OF THE POSSIBILITY OF SUCH DAMAGE. .\" -.Dd $Mdocdate: August 29 2023 $ +.Dd $Mdocdate: February 16 2024 $ .Dt EC_KEY_NEW 3 .Os .Sh NAME @@ -324,6 +324,10 @@ object, the private key and the public key for the .Fa key , respectively. +The setters copy the group and key objects without sanity checks +and it is the caller's responsibility to ensure that +the resulting key is valid, for example using +.Fn EC_KEY_check_key . .Pp The functions .Fn EC_KEY_get_enc_flags -- 2.20.1