From c983c81a239d44b53a32f794ee77a86f7f8d9e38 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 1 Feb 2019 16:41:30 +0000 Subject: [PATCH 1/6] Fix 1-byte buffer overflow in mbedtls_mpi_write_string() This can only occur for negative numbers. Fixes #2404. --- library/bignum.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/bignum.c b/library/bignum.c index 87015af0c8..23bcca92c9 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -602,7 +602,10 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix, mbedtls_mpi_init( &T ); if( X->s == -1 ) + { *p++ = '-'; + buflen--; + } if( radix == 16 ) { From f5e2861958fe55dffa4aa78927abd212cb185404 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 1 Feb 2019 16:42:48 +0000 Subject: [PATCH 2/6] Adapt ChangeLog --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 5c2fbbbd4f..b59e31528e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,8 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.x.x branch released xxxx-xx-xx Bugfix + * Fix 1-byte buffer overflow in mbedtls_mpi_write_string() when + used with negative inputs. Found by Guido Vranken in #2404. * Fix a compilation issue with mbedtls_ecp_restart_ctx not being defined when MBEDTLS_ECP_ALT is defined. Reported by jwhui. Fixes #2242. * Run the AD too long test only if MBEDTLS_CCM_ALT is not defined. From 23cfea01e8a38878bc3f6d273e3588ce35819e4b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 4 Feb 2019 09:45:07 +0000 Subject: [PATCH 3/6] Improve documentation of mbedtls_mpi_write_string() --- library/bignum.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 23bcca92c9..50b75be6ab 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -582,15 +582,20 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix, if( radix < 2 || radix > 16 ) return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); - n = mbedtls_mpi_bitlen( X ); - if( radix >= 4 ) n >>= 1; - if( radix >= 16 ) n >>= 1; - /* - * Round up the buffer length to an even value to ensure that there is - * enough room for hexadecimal values that can be represented in an odd - * number of digits. - */ - n += 3 + ( ( n + 1 ) & 1 ); + n = mbedtls_mpi_bitlen( X ); /* Number of bits necessary to present `n`. */ + if( radix >= 4 ) n >>= 1; /* Number of 4-adic digits necessary to present + * `n`. If radix > 4, this might be a strict + * overapproximation of the number of + * radix-adic digits needed to present `n`. */ + if( radix >= 16 ) n >>= 1; /* Number of hexadecimal digits necessary to + * present `n`. */ + + n += 1; /* NULL termination */ + n += 1; /* Compensate for the divisions above, which round down `n` + * in case it's not even. */ + n += 1; /* Potential '-'-sign. */ + n += ( n & 1 ); /* Make n even to have enough space for hexadecimal writing, + * which always uses an even number of hex-digits. */ if( buflen < n ) { From 04dadb73fdbf9556b3cd3f72524d801bd91aa9cc Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 6 Mar 2019 12:29:37 +0000 Subject: [PATCH 4/6] Add non-regression test for buffer overflow --- tests/suites/test_suite_mpi.data | 3 +++ tests/suites/test_suite_mpi.function | 3 +++ 2 files changed, 6 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 8b5f97d389..425e93ad20 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -25,6 +25,9 @@ mpi_read_write_string:16:"-20":10:"-32":100:0:0 Base test mpi_read_write_string #3 (Negative decimal) mpi_read_write_string:16:"-23":16:"-23":100:0:0 +Base test mpi_read_write_string #4 (Buffer just fits) +mpi_read_write_string:16:"-4":4:"-10":4:0:0 + Test mpi_read_write_string #1 (Invalid character) mpi_read_write_string:10:"a28":0:"":100:MBEDTLS_ERR_MPI_INVALID_CHARACTER:0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index d1fa5a46cf..f982385e13 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -294,6 +294,8 @@ void mpi_read_write_string( int radix_X, char * input_X, int radix_A, mbedtls_mpi_init( &X ); + memset( str, '!', sizeof( str ) ); + TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == result_read ); if( result_read == 0 ) { @@ -301,6 +303,7 @@ void mpi_read_write_string( int radix_X, char * input_X, int radix_A, if( result_write == 0 ) { TEST_ASSERT( strcasecmp( str, input_A ) == 0 ); + TEST_ASSERT( str[len] == '!' ); } } From 80470627e2e7b9310eefbaa3aafa10930f64bd51 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 6 Mar 2019 13:43:02 +0000 Subject: [PATCH 5/6] Fix typo --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 50b75be6ab..02086c04b4 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -590,7 +590,7 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix, if( radix >= 16 ) n >>= 1; /* Number of hexadecimal digits necessary to * present `n`. */ - n += 1; /* NULL termination */ + n += 1; /* Terminating null byte */ n += 1; /* Compensate for the divisions above, which round down `n` * in case it's not even. */ n += 1; /* Potential '-'-sign. */ From b6a59f66cdea97465d04be7a3cb1367770c97dd1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 6 Mar 2019 16:29:37 +0000 Subject: [PATCH 6/6] Fix ChangeLog entry ordering --- ChangeLog | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index b59e31528e..8f989eb190 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,8 +3,6 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.x.x branch released xxxx-xx-xx Bugfix - * Fix 1-byte buffer overflow in mbedtls_mpi_write_string() when - used with negative inputs. Found by Guido Vranken in #2404. * Fix a compilation issue with mbedtls_ecp_restart_ctx not being defined when MBEDTLS_ECP_ALT is defined. Reported by jwhui. Fixes #2242. * Run the AD too long test only if MBEDTLS_CCM_ALT is not defined. @@ -21,6 +19,8 @@ Bugfix in X.509 module. Fixes #2212. * Reduce stack usage of `mpi_write_hlp()` by eliminating recursion. Fixes #2190. + * Fix 1-byte buffer overflow in mbedtls_mpi_write_string() when + used with negative inputs. Found by Guido Vranken in #2404. Changes * Include configuration file in all header files that use configuration,