Merge remote-tracking branch 'origin/pr/2405' into development
* origin/pr/2405:
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 e1588f5..b7ee43a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -13,6 +13,8 @@
* Fix potential memory leak in X.509 self test. Found and fixed by
Junhwan Park, #2106.
* Reduce stack usage of hkdf tests. Fixes #2195.
+ * Fix 1-byte buffer overflow in mbedtls_mpi_write_string() when
+ used with negative inputs. Found by Guido Vranken in #2404.
Changes
* Server's RSA certificate in certs.c was SHA-1 signed. In the default
diff --git a/library/bignum.c b/library/bignum.c
index 592aa2e..98ee12a 100644
--- a/library/bignum.c
+++ b/library/bignum.c
@@ -582,15 +582,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 )
{
@@ -602,7 +607,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 f2be148..3eebcff 100644
--- a/tests/suites/test_suite_mpi.data
+++ b/tests/suites/test_suite_mpi.data
@@ -25,6 +25,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 67894e6..eaae196 100644
--- a/tests/suites/test_suite_mpi.function
+++ b/tests/suites/test_suite_mpi.function
@@ -294,6 +294,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 )
{
@@ -301,6 +303,7 @@
if( result_write == 0 )
{
TEST_ASSERT( strcasecmp( str, input_A ) == 0 );
+ TEST_ASSERT( str[len] == '!' );
}
}