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

Fix potential memory leak in EC multiplication #3318

Merged
merged 4 commits into from
Jun 5, 2020
Merged

Fix potential memory leak in EC multiplication #3318

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
6692a06
Fix potential memory leak in EC multiplication
tiod4420 May 8, 2020
923d579
Add test cases to check rng failure
tiod4420 May 13, 2020
4a67182
Add Changelog entry for #3318
tiod4420 May 13, 2020
b246214
Fix Changelag PR number and uniformize code when prng fails
tiod4420 May 28, 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
3 changes: 3 additions & 0 deletions ChangeLog.d/fix-ecp-mul-memory-leak.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Bugfix
* Fix potential memory leaks in ecp_randomize_jac() and ecp_randomize_mxz()
when PRNG function fails. Contributed by Jonas Lejeune in #3318.
15 changes: 12 additions & 3 deletions library/ecp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,7 +1544,10 @@ static int ecp_randomize_jac( const mbedtls_ecp_group *grp, mbedtls_ecp_point *p
MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, 1 ) );

if( count++ > 10 )
return( MBEDTLS_ERR_ECP_RANDOM_FAILED );
{
ret = MBEDTLS_ERR_ECP_RANDOM_FAILED;
goto cleanup;
}
}
while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 );

Expand Down Expand Up @@ -2278,7 +2281,10 @@ static int ecp_randomize_mxz( const mbedtls_ecp_group *grp, mbedtls_ecp_point *P
MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, 1 ) );

if( count++ > 10 )
return( MBEDTLS_ERR_ECP_RANDOM_FAILED );
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about to do this change @ line 2859 (line number in the initial version of the file) as well:

if( ++count > 30 )                                                   
    return( MBEDTLS_ERR_ECP_RANDOM_FAILED );
ret = mbedtls_mpi_lt_mpi_ct( d, &grp->N, &cmp );                     
    if( ret != 0 )                                                       
    {
        goto cleanup;                                                    
     }

I can see that there is no clean-up to do here (currently) but this would align with the code just below.

ret = MBEDTLS_ERR_ECP_RANDOM_FAILED;
goto cleanup;
}
}
while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 );

Expand Down Expand Up @@ -2856,7 +2862,10 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp,
* such as secp224k1 are actually very close to the worst case.
*/
if( ++count > 30 )
return( MBEDTLS_ERR_ECP_RANDOM_FAILED );
{
ret = MBEDTLS_ERR_ECP_RANDOM_FAILED;
goto cleanup;
}

ret = mbedtls_mpi_lt_mpi_ct( d, &grp->N, &cmp );
if( ret != 0 )
Expand Down
8 changes: 8 additions & 0 deletions tests/suites/test_suite_ecp.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,14 @@ ECP point multiplication Curve25519 (element of order 8) #5
depends_on:MBEDTLS_ECP_DP_CURVE25519_ENABLED
ecp_test_mul:MBEDTLS_ECP_DP_CURVE25519:"5AC99F33632E5A768DE7E81BF854C27C46E3FBF2ABBACD29EC4AFF517369C660":"B8495F16056286FDB1329CEB8D09DA6AC49FF1FAE35616AEB8413B7C7AEBE0":"00":"01":"00":"01":"00":MBEDTLS_ERR_MPI_NOT_ACCEPTABLE

ECP point multiplication rng fail secp256r1
depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED
ecp_test_mul_rng:MBEDTLS_ECP_DP_SECP256R1:"814264145F2F56F2E96A8E337A1284993FAF432A5ABCE59E867B7291D507A3AF"

ECP point multiplication rng fail Curve25519
depends_on:MBEDTLS_ECP_DP_CURVE25519_ENABLED
ecp_test_mul_rng:MBEDTLS_ECP_DP_CURVE25519:"5AC99F33632E5A768DE7E81BF854C27C46E3FBF2ABBACD29EC4AFF517369C660"

ECP test vectors Curve448 (RFC 7748 6.2, after decodeUCoordinate)
depends_on:MBEDTLS_ECP_DP_CURVE448_ENABLED
ecp_test_vec_x:MBEDTLS_ECP_DP_CURVE448:"eb7298a5c0d8c29a1dab27f1a6826300917389449741a974f5bac9d98dc298d46555bce8bae89eeed400584bb046cf75579f51d125498f98":"a01fc432e5807f17530d1288da125b0cd453d941726436c8bbd9c5222c3da7fa639ce03db8d23b274a0721a1aed5227de6e3b731ccf7089b":"ad997351b6106f36b0d1091b929c4c37213e0d2b97e85ebb20c127691d0dad8f1d8175b0723745e639a3cb7044290b99e0e2a0c27a6a301c":"0936f37bc6c1bd07ae3dec7ab5dc06a73ca13242fb343efc72b9d82730b445f3d4b0bd077162a46dcfec6f9b590bfcbcf520cdb029a8b73e":"9d874a5137509a449ad5853040241c5236395435c36424fd560b0cb62b281d285275a740ce32a22dd1740f4aa9161cec95ccc61a18f4ff07"
Expand Down
25 changes: 25 additions & 0 deletions tests/suites/test_suite_ecp.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,31 @@ exit:
}
/* END_CASE */

/* BEGIN_CASE */
void ecp_test_mul_rng( int id, data_t * d_hex)
{
mbedtls_ecp_group grp;
mbedtls_mpi d;
mbedtls_ecp_point Q;

mbedtls_ecp_group_init( &grp ); mbedtls_mpi_init( &d );
mbedtls_ecp_point_init( &Q );

TEST_ASSERT( mbedtls_ecp_group_load( &grp, id ) == 0 );

TEST_ASSERT( mbedtls_ecp_check_pubkey( &grp, &grp.G ) == 0 );

TEST_ASSERT( mbedtls_mpi_read_binary( &d, d_hex->x, d_hex->len ) == 0 );

TEST_ASSERT( mbedtls_ecp_mul( &grp, &Q, &d, &grp.G, &rnd_zero_rand, NULL )
== MBEDTLS_ERR_ECP_RANDOM_FAILED );

exit:
mbedtls_ecp_group_free( &grp ); mbedtls_mpi_free( &d );
mbedtls_ecp_point_free( &Q );
}
/* END_CASE */

/* BEGIN_CASE */
void ecp_fast_mod( int id, char * N_str )
{
Expand Down