Possible unchecked function call in crypto/rsa/rsa_ameth.c

[robbe0x00]

version: latest

In the function rsa_item_sign, there is a call to EVP_MD_CTX_get_pkey_ctx. https://github.com/openssl/openssl/blob/f426dd1311eecd12f24190c94f56eb85e62aaa27/crypto/rsa/rsa_ameth.c#L654 The result of this call is not checked to be non-NULL. The EVP_MD_CTX_get_pkey_ctx function is called all over the codebase, and is almost never checked to return a NON-null result.

However, in the apps/speed.c file, the result of this function is checked at two different points: https://github.com/openssl/openssl/blob/f426dd1311eecd12f24190c94f56eb85e62aaa27/apps/speed.c#L4699 https://github.com/openssl/openssl/blob/f426dd1311eecd12f24190c94f56eb85e62aaa27/apps/speed.c#L4704

It is not immediately obvious to me if this function can return NULL. Should it be possible, then the rsa_ameth file is certainly not the only location where this check is missing. Should this never be able to return NULL, maybe the speed.c file needs to be changed to avoid confusion.

Found while working on a static analyser.

[andrewkdinh]

I am also not sure if EVP_MD_CTX_get_pkey_ctx can ever return NULL, but even if we're calling it in places we expect to have it for sure be non-NULL, it should probably be checked. @nhorman @t8m @mattcaswell can any of y'all comment on this?

EVP_PKEY_CTX is defined here: https://github.com/openssl/openssl/blob/c37b9e3425c8576d089342c7cfdcc4dc0aedde54/crypto/evp/evp_lib.c#L1069-L1072

EVP_MD_CTX is defined as such: https://github.com/openssl/openssl/blob/c37b9e3425c8576d089342c7cfdcc4dc0aedde54/include/openssl/types.h#L113

https://github.com/openssl/openssl/blob/c37b9e3425c8576d089342c7cfdcc4dc0aedde54/crypto/evp/evp_local.h#L16-L24

[t8m]

In general EVP_MD_CTX_get_pkey_ctx() can of course return NULL. However I do not think in the particular call in rsa_ameth.c it can.

[andrewkdinh]

Should we add the non-NULL check everywhere as a matter of caution?

[mattcaswell]

I am also not sure if EVP_MD_CTX_get_pkey_ctx can ever return NULL

In general it can.

In the function rsa_item_sign it is currently not possible for the EVP_MD_CTX to be in such a state that EVP_MD_CTX_get_pkey_ctx ever would return NULL.

That is based on the code as it currently stands. It is always possible of course for the code to be modified such that unexpected things happen, so we should probably not rely on that.

However, even in the case that it does return NULL, in practice the code would do the right thing. The call on the next line to EVP_PKEY_CTX_get_rsa_padding correctly handles a NULL EVP_PKEY_CTX being passed to it, and returns an error if so. So an additional check for NULL is redundant (although harmless) in this case.