From 2845fcc8ab0bcce1827a9e94c56962febf9b2b77 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Jul 2020 01:18:11 +0200 Subject: [PATCH 1/4] Always test in-place addition and subtraction Run all the addition and subtraction tests with the result aliased to the first operand and with the result aliased to the second operand. Before, only some of the aliasing possibilities were tested, for only some of the functions, with only some inputs. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 6 --- tests/suites/test_suite_mpi.function | 64 ++++++++++++++-------------- 2 files changed, 33 insertions(+), 37 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 00926ff97..9900abd77 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -433,12 +433,6 @@ mbedtls_mpi_add_abs:10:"-12345678":10:"-642531":10:"12988209" Test mbedtls_mpi_add_abs #1 mbedtls_mpi_add_abs:10:"-643808006803554439230129854961492699151386107534013432918073439524138264842370630061369715394739134090922937332590384720397133335969549256322620979036686633213903952966175107096769180017646161851573147596390153":10:"56125680981752282333498088313568935051383833838594899821664631784577337171193624243181360054669678410455329112434552942717084003541384594864129940145043086760031292483340068923506115878221189886491132772739661669044958531131327771":10:"56125680981752282334141896320372489490613963693556392520816017892111350604111697682705498319512049040516698827829292076808006940873974979584527073481012636016353913462376755556720019831187364993587901952757307830896531678727717924" -Test mbedtls_mpi_add_abs #2 (add to first value) -mpi_add_abs_add_first:10:"123123":10:"123123":10:"246246" - -Test mbedtls_mpi_add_abs #3 (add to second value) -mpi_add_abs_add_second:10:"123123":10:"123123":10:"246246" - Regression mbedtls_mpi_add_abs (add small to very large MPI with carry rollover) mbedtls_mpi_add_abs:16:"FFFFFFFFFFFFFFFFFFFFFFFFFFFFF8":16:"08":16:"1000000000000000000000000000000" diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index f2702f12b..4333ec4f4 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -775,6 +775,15 @@ void mbedtls_mpi_add_mpi( int radix_X, char * input_X, int radix_Y, TEST_ASSERT( mbedtls_mpi_add_mpi( &Z, &X, &Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &A ) == 0 ); + /* result == first operand */ + TEST_ASSERT( mbedtls_mpi_add_mpi( &X, &X, &Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); + + /* result == second operand */ + TEST_ASSERT( mbedtls_mpi_add_mpi( &Y, &X, &Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + exit: mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); mbedtls_mpi_free( &A ); } @@ -820,44 +829,17 @@ void mbedtls_mpi_add_abs( int radix_X, char * input_X, int radix_Y, TEST_ASSERT( mbedtls_mpi_add_abs( &Z, &X, &Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &A ) == 0 ); -exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); mbedtls_mpi_free( &A ); -} -/* END_CASE */ - -/* BEGIN_CASE */ -void mpi_add_abs_add_first( int radix_X, char * input_X, int radix_Y, - char * input_Y, int radix_A, char * input_A ) -{ - mbedtls_mpi X, Y, A; - mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &A ); - - TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_string( &A, radix_A, input_A ) == 0 ); + /* result == first operand */ TEST_ASSERT( mbedtls_mpi_add_abs( &X, &X, &Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &A ) == 0 ); - -exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &A ); -} -/* END_CASE */ - -/* BEGIN_CASE */ -void mpi_add_abs_add_second( int radix_X, char * input_X, int radix_Y, - char * input_Y, int radix_A, char * input_A ) -{ - mbedtls_mpi X, Y, A; - mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &A ); - TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_string( &A, radix_A, input_A ) == 0 ); + + /* result == second operand */ TEST_ASSERT( mbedtls_mpi_add_abs( &Y, &X, &Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &A ); + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); mbedtls_mpi_free( &A ); } /* END_CASE */ @@ -891,6 +873,15 @@ void mbedtls_mpi_sub_mpi( int radix_X, char * input_X, int radix_Y, TEST_ASSERT( mbedtls_mpi_sub_mpi( &Z, &X, &Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &A ) == 0 ); + /* result == first operand */ + TEST_ASSERT( mbedtls_mpi_sub_mpi( &X, &X, &Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); + + /* result == second operand */ + TEST_ASSERT( mbedtls_mpi_sub_mpi( &Y, &X, &Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + exit: mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); mbedtls_mpi_free( &A ); } @@ -914,6 +905,17 @@ void mbedtls_mpi_sub_abs( int radix_X, char * input_X, int radix_Y, if( res == 0 ) TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &A ) == 0 ); + /* result == first operand */ + TEST_ASSERT( mbedtls_mpi_sub_abs( &X, &X, &Y ) == sub_result ); + if( sub_result == 0 ) + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); + + /* result == second operand */ + TEST_ASSERT( mbedtls_mpi_sub_abs( &Y, &X, &Y ) == sub_result ); + if( sub_result == 0 ) + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + exit: mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); mbedtls_mpi_free( &A ); } From 84697ca3591d958d5154005dc0e9831b1f453637 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Jul 2020 01:16:46 +0200 Subject: [PATCH 2/4] Fix memory leak in mbedtls_mpi_sub_abs Fix a memory leak in mbedtls_mpi_sub_abs when the output parameter is aliased to the second operand (X = A - X) and the result is negative. Signed-off-by: Gilles Peskine --- library/bignum.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index f42b97650..89a62a1c4 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1366,7 +1366,10 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi /* If we ran out of space for the carry, it means that the result * is negative. */ if( n == X->n ) - return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); + { + ret = MBEDTLS_ERR_MPI_NEGATIVE_VALUE; + goto cleanup; + } --X->p[n]; } From 7d6326dbf9d3f2e078537d5374175bb547d10e80 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Jul 2020 01:14:34 +0200 Subject: [PATCH 3/4] Fix uncaught error if fix_negative fails fix_negative allocates memory for its result. The calling site didn't check the return value, so an out-of-memory error could lead to an incorrect calculation. Fix this. Signed-off-by: Gilles Peskine --- library/ecp_curves.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 796e0d125..f349cd1c0 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -1046,7 +1046,7 @@ static inline void sub32( uint32_t *dst, uint32_t src, signed char *carry ) STORE32; i++; \ cur = c > 0 ? c : 0; STORE32; \ cur = 0; while( ++i < MAX32 ) { STORE32; } \ - if( c < 0 ) fix_negative( N, c, &C, bits ); + if( c < 0 ) MBEDTLS_MPI_CHK( fix_negative( N, c, &C, bits ) ); /* * If the result is negative, we get it in the form From 3ce9480c38b7ce7482e0a3fd709027c6f8003eb5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 30 Sep 2020 00:04:47 +0200 Subject: [PATCH 4/4] Add changelog entry for the memory management fixes Signed-off-by: Gilles Peskine --- ChangeLog.d/ecp-bignum-error-checks.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/ecp-bignum-error-checks.txt diff --git a/ChangeLog.d/ecp-bignum-error-checks.txt b/ChangeLog.d/ecp-bignum-error-checks.txt new file mode 100644 index 000000000..8cad08e97 --- /dev/null +++ b/ChangeLog.d/ecp-bignum-error-checks.txt @@ -0,0 +1,5 @@ +Bugfix + * Fix a memory leak in mbedtls_mpi_sub_abs() when the result was negative + (an error condition) and the second operand was aliased to the result. + * Fix a case in elliptic curve arithmetic where an out-of-memory condition + could go undetected, resulting in an incorrect result.