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_gen_key() [OpenSSL] #516

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

Comments

@outspace
Copy link

commented Aug 2, 2019

in https://github.com/cossacklabs/themis/blob/master/src/soter/openssl/soter_rsa_common.c, in

if (1 > EVP_PKEY_CTX_ctrl(pkey_ctx, -1, -1, EVP_PKEY_CTRL_RSA_KEYGEN_BITS, SOTER_RSA_KEY_LENGTH, NULL)) {
        return SOTER_FAIL;
    }

I think that BN_free(pub_exp) need to be called in this plase. Please check it out.

@Lagovas

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

yes, agree. we will fix it

@Lagovas

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

but read documentation and think we should not change anything. from documentation of openssl, EVP_PKEY_CTX_ctrl and EVP_PKEY_CTRL_RSA_KEYGEN_BITS the macros for EVP_PKEY_CTX_set_rsa_keygen_pubexp function call - https://docs.huihoo.com/doxygen/openssl/1.0.1c/crypto_2rsa_2rsa_8h_source.html#l00232
And docs tell next:

The EVP_PKEY_CTX_set_rsa_keygen_pubexp() macro sets the public exponent value for RSA key generation to pubexp. Currently it should be an odd integer. The pubexp pointer is used internally by this function so it should not be modified or freed after the call. If not specified 65537 is used.

So we should not free after call

@Lagovas Lagovas closed this Aug 5, 2019
@vixentael vixentael added the core label Aug 5, 2019
@Lagovas

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

hm, you right. not so easy as I thought) thanks very match, I will re-check more carefully usage of these functions and unify across our backends

@Lagovas Lagovas reopened this Aug 5, 2019
@ilammy

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

I have perused OpenSSL's documentation and source code and it seems that the current code in OpenSSL backend is correct:

if (1 > EVP_PKEY_CTX_ctrl(pkey_ctx, -1, -1, EVP_PKEY_CTRL_RSA_KEYGEN_BITS, SOTER_RSA_KEY_LENGTH, NULL)) {
return SOTER_FAIL;
}

We must not free pub_exp after a successful call to EVP_PKEY_CTX_set_rsa_keygen_pubexp() because it transfers ownership over the bignum to EVP_PKEY_CTX:

https://github.com/openssl/openssl/blob/70b0b977f73cd70e17538af3095d18e0cf59132e/crypto/rsa/rsa_pmeth.c#L472-L479

We should fix other places instead.

@ilammy

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Superseded by #517

@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
4 participants
You can’t perform that action at this time.