From e7876341af8812c6d6571fe88bc8595e8591ce77 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 a6a642306..a07e9af16 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -427,12 +427,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 d023466eb..0d311e3a5 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -570,6 +570,15 @@ void mbedtls_mpi_add_mpi( int radix_X, char *input_X, int radix_Y, char *input_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 ); } @@ -614,44 +623,17 @@ void mbedtls_mpi_add_abs( int radix_X, char *input_X, int radix_Y, char *input_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 */ @@ -685,6 +667,15 @@ void mbedtls_mpi_sub_mpi( int radix_X, char *input_X, int radix_Y, char *input_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 ); } @@ -707,6 +698,17 @@ void mbedtls_mpi_sub_abs( int radix_X, char *input_X, int radix_Y, char *input_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 91070e43a6f33e6ce99f2d5779918f7d6b259dd6 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 0e39e3a44..c4eb7b04b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1201,7 +1201,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 b51c8a29b5b5a1efffdfd3e341911f0774bbb737 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 26f62584c..933158706 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -974,7 +974,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 9dd91ecf53bbc128755812afb0e00dc3290254f3 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.