From: Daniel Bevenius <daniel.bevenius@gmail.com>
Date: Wed, 11 Nov 2020 04:23:11 +0000 (+0100)
Subject: EVP: don't touch the lock for evp_pkey_downgrade
X-Git-Tag: openssl-3.0.0-alpha10~254
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8dc34b1f579f71f24aa385d33112da4a91db7079;p=thirdparty%2Fopenssl.git

EVP: don't touch the lock for evp_pkey_downgrade

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 changes the evp_pkey_reset_unlocked to not touch the lock
and the creation of a new lock is done in EVP_PKEY_new.

Refs:
https://github.com/danbev/learning-libcrypto/blob/master/notes/issues.md#openssl-investigationtroubleshooting
https://github.com/nodejs/node/issues/29817

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/13374)
---

diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index a0c131d0c03..ad7a0ebee7a 100644
--- a/crypto/evp/p_lib.c
+++ b/crypto/evp/p_lib.c
@@ -1345,7 +1345,7 @@ size_t EVP_PKEY_get1_encoded_public_key(EVP_PKEY *pkey, unsigned char **ppub)
 /*
  * This reset function must be used very carefully, as it literally throws
  * away everything in an EVP_PKEY without freeing them, and may cause leaks
- * of memory, locks, what have you.
+ * of memory, what have you.
  * The only reason we have this is to have the same code for EVP_PKEY_new()
  * and evp_pkey_downgrade().
  */
@@ -1354,17 +1354,21 @@ static int evp_pkey_reset_unlocked(EVP_PKEY *pk)
     if (pk == NULL)
         return 0;
 
-    memset(pk, 0, sizeof(*pk));
+    if (pk->lock != NULL) {
+      const size_t offset = (unsigned char *)&pk->lock - (unsigned char *)pk;
+
+      memset(pk, 0, offset);
+      memset((unsigned char *)pk + offset + sizeof(pk->lock),
+             0,
+             sizeof(*pk) - offset - sizeof(pk->lock));
+    }
+    /* EVP_PKEY_new uses zalloc so no need to call memset if pk->lock is NULL */
+
     pk->type = EVP_PKEY_NONE;
     pk->save_type = EVP_PKEY_NONE;
     pk->references = 1;
     pk->save_parameters = 1;
 
-    pk->lock = CRYPTO_THREAD_lock_new();
-    if (pk->lock == NULL) {
-        ERR_raise(ERR_LIB_EVP, ERR_R_MALLOC_FAILURE);
-        return 0;
-    }
     return 1;
 }
 
@@ -1380,6 +1384,12 @@ EVP_PKEY *EVP_PKEY_new(void)
     if (!evp_pkey_reset_unlocked(ret))
         goto err;
 
+    ret->lock = CRYPTO_THREAD_lock_new();
+    if (ret->lock == NULL) {
+        EVPerr(ERR_LIB_EVP, ERR_R_MALLOC_FAILURE);
+        goto err;
+    }
+
 #ifndef FIPS_MODULE
     if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_EVP_PKEY, ret, &ret->ex_data)) {
         ERR_raise(ERR_LIB_EVP, ERR_R_MALLOC_FAILURE);
@@ -1880,7 +1890,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))
@@ -1908,12 +1917,9 @@ int evp_pkey_downgrade(EVP_PKEY *pk)
 
     if (evp_pkey_reset_unlocked(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;
@@ -1945,16 +1951,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 */
+        *pk = tmp_copy;
     }
 
-    /* Free the temporary lock.  It should never be NULL */
-    CRYPTO_THREAD_lock_free(tmp_lock);
-
  end:
     if (!CRYPTO_THREAD_unlock(pk->lock))
         return 0;