|
| 1 | +https://bugs.gentoo.org/941643 |
| 2 | +https://github.com/openssl/openssl/commit/bc7e04d7c8d509fb78fc0e285aa948fb0da04700 |
| 3 | + |
| 4 | +From bc7e04d7c8d509fb78fc0e285aa948fb0da04700 Mon Sep 17 00:00:00 2001 |
| 5 | +From: Viktor Dukhovni < [email protected]> |
| 6 | +Date: Thu, 19 Sep 2024 01:02:40 +1000 |
| 7 | +Subject: [PATCH] Harden BN_GF2m_poly2arr against misuse. |
| 8 | + |
| 9 | +The BN_GF2m_poly2arr() function converts characteristic-2 field |
| 10 | +(GF_{2^m}) Galois polynomials from a representation as a BIGNUM bitmask, |
| 11 | +to a compact array with just the exponents of the non-zero terms. |
| 12 | + |
| 13 | +These polynomials are then used in BN_GF2m_mod_arr() to perform modular |
| 14 | +reduction. A precondition of calling BN_GF2m_mod_arr() is that the |
| 15 | +polynomial must have a non-zero constant term (i.e. the array has `0` as |
| 16 | +its final element). |
| 17 | + |
| 18 | +Internally, callers of BN_GF2m_poly2arr() did not verify that |
| 19 | +precondition, and binary EC curve parameters with an invalid polynomial |
| 20 | +could lead to out of bounds memory reads and writes in BN_GF2m_mod_arr(). |
| 21 | + |
| 22 | +The precondition is always true for polynomials that arise from the |
| 23 | +standard form of EC parameters for characteristic-two fields (X9.62). |
| 24 | +See the "Finite Field Identification" section of: |
| 25 | + |
| 26 | + https://www.itu.int/ITU-T/formal-language/itu-t/x/x894/2018-cor1/ANSI-X9-62.html |
| 27 | + |
| 28 | +The OpenSSL GF(2^m) code supports only the trinomial and pentanomial |
| 29 | +basis X9.62 forms. |
| 30 | + |
| 31 | +This commit updates BN_GF2m_poly2arr() to return `0` (failure) when |
| 32 | +the constant term is zero (i.e. the input bitmask BIGNUM is not odd). |
| 33 | + |
| 34 | +Additionally, the return value is made unambiguous when there is not |
| 35 | +enough space to also pad the array with a final `-1` sentinel value. |
| 36 | +The return value is now always the number of elements (including the |
| 37 | +final `-1`) that would be filled when the output array is sufficiently |
| 38 | +large. Previously the same count was returned both when the array has |
| 39 | +just enough room for the final `-1` and when it had only enough space |
| 40 | +for non-sentinel values. |
| 41 | + |
| 42 | +Finally, BN_GF2m_poly2arr() is updated to reject polynomials whose |
| 43 | +degree exceeds `OPENSSL_ECC_MAX_FIELD_BITS`, this guards against |
| 44 | +CPU exhausition attacks via excessively large inputs. |
| 45 | + |
| 46 | +The above issues do not arise in processing X.509 certificates. These |
| 47 | +generally have EC keys from "named curves", and RFC5840 (Section 2.1.1) |
| 48 | +disallows explicit EC parameters. The TLS code in OpenSSL enforces this |
| 49 | +constraint only after the certificate is decoded, but, even if explicit |
| 50 | +parameters are specified, they are in X9.62 form, which cannot represent |
| 51 | +problem values as noted above. |
| 52 | + |
| 53 | +Initially reported as oss-fuzz issue 71623. |
| 54 | + |
| 55 | +A closely related issue was earlier reported in |
| 56 | +<https://github.com/openssl/openssl/issues/19826>. |
| 57 | + |
| 58 | +Severity: Low, CVE-2024-9143 |
| 59 | + |
| 60 | +Reviewed-by: Matt Caswell < [email protected]> |
| 61 | +Reviewed-by: Bernd Edlinger < [email protected]> |
| 62 | +Reviewed-by: Paul Dale < [email protected]> |
| 63 | +Reviewed-by: Tomas Mraz < [email protected]> |
| 64 | +(Merged from https://github.com/openssl/openssl/pull/25639) |
| 65 | + |
| 66 | +(cherry picked from commit 8e008cb8b23ec7dc75c45a66eeed09c815b11cd2) |
| 67 | +--- a/crypto/bn/bn_gf2m.c |
| 68 | ++++ b/crypto/bn/bn_gf2m.c |
| 69 | +@@ -15,6 +15,7 @@ |
| 70 | + #include "bn_local.h" |
| 71 | + |
| 72 | + #ifndef OPENSSL_NO_EC2M |
| 73 | ++# include <openssl/ec.h> |
| 74 | + |
| 75 | + /* |
| 76 | + * Maximum number of iterations before BN_GF2m_mod_solve_quad_arr should |
| 77 | +@@ -1130,16 +1131,26 @@ int BN_GF2m_mod_solve_quad(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, |
| 78 | + /* |
| 79 | + * Convert the bit-string representation of a polynomial ( \sum_{i=0}^n a_i * |
| 80 | + * x^i) into an array of integers corresponding to the bits with non-zero |
| 81 | +- * coefficient. Array is terminated with -1. Up to max elements of the array |
| 82 | +- * will be filled. Return value is total number of array elements that would |
| 83 | +- * be filled if array was large enough. |
| 84 | ++ * coefficient. The array is intended to be suitable for use with |
| 85 | ++ * `BN_GF2m_mod_arr()`, and so the constant term of the polynomial must not be |
| 86 | ++ * zero. This translates to a requirement that the input BIGNUM `a` is odd. |
| 87 | ++ * |
| 88 | ++ * Given sufficient room, the array is terminated with -1. Up to max elements |
| 89 | ++ * of the array will be filled. |
| 90 | ++ * |
| 91 | ++ * The return value is total number of array elements that would be filled if |
| 92 | ++ * array was large enough, including the terminating `-1`. It is `0` when `a` |
| 93 | ++ * is not odd or the constant term is zero contrary to requirement. |
| 94 | ++ * |
| 95 | ++ * The return value is also `0` when the leading exponent exceeds |
| 96 | ++ * `OPENSSL_ECC_MAX_FIELD_BITS`, this guards against CPU exhaustion attacks, |
| 97 | + */ |
| 98 | + int BN_GF2m_poly2arr(const BIGNUM *a, int p[], int max) |
| 99 | + { |
| 100 | + int i, j, k = 0; |
| 101 | + BN_ULONG mask; |
| 102 | + |
| 103 | +- if (BN_is_zero(a)) |
| 104 | ++ if (!BN_is_odd(a)) |
| 105 | + return 0; |
| 106 | + |
| 107 | + for (i = a->top - 1; i >= 0; i--) { |
| 108 | +@@ -1157,12 +1168,13 @@ int BN_GF2m_poly2arr(const BIGNUM *a, int p[], int max) |
| 109 | + } |
| 110 | + } |
| 111 | + |
| 112 | +- if (k < max) { |
| 113 | ++ if (k > 0 && p[0] > OPENSSL_ECC_MAX_FIELD_BITS) |
| 114 | ++ return 0; |
| 115 | ++ |
| 116 | ++ if (k < max) |
| 117 | + p[k] = -1; |
| 118 | +- k++; |
| 119 | +- } |
| 120 | + |
| 121 | +- return k; |
| 122 | ++ return k + 1; |
| 123 | + } |
| 124 | + |
| 125 | + /* |
| 126 | +--- a/test/ec_internal_test.c |
| 127 | ++++ b/test/ec_internal_test.c |
| 128 | +@@ -155,6 +155,56 @@ static int field_tests_ecp_mont(void) |
| 129 | + } |
| 130 | + |
| 131 | + #ifndef OPENSSL_NO_EC2M |
| 132 | ++/* Test that decoding of invalid GF2m field parameters fails. */ |
| 133 | ++static int ec2m_field_sanity(void) |
| 134 | ++{ |
| 135 | ++ int ret = 0; |
| 136 | ++ BN_CTX *ctx = BN_CTX_new(); |
| 137 | ++ BIGNUM *p, *a, *b; |
| 138 | ++ EC_GROUP *group1 = NULL, *group2 = NULL, *group3 = NULL; |
| 139 | ++ |
| 140 | ++ TEST_info("Testing GF2m hardening\n"); |
| 141 | ++ |
| 142 | ++ BN_CTX_start(ctx); |
| 143 | ++ p = BN_CTX_get(ctx); |
| 144 | ++ a = BN_CTX_get(ctx); |
| 145 | ++ if (!TEST_ptr(b = BN_CTX_get(ctx)) |
| 146 | ++ || !TEST_true(BN_one(a)) |
| 147 | ++ || !TEST_true(BN_one(b))) |
| 148 | ++ goto out; |
| 149 | ++ |
| 150 | ++ /* Even pentanomial value should be rejected */ |
| 151 | ++ if (!TEST_true(BN_set_word(p, 0xf2))) |
| 152 | ++ goto out; |
| 153 | ++ if (!TEST_ptr_null(group1 = EC_GROUP_new_curve_GF2m(p, a, b, ctx))) |
| 154 | ++ TEST_error("Zero constant term accepted in GF2m polynomial"); |
| 155 | ++ |
| 156 | ++ /* Odd hexanomial should also be rejected */ |
| 157 | ++ if (!TEST_true(BN_set_word(p, 0xf3))) |
| 158 | ++ goto out; |
| 159 | ++ if (!TEST_ptr_null(group2 = EC_GROUP_new_curve_GF2m(p, a, b, ctx))) |
| 160 | ++ TEST_error("Hexanomial accepted as GF2m polynomial"); |
| 161 | ++ |
| 162 | ++ /* Excessive polynomial degree should also be rejected */ |
| 163 | ++ if (!TEST_true(BN_set_word(p, 0x71)) |
| 164 | ++ || !TEST_true(BN_set_bit(p, OPENSSL_ECC_MAX_FIELD_BITS + 1))) |
| 165 | ++ goto out; |
| 166 | ++ if (!TEST_ptr_null(group3 = EC_GROUP_new_curve_GF2m(p, a, b, ctx))) |
| 167 | ++ TEST_error("GF2m polynomial degree > %d accepted", |
| 168 | ++ OPENSSL_ECC_MAX_FIELD_BITS); |
| 169 | ++ |
| 170 | ++ ret = group1 == NULL && group2 == NULL && group3 == NULL; |
| 171 | ++ |
| 172 | ++ out: |
| 173 | ++ EC_GROUP_free(group1); |
| 174 | ++ EC_GROUP_free(group2); |
| 175 | ++ EC_GROUP_free(group3); |
| 176 | ++ BN_CTX_end(ctx); |
| 177 | ++ BN_CTX_free(ctx); |
| 178 | ++ |
| 179 | ++ return ret; |
| 180 | ++} |
| 181 | ++ |
| 182 | + /* test EC_GF2m_simple_method directly */ |
| 183 | + static int field_tests_ec2_simple(void) |
| 184 | + { |
| 185 | +@@ -443,6 +493,7 @@ int setup_tests(void) |
| 186 | + ADD_TEST(field_tests_ecp_simple); |
| 187 | + ADD_TEST(field_tests_ecp_mont); |
| 188 | + #ifndef OPENSSL_NO_EC2M |
| 189 | ++ ADD_TEST(ec2m_field_sanity); |
| 190 | + ADD_TEST(field_tests_ec2_simple); |
| 191 | + #endif |
| 192 | + ADD_ALL_TESTS(field_tests_default, crv_len); |
| 193 | + |
0 commit comments