Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing some error checks in ECP and bignum #3942

Merged
merged 4 commits into from
Dec 7, 2020
Merged

Add missing some error checks in ECP and bignum #3942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56f943a
Always test in-place addition and subtraction
gilles-peskine-arm Jul 22, 2020
89b4130
Fix memory leak in mbedtls_mpi_sub_abs
gilles-peskine-arm Jul 22, 2020
e54be3a
Fix uncaught error if fix_negative fails
gilles-peskine-arm Jul 22, 2020
e410bf7
Add changelog entry for the memory management fixes
gilles-peskine-arm Sep 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog.d/ecp-bignum-error-checks.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 4 additions & 1 deletion library/bignum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,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];
}

Expand Down
2 changes: 1 addition & 1 deletion library/ecp_curves.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,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
Expand Down
6 changes: 0 additions & 6 deletions tests/suites/test_suite_mpi.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,12 +445,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) [#1]
mbedtls_mpi_add_abs:16:"FFFFFFFFFFFFFFFFFFFFFFFFFFFFF8":16:"08":16:"1000000000000000000000000000000"

Expand Down
64 changes: 33 additions & 31 deletions tests/suites/test_suite_mpi.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,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 );
}
Expand Down Expand Up @@ -874,44 +883,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 */

Expand Down Expand Up @@ -945,6 +927,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 );
}
Expand All @@ -968,6 +959,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 );
}
Expand Down