Make it explicit that the EC_KEY setters don't check things
authortb <tb@openbsd.org>
Fri, 16 Feb 2024 06:09:36 +0000 (06:09 +0000)
committertb <tb@openbsd.org>
Fri, 16 Feb 2024 06:09:36 +0000 (06:09 +0000)
commit19619287f1f0d0217f654c44bd972f72836ef04e
tree42b8d6163f25f4bcd7061d0f189ec1e4382c5432
parent23c6f3a22705efceb42c611c5e178273d2526022
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