From 6110a16555bcb185c825fbfcccaca200a1c98c95 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Nov 2022 21:22:27 +0100 Subject: [PATCH 1/4] Document mbedtls_mpi_uint and mbedtls_mpi_sint Since they're part of the public API (even if only through a few functions), they should be documented. I deliberately skipped documenting how to configure the size of the type. Right now, MBEDTLS_HAVE_INT32 and MBEDTLS_HAVE_INT64 have no Doxygen documentation, so it's ambiguous whether they're part of the public API. Resolving this ambiguity is out of scope of my current work. Signed-off-by: Gilles Peskine --- include/mbedtls/bignum.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 9d15955f34..5800d4acba 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -179,6 +179,20 @@ #endif /* !MBEDTLS_NO_UDBL_DIVISION */ #endif /* !MBEDTLS_HAVE_INT64 */ +/** \typedef mbedtls_mpi_uint + * \brief The type of machine digits in a bignum, called _limbs_. + * + * This is always an unsigned integer type with no padding bits. The size + * is platform-dependent. + */ + +/** \typedef mbedtls_mpi_sint + * \brief The signed type corresponding to #mbedtls_mpi_uint. + * + * This is always an signed integer type with no padding bits. The size + * is platform-dependent. + */ + #ifdef __cplusplus extern "C" { #endif From db14a9d180069e8a6cb63455e75463aad9f68889 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Nov 2022 22:59:00 +0100 Subject: [PATCH 2/4] Fix NULL+0 in addition 0 + 0 Fix undefined behavior (typically harmless in practice) of mbedtls_mpi_add_mpi(), mbedtls_mpi_add_abs() and mbedtls_mpi_add_int() when both operands are 0 and the left operand is represented with 0 limbs. Signed-off-by: Gilles Peskine --- ChangeLog.d/mpi-add-0-ub.txt | 4 ++++ library/bignum.c | 5 +++++ 2 files changed, 9 insertions(+) create mode 100644 ChangeLog.d/mpi-add-0-ub.txt diff --git a/ChangeLog.d/mpi-add-0-ub.txt b/ChangeLog.d/mpi-add-0-ub.txt new file mode 100644 index 0000000000..9f131a4300 --- /dev/null +++ b/ChangeLog.d/mpi-add-0-ub.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix undefined behavior (typically harmless in practice) of + mbedtls_mpi_add_mpi(), mbedtls_mpi_add_abs() and mbedtls_mpi_add_int() + when both operands are 0 and the left operand is represented with 0 limbs. diff --git a/library/bignum.c b/library/bignum.c index 521787d749..497ccbc817 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -889,6 +889,11 @@ int mbedtls_mpi_add_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi if( B->p[j - 1] != 0 ) break; + /* Exit early to avoid undefined behavior on NULL+0 when X->n == 0 + * and B is 0 (of any size). */ + if( j == 0 ) + return( 0 ); + MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, j ) ); /* j is the number of non-zero limbs of B. Add those to X. */ From af601f97519f90cc4b669984dd3fcf215b8b2792 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Nov 2022 23:02:14 +0100 Subject: [PATCH 3/4] Fix undefined behavior with the most negative mbedtls_mpi_sint When x is the most negative value of a two's complement type, `(unsigned_type)(-x)` has undefined behavior, whereas `-(unsigned_type)x` has well-defined behavior and does what was intended. Signed-off-by: Gilles Peskine --- ChangeLog.d/mpi-most-negative-sint.txt | 4 + library/bignum.c | 10 +- tests/suites/test_suite_bignum.function | 144 +++++++++++++++++++++++ tests/suites/test_suite_bignum.misc.data | 3 + 4 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 ChangeLog.d/mpi-most-negative-sint.txt diff --git a/ChangeLog.d/mpi-most-negative-sint.txt b/ChangeLog.d/mpi-most-negative-sint.txt new file mode 100644 index 0000000000..5e775c4825 --- /dev/null +++ b/ChangeLog.d/mpi-most-negative-sint.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix undefined behavior (typically harmless in practice) when some bignum + functions receive the most negative value of mbedtls_mpi_sint. Credit + to OSS-Fuzz. Fixes #6597. diff --git a/library/bignum.c b/library/bignum.c index 497ccbc817..04aca69e80 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -263,7 +263,7 @@ int mbedtls_mpi_lset( mbedtls_mpi *X, mbedtls_mpi_sint z ) MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, 1 ) ); memset( X->p, 0, X->n * ciL ); - X->p[0] = ( z < 0 ) ? -z : z; + X->p[0] = ( z < 0 ) ? -(mbedtls_mpi_uint)z : z; X->s = ( z < 0 ) ? -1 : 1; cleanup: @@ -853,7 +853,7 @@ int mbedtls_mpi_cmp_int( const mbedtls_mpi *X, mbedtls_mpi_sint z ) mbedtls_mpi_uint p[1]; MPI_VALIDATE_RET( X != NULL ); - *p = ( z < 0 ) ? -z : z; + *p = ( z < 0 ) ? -(mbedtls_mpi_uint)z : z; Y.s = ( z < 0 ) ? -1 : 1; Y.n = 1; Y.p = p; @@ -1057,7 +1057,7 @@ int mbedtls_mpi_add_int( mbedtls_mpi *X, const mbedtls_mpi *A, mbedtls_mpi_sint MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); - p[0] = ( b < 0 ) ? -b : b; + p[0] = ( b < 0 ) ? -(mbedtls_mpi_uint)b : b; B.s = ( b < 0 ) ? -1 : 1; B.n = 1; B.p = p; @@ -1075,7 +1075,7 @@ int mbedtls_mpi_sub_int( mbedtls_mpi *X, const mbedtls_mpi *A, mbedtls_mpi_sint MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); - p[0] = ( b < 0 ) ? -b : b; + p[0] = ( b < 0 ) ? -(mbedtls_mpi_uint)b : b; B.s = ( b < 0 ) ? -1 : 1; B.n = 1; B.p = p; @@ -1413,7 +1413,7 @@ int mbedtls_mpi_div_int( mbedtls_mpi *Q, mbedtls_mpi *R, mbedtls_mpi_uint p[1]; MPI_VALIDATE_RET( A != NULL ); - p[0] = ( b < 0 ) ? -b : b; + p[0] = ( b < 0 ) ? -(mbedtls_mpi_uint)b : b; B.s = ( b < 0 ) ? -1 : 1; B.n = 1; B.p = p; diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 5c3d776f09..3238467598 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -1447,6 +1447,150 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void most_negative_mpi_sint( ) +{ + /* Ad hoc tests for n = -p = -2^(biL-1) as a mbedtls_mpi_sint. We + * guarantee that mbedtls_mpi_sint is a two's complement type, so this + * is a valid value. However, negating it (`-n`) has undefined behavior + * (although in practice `-n` evaluates to the value n). + * + * This function has ad hoc tests for this value. It's separated from other + * functions because the test framework makes it hard to pass this value + * into test cases. + * + * In the comments here: + * - biL = number of bits in limbs + * - p = 2^(biL-1) (smallest positive value not in mbedtls_mpi_sint range) + * - n = -2^(biL-1) (largest negative value in mbedtls_mpi_sint range) + */ + + mbedtls_mpi A, R, X; + mbedtls_mpi_init( &A ); + mbedtls_mpi_init( &R ); + mbedtls_mpi_init( &X ); + + const size_t biL = 8 * sizeof( mbedtls_mpi_sint ); + mbedtls_mpi_uint most_positive_plus_1 = (mbedtls_mpi_uint) 1 << ( biL - 1 ); + const mbedtls_mpi_sint most_positive = most_positive_plus_1 - 1; + const mbedtls_mpi_sint most_negative = - most_positive - 1; + TEST_EQUAL( (mbedtls_mpi_uint) most_negative, + (mbedtls_mpi_uint) 1 << ( biL - 1 ) ); + TEST_EQUAL( (mbedtls_mpi_uint) most_negative << 1, 0 ); + + /* Test mbedtls_mpi_lset() */ + TEST_EQUAL( mbedtls_mpi_lset( &A, most_negative ), 0 ); + TEST_EQUAL( A.s, -1 ); + TEST_EQUAL( A.n, 1 ); + TEST_EQUAL( A.p[0], most_positive_plus_1 ); + + /* Test mbedtls_mpi_cmp_int(): -p == -p */ + TEST_EQUAL( mbedtls_mpi_cmp_int( &A, most_negative ), 0 ); + + /* Test mbedtls_mpi_cmp_int(): -(p+1) < -p */ + A.p[0] = most_positive_plus_1 + 1; + TEST_EQUAL( mbedtls_mpi_cmp_int( &A, most_negative ), -1 ); + + /* Test mbedtls_mpi_cmp_int(): -(p-1) > -p */ + A.p[0] = most_positive_plus_1 - 1; + TEST_EQUAL( mbedtls_mpi_cmp_int( &A, most_negative ), 1 ); + + /* Test mbedtls_mpi_add_int(): (p-1) + (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, most_positive ), 0 ); + TEST_EQUAL( mbedtls_mpi_add_int( &X, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, -1 ), 0 ); + + /* Test mbedtls_mpi_add_int(): (0) + (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, 0 ), 0 ); + TEST_EQUAL( mbedtls_mpi_add_int( &X, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, most_negative ), 0 ); + + /* Test mbedtls_mpi_add_int(): (-p) + (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_add_int( &X, &A, most_negative ), 0 ); + TEST_EQUAL( X.s, -1 ); + TEST_EQUAL( X.n, 2 ); + TEST_EQUAL( X.p[0], 0 ); + TEST_EQUAL( X.p[1], 1 ); + + /* Test mbedtls_mpi_sub_int(): (p) - (-p) */ + mbedtls_mpi_free( &X ); + TEST_EQUAL( mbedtls_mpi_lset( &A, most_positive ), 0 ); + TEST_EQUAL( mbedtls_mpi_sub_int( &X, &A, most_negative ), 0 ); + TEST_EQUAL( X.s, 1 ); + TEST_EQUAL( X.n, 1 ); + TEST_EQUAL( X.p[0], ~(mbedtls_mpi_uint)0 ); + + /* Test mbedtls_mpi_sub_int(): (0) - (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, 0 ), 0 ); + TEST_EQUAL( mbedtls_mpi_sub_int( &X, &A, most_negative ), 0 ); + TEST_EQUAL( X.s, 1 ); + TEST_EQUAL( X.n, 1 ); + TEST_EQUAL( X.p[0], most_positive_plus_1 ); + + /* Test mbedtls_mpi_sub_int(): (-p) - (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_sub_int( &X, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, 0 ), 0 ); + + /* Test mbedtls_mpi_div_int(): (-p+1) / (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, -most_positive ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, 0 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, -most_positive ), 0 ); + + /* Test mbedtls_mpi_div_int(): (-p) / (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, 1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, 0 ), 0 ); + + /* Test mbedtls_mpi_div_int(): (-2*p) / (-p) */ + TEST_EQUAL( mbedtls_mpi_shift_l( &A, 1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, 2 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, 0 ), 0 ); + + /* Test mbedtls_mpi_div_int(): (-2*p+1) / (-p) */ + TEST_EQUAL( mbedtls_mpi_add_int( &A, &A, 1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, 1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, -most_positive ), 0 ); + + /* Test mbedtls_mpi_div_int(): (p-1) / (-p) */ + TEST_EQUAL( mbedtls_mpi_lset( &A, most_positive ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, 0 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, most_positive ), 0 ); + + /* Test mbedtls_mpi_div_int(): (p) / (-p) */ + TEST_EQUAL( mbedtls_mpi_add_int( &A, &A, 1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, -1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, 0 ), 0 ); + + /* Test mbedtls_mpi_div_int(): (2*p) / (-p) */ + TEST_EQUAL( mbedtls_mpi_shift_l( &A, 1 ), 0 ); + TEST_EQUAL( mbedtls_mpi_div_int( &X, &R, &A, most_negative ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &X, -2 ), 0 ); + TEST_EQUAL( mbedtls_mpi_cmp_int( &R, 0 ), 0 ); + + /* Test mbedtls_mpi_mod_int(): never valid */ + TEST_EQUAL( mbedtls_mpi_mod_int( X.p, &A, most_negative ), + MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); + + /* Test mbedtls_mpi_random(): never valid */ + TEST_EQUAL( mbedtls_mpi_random( &X, most_negative, &A, + mbedtls_test_rnd_std_rand, NULL ), + MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + +exit: + mbedtls_mpi_free( &A ); + mbedtls_mpi_free( &R ); + mbedtls_mpi_free( &X ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void mpi_selftest( ) { diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data index 0b8aa334ac..7aaacbe73d 100644 --- a/tests/suites/test_suite_bignum.misc.data +++ b/tests/suites/test_suite_bignum.misc.data @@ -1934,6 +1934,9 @@ mpi_random_fail:2:"01":MBEDTLS_ERR_MPI_BAD_INPUT_DATA MPI random bad arguments: min > N = 1, 0 limb in upper bound mpi_random_fail:2:"000000000000000001":MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Most negative mbedtls_mpi_sint +most_negative_mpi_sint: + MPI Selftest depends_on:MBEDTLS_SELF_TEST mpi_selftest: From ef7f4e47b183549784239d2d5bd3bb5c856e93c6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Nov 2022 23:25:27 +0100 Subject: [PATCH 4/4] Express abs(z) in a way that satisfies GCC and MSVC Signed-off-by: Gilles Peskine --- library/bignum.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 04aca69e80..53a9aa5e71 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -252,6 +252,17 @@ void mbedtls_mpi_swap( mbedtls_mpi *X, mbedtls_mpi *Y ) memcpy( Y, &T, sizeof( mbedtls_mpi ) ); } +static inline mbedtls_mpi_uint mpi_sint_abs( mbedtls_mpi_sint z ) +{ + if( z >= 0 ) + return( z ); + /* Take care to handle the most negative value (-2^(biL-1)) correctly. + * A naive -z would have undefined behavior. + * Write this in a way that makes popular compilers happy (GCC, Clang, + * MSVC). */ + return( (mbedtls_mpi_uint) 0 - (mbedtls_mpi_uint) z ); +} + /* * Set value from integer */ @@ -263,7 +274,7 @@ int mbedtls_mpi_lset( mbedtls_mpi *X, mbedtls_mpi_sint z ) MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, 1 ) ); memset( X->p, 0, X->n * ciL ); - X->p[0] = ( z < 0 ) ? -(mbedtls_mpi_uint)z : z; + X->p[0] = mpi_sint_abs( z ); X->s = ( z < 0 ) ? -1 : 1; cleanup: @@ -853,7 +864,7 @@ int mbedtls_mpi_cmp_int( const mbedtls_mpi *X, mbedtls_mpi_sint z ) mbedtls_mpi_uint p[1]; MPI_VALIDATE_RET( X != NULL ); - *p = ( z < 0 ) ? -(mbedtls_mpi_uint)z : z; + *p = mpi_sint_abs( z ); Y.s = ( z < 0 ) ? -1 : 1; Y.n = 1; Y.p = p; @@ -1057,7 +1068,7 @@ int mbedtls_mpi_add_int( mbedtls_mpi *X, const mbedtls_mpi *A, mbedtls_mpi_sint MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); - p[0] = ( b < 0 ) ? -(mbedtls_mpi_uint)b : b; + p[0] = mpi_sint_abs( b ); B.s = ( b < 0 ) ? -1 : 1; B.n = 1; B.p = p; @@ -1075,7 +1086,7 @@ int mbedtls_mpi_sub_int( mbedtls_mpi *X, const mbedtls_mpi *A, mbedtls_mpi_sint MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); - p[0] = ( b < 0 ) ? -(mbedtls_mpi_uint)b : b; + p[0] = mpi_sint_abs( b ); B.s = ( b < 0 ) ? -1 : 1; B.n = 1; B.p = p; @@ -1413,7 +1424,7 @@ int mbedtls_mpi_div_int( mbedtls_mpi *Q, mbedtls_mpi *R, mbedtls_mpi_uint p[1]; MPI_VALIDATE_RET( A != NULL ); - p[0] = ( b < 0 ) ? -(mbedtls_mpi_uint)b : b; + p[0] = mpi_sint_abs( b ); B.s = ( b < 0 ) ? -1 : 1; B.n = 1; B.p = p;