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

Jump to bottom

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

merged 4 commits into from
Jun 5, 2020

Conversation

tiod4420
Copy link
Contributor

@tiod4420 tiod4420 commented May 8, 2020 •
edited by gilles-peskine-arm
Loading

Signed-off-by: Jonas [email protected]

Description

Fix memory leak issue with EC multiplication described in #3317

Backports: 2.16: #3352; 2.7: #3353

@gilles-peskine-arm gilles-peskine-arm self-requested a review May 11, 2020 10:13
@gilles-peskine-arm gilles-peskine-arm added bug component-crypto Crypto primitives and low-level interfaces needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. needs: changelog labels May 11, 2020
@gilles-peskine-arm
Copy link
Contributor

Thanks for the bug report and the fix!

Please add a changelog entry file.

Would you mind adding a test case to test_suite_ecp? You can use rnd_std_rand for the good RNG. The test should fail on the current code with ASan or Valgrind (we run both on our CI).

@mpg mpg self-requested a review May 12, 2020 10:26
mpg
mpg approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests requested by Gilles. Looks good to me.

@ronald-cron-arm ronald-cron-arm self-requested a review May 18, 2020 13:27
gilles-peskine-arm
gilles-peskine-arm previously approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Thank you for adding a non-regression test!

@gilles-peskine-arm gilles-peskine-arm removed needs: changelog needs-review Every commit must be reviewed by at least two team members, labels May 19, 2020
ronald-cron-arm
ronald-cron-arm reviewed May 19, 2020
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me as well. I just have a suggestion though.

library/ecp.c
@@ -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.

gilles-peskine-arm pushed a commit to gilles-peskine-arm/mbedtls that referenced this pull request May 25, 2020
@tiod4420 @gilles-peskine-arm
Add Changelog entry for Mbed-TLS#3318
2ab27a3 
Signed-off-by: Jonas <[email protected]>
gilles-peskine-arm pushed a commit to gilles-peskine-arm/mbedtls that referenced this pull request May 25, 2020
@tiod4420 @gilles-peskine-arm
Add Changelog entry for Mbed-TLS#3318
3928e82 
Signed-off-by: Jonas <[email protected]>
This was referenced May 28, 2020
Merged
Merged
mpg
mpg requested changes May 28, 2020
View reviewed changes
ChangeLog.d/fix-ecp-mul-memory-leak.txt Outdated
@@ -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 #3317.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not noticing earlier: I think this should be 3318.

@mpg mpg self-requested a review May 28, 2020 11:05
mpg
mpg approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the ChangeLog entry (and uniformizing error handling). Looks good to me.

@mpg
Copy link
Contributor

mpg commented May 29, 2020

Note: the only failure in the CI is a failure to clone the repo in the freebsd part of the pr-merge job. Clearly this is an infrastructure glitch unrelated to this PR, so not a blocker for merging.

@mpg mpg added the needs-review Every commit must be reviewed by at least two team members, label May 29, 2020
ronald-cron-arm
ronald-cron-arm approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my comment.

gilles-peskine-arm pushed a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 4, 2020
@tiod4420 @gilles-peskine-arm
Add Changelog entry for Mbed-TLS#3318
7c0f1c3 
Signed-off-by: Jonas <[email protected]>
gilles-peskine-arm pushed a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 4, 2020
@tiod4420 @gilles-peskine-arm
Add Changelog entry for Mbed-TLS#3318
701063b 
Signed-off-by: Jonas <[email protected]>
@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Jun 4, 2020
@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Jun 5, 2020
@mpg mpg merged commit e860fef into Mbed-TLS:development Jun 5, 2020
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Feb 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@gilles-peskine-arm gilles-peskine-arm Awaiting requested review from gilles-peskine-arm

Assignees

@gilles-peskine-arm gilles-peskine-arm

Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tiod4420 @gilles-peskine-arm @mpg @ronald-cron-arm