Merge remote-tracking branch 'origin/pr/2504' into mbedtls-2.7
* origin/pr/2504:
Fix ChangeLog entry ordering
Fix typo
Add non-regression test for buffer overflow
Improve documentation of mbedtls_mpi_write_string()
Adapt ChangeLog
Fix 1-byte buffer overflow in mbedtls_mpi_write_string()
diff --git a/ChangeLog b/ChangeLog
index a7b8c89..9d963c0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -23,6 +23,8 @@
Christian Walther in #2239.
* Fix potential memory leak in X.509 self test. Found and fixed by
Junhwan Park, #2106.
+ * Fix 1-byte buffer overflow in mbedtls_mpi_write_string() when
+ used with negative inputs. Found by Guido Vranken in #2404.
Changes
* Return from various debugging routines immediately if the
diff --git a/library/bignum.c b/library/bignum.c
index f6e50b9..d142fe6 100644
--- a/library/bignum.c
+++ b/library/bignum.c
@@ -552,15 +552,20 @@
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; /* Terminating null byte */
+ 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 )
{
@@ -572,7 +577,10 @@
mbedtls_mpi_init( &T );
if( X->s == -1 )
+ {
*p++ = '-';
+ buflen--;
+ }
if( radix == 16 )
{
diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data
index 2960641..b8d7ad1 100644
--- a/tests/suites/test_suite_mpi.data
+++ b/tests/suites/test_suite_mpi.data
@@ -19,6 +19,9 @@
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 04dca0f..aa3c332 100644
--- a/tests/suites/test_suite_mpi.function
+++ b/tests/suites/test_suite_mpi.function
@@ -81,6 +81,8 @@
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 )
{
@@ -88,6 +90,7 @@
if( result_write == 0 )
{
TEST_ASSERT( strcasecmp( str, input_A ) == 0 );
+ TEST_ASSERT( str[len] == '!' );
}
}