Skip to content

Commit

Permalink
EVP: don't call memset for evp_pkey_downgrade
Browse files Browse the repository at this point in the history
This commit tries to address a locking issue in evp_pkey_reset_unlocked
which can occur when it is called from evp_pkey_downgrade.

evp_pkey_downgrade will acquire a lock for pk->lock and if successful
then call evp_pkey_reset_unlocked. evp_pkey_reset_unlocked will call
memset on pk, and then create a new lock and set pk->lock to point to
that new lock. I believe there are two problems with this.

The first is that after the call to memset, another thread would try to
acquire a lock for NULL as that is what the value of pk->lock would be
at that point.

The second issue is that after the new lock has been assigned to
pk->lock, that lock is different from the one currently locked so
another thread trying to acquire the lock will succeed which can lead to
strange behaviour. More details and a reproducer can be found in the
Refs link below.

This commit introduces a new function that is only called by
evp_pkey_downgrade which does not use memset but instead "manually"
sets the EVP_PKEY values to their default values, but does not modify
pk->lock. This could perhaps be updated to go back to only having one
function that is called for both evp_pkey_downgrade and EVP_PKEY_new
and only create a new lock if one does not already exist.

Refs:
https://github.com/danbev/learning-libcrypto/blob/master/notes/issues.md#openssl-investigationtroubleshooting
nodejs/node#29817
  • Loading branch information
danbev committed Nov 24, 2020
1 parent a68eee6 commit 2908a5c
Showing 1 changed file with 43 additions and 11 deletions.
54 changes: 43 additions & 11 deletions crypto/evp/p_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -1368,6 +1368,48 @@ static int evp_pkey_reset_unlocked(EVP_PKEY *pk)
return 1;
}

#ifndef FIPS_MODULE
static int evp_pkey_reset_downgrade(EVP_PKEY *pk)
{
if (pk == NULL)
return 0;

pk->type = EVP_PKEY_NONE;
pk->save_type = EVP_PKEY_NONE;

pk->ameth = NULL;
pk->engine = NULL;
pk->pmeth_engine = NULL;
pk->pkey.ptr = NULL;
# ifndef OPENSSL_NO_RSA
pk->pkey.rsa = NULL;
# endif
# ifndef OPENSSL_NO_DSA
pk->pkey.dsa = NULL;
# endif
# ifndef OPENSSL_NO_DH
pk->pkey.dh = NULL;
# endif
# ifndef OPENSSL_NO_EC
pk->pkey.ec = NULL;
pk->pkey.ecx = NULL;
# endif

pk->references = 1;
pk->save_parameters = 1;
pk->attributes = NULL;
memset(&pk->ex_data, 0, sizeof(CRYPTO_EX_DATA));
pk->keymgmt = NULL;
pk->keydata = NULL;
pk->dirty_cnt = 0;
memset(&pk->operation_cache, 0, sizeof(pk->operation_cache));
pk->dirty_cnt_copy = 0;
memset(&pk->cache, 0, sizeof(pk->cache));

return 1;
}
#endif

EVP_PKEY *EVP_PKEY_new(void)
{
EVP_PKEY *ret = OPENSSL_zalloc(sizeof(*ret));
Expand Down Expand Up @@ -1880,7 +1922,6 @@ int evp_pkey_copy_downgraded(EVP_PKEY **dest, const EVP_PKEY *src)
int evp_pkey_downgrade(EVP_PKEY *pk)
{
EVP_PKEY tmp_copy; /* Stack allocated! */
CRYPTO_RWLOCK *tmp_lock = NULL; /* Temporary lock */
int rv = 0;

if (!ossl_assert(pk != NULL))
Expand All @@ -1906,14 +1947,11 @@ int evp_pkey_downgrade(EVP_PKEY *pk)
*/
tmp_copy = *pk; /* |tmp_copy| now owns THE lock */

if (evp_pkey_reset_unlocked(pk)
if (evp_pkey_reset_downgrade(pk)
&& evp_pkey_copy_downgraded(&pk, &tmp_copy)) {
/* Grab the temporary lock to avoid lock leak */
tmp_lock = pk->lock;

/* Restore the common attributes, then empty |tmp_copy| */
pk->references = tmp_copy.references;
pk->lock = tmp_copy.lock; /* |pk| now owns THE lock */
pk->attributes = tmp_copy.attributes;
pk->save_parameters = tmp_copy.save_parameters;
pk->ex_data = tmp_copy.ex_data;
Expand Down Expand Up @@ -1945,16 +1983,10 @@ int evp_pkey_downgrade(EVP_PKEY *pk)
evp_pkey_free_it(&tmp_copy);
rv = 1;
} else {
/* Grab the temporary lock to avoid lock leak */
tmp_lock = pk->lock;

/* Restore the original key */
*pk = tmp_copy; /* |pk| now owns THE lock */
}

/* Free the temporary lock. It should never be NULL */
CRYPTO_THREAD_lock_free(tmp_lock);

end:
if (!CRYPTO_THREAD_unlock(pk->lock))
return 0;
Expand Down

0 comments on commit 2908a5c

Please sign in to comment.