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.

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

possible memory leak in soter_rsa_priv_key_to_engine_specific() [OpenSSL] #514

Closed
outspace opened this issue Aug 2, 2019 · 5 comments

Comments

@outspace
Copy link

commented Aug 2, 2019

In https://github.com/cossacklabs/themis/blob/master/src/soter/openssl/soter_rsa_key.c In function soter_rsa_priv_key_to_engine_specific() when you checking for BIGNUM allocation you don't free previous allocated BIGNUM.

if (!rsa_dmp1) {
        RSA_free(rsa);
        return SOTER_NO_MEMORY;
}

In this case rsa_q, rsa_p, rsa_d, rsa_e will leak.

@outspace

This comment has been minimized.

Copy link
Author

commented Aug 2, 2019

Maybe it will be a good idea to use BN_secure_new() for allocation

@vixentael

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Thank you, we'll take a deeper look!

Also, we'll look at similar piece for BoringSSL
https://github.com/cossacklabs/themis/blob/master/src/soter/boringssl/soter_rsa_key.c#L394

Have you found it by manual scanning, or using some tool?

@outspace

This comment has been minimized.

Copy link
Author

commented Aug 2, 2019

It just static code analysis. In your thread of BoringSSL you are using rsa structure directly and if something go wrong it will be free in RSA_free().

@Lagovas

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

oh yes, looks like we need here a lot of bn_free...
and manually re-check with valgrind and check that he really not found these leaks

@outspace which static code analysis you used? if it is not a secret )

@ilammy

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@outspace, thank you for your keen eyes. Known leaks have been eliminated.

@ilammy ilammy closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.