From c0d3e4d32d2805f49bec30547f225bc4d092e1f4 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Thu, 19 Sep 2024 01:02:40 +1000 Subject: [PATCH] Harden BN_GF2m_poly2arr against misuse. The BN_GF2m_poly2arr() function converts characteristic-2 field (GF_{2^m}) Galois polynomials from a representation as a BIGNUM bitmask, to a compact array with just the exponents of the non-zero terms. These polynomials are then used in BN_GF2m_mod_arr() to perform modular reduction. A precondition of calling BN_GF2m_mod_arr() is that the polynomial must have a non-zero constant term (i.e. the array has `0` as its final element). Internally, callers of BN_GF2m_poly2arr() did not verify that precondition, and binary EC curve parameters with an invalid polynomial could lead to out of bounds memory reads and writes in BN_GF2m_mod_arr(). The precondition is always true for polynomials that arise from the standard form of EC parameters for characteristic-two fields (X9.62). See the "Finite Field Identification" section of: https://www.itu.int/ITU-T/formal-language/itu-t/x/x894/2018-cor1/ANSI-X9-62.html The OpenSSL GF(2^m) code supports only the trinomial and pentanomial basis X9.62 forms. This commit updates BN_GF2m_poly2arr() to return `0` (failure) when the constant term is zero (i.e. the input bitmask BIGNUM is not odd). Additionally, the return value is made unambiguous when there is not enough space to also pad the array with a final `-1` sentinel value. The return value is now always the number of elements (including the final `-1`) that would be filled when the output array is sufficiently large. Previously the same count was returned both when the array has just enough room for the final `-1` and when it had only enough space for non-sentinel values. Finally, BN_GF2m_poly2arr() is updated to reject polynomials whose degree exceeds `OPENSSL_ECC_MAX_FIELD_BITS`, this guards against CPU exhausition attacks via excessively large inputs. The above issues do not arise in processing X.509 certificates. These generally have EC keys from "named curves", and RFC5840 (Section 2.1.1) disallows explicit EC parameters. The TLS code in OpenSSL enforces this constraint only after the certificate is decoded, but, even if explicit parameters are specified, they are in X9.62 form, which cannot represent problem values as noted above. Initially reported as oss-fuzz issue 71623. A closely related issue was earlier reported in . Severity: Low, CVE-2024-9143 Reviewed-by: Matt Caswell Reviewed-by: Bernd Edlinger Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/25639) (cherry picked from commit 8e008cb8b23ec7dc75c45a66eeed09c815b11cd2) --- crypto/bn/bn_gf2m.c | 28 +++++++++++++++------- test/ec_internal_test.c | 51 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/crypto/bn/bn_gf2m.c b/crypto/bn/bn_gf2m.c index 444c5ca7a3755..ae7e9d751c29c 100644 --- crypto/bn/bn_gf2m.c.orig +++ crypto/bn/bn_gf2m.c @@ -15,6 +15,7 @@ #include "bn_local.h" #ifndef OPENSSL_NO_EC2M +# include /* * Maximum number of iterations before BN_GF2m_mod_solve_quad_arr should @@ -1130,16 +1131,26 @@ int BN_GF2m_mod_solve_quad(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, /* * Convert the bit-string representation of a polynomial ( \sum_{i=0}^n a_i * * x^i) into an array of integers corresponding to the bits with non-zero - * coefficient. Array is terminated with -1. Up to max elements of the array - * will be filled. Return value is total number of array elements that would - * be filled if array was large enough. + * coefficient. The array is intended to be suitable for use with + * `BN_GF2m_mod_arr()`, and so the constant term of the polynomial must not be + * zero. This translates to a requirement that the input BIGNUM `a` is odd. + * + * Given sufficient room, the array is terminated with -1. Up to max elements + * of the array will be filled. + * + * The return value is total number of array elements that would be filled if + * array was large enough, including the terminating `-1`. It is `0` when `a` + * is not odd or the constant term is zero contrary to requirement. + * + * The return value is also `0` when the leading exponent exceeds + * `OPENSSL_ECC_MAX_FIELD_BITS`, this guards against CPU exhaustion attacks, */ int BN_GF2m_poly2arr(const BIGNUM *a, int p[], int max) { int i, j, k = 0; BN_ULONG mask; - if (BN_is_zero(a)) + if (!BN_is_odd(a)) return 0; for (i = a->top - 1; i >= 0; i--) { @@ -1157,12 +1168,13 @@ int BN_GF2m_poly2arr(const BIGNUM *a, int p[], int max) } } - if (k < max) { + if (k > 0 && p[0] > OPENSSL_ECC_MAX_FIELD_BITS) + return 0; + + if (k < max) p[k] = -1; - k++; - } - return k; + return k + 1; } /* diff --git a/test/ec_internal_test.c b/test/ec_internal_test.c index 5076f9894d5b8..92904cfc42b20 100644 --- test/ec_internal_test.c.orig +++ test/ec_internal_test.c @@ -155,6 +155,56 @@ static int field_tests_ecp_mont(void) } #ifndef OPENSSL_NO_EC2M +/* Test that decoding of invalid GF2m field parameters fails. */ +static int ec2m_field_sanity(void) +{ + int ret = 0; + BN_CTX *ctx = BN_CTX_new(); + BIGNUM *p, *a, *b; + EC_GROUP *group1 = NULL, *group2 = NULL, *group3 = NULL; + + TEST_info("Testing GF2m hardening\n"); + + BN_CTX_start(ctx); + p = BN_CTX_get(ctx); + a = BN_CTX_get(ctx); + if (!TEST_ptr(b = BN_CTX_get(ctx)) + || !TEST_true(BN_one(a)) + || !TEST_true(BN_one(b))) + goto out; + + /* Even pentanomial value should be rejected */ + if (!TEST_true(BN_set_word(p, 0xf2))) + goto out; + if (!TEST_ptr_null(group1 = EC_GROUP_new_curve_GF2m(p, a, b, ctx))) + TEST_error("Zero constant term accepted in GF2m polynomial"); + + /* Odd hexanomial should also be rejected */ + if (!TEST_true(BN_set_word(p, 0xf3))) + goto out; + if (!TEST_ptr_null(group2 = EC_GROUP_new_curve_GF2m(p, a, b, ctx))) + TEST_error("Hexanomial accepted as GF2m polynomial"); + + /* Excessive polynomial degree should also be rejected */ + if (!TEST_true(BN_set_word(p, 0x71)) + || !TEST_true(BN_set_bit(p, OPENSSL_ECC_MAX_FIELD_BITS + 1))) + goto out; + if (!TEST_ptr_null(group3 = EC_GROUP_new_curve_GF2m(p, a, b, ctx))) + TEST_error("GF2m polynomial degree > %d accepted", + OPENSSL_ECC_MAX_FIELD_BITS); + + ret = group1 == NULL && group2 == NULL && group3 == NULL; + + out: + EC_GROUP_free(group1); + EC_GROUP_free(group2); + EC_GROUP_free(group3); + BN_CTX_end(ctx); + BN_CTX_free(ctx); + + return ret; +} + /* test EC_GF2m_simple_method directly */ static int field_tests_ec2_simple(void) { @@ -443,6 +493,7 @@ int setup_tests(void) ADD_TEST(field_tests_ecp_simple); ADD_TEST(field_tests_ecp_mont); #ifndef OPENSSL_NO_EC2M + ADD_TEST(ec2m_field_sanity); ADD_TEST(field_tests_ec2_simple); #endif ADD_ALL_TESTS(field_tests_default, crv_len);