[openssl] master update

[tmraz at fedoraproject.org]

The branch master has been updated
       via  8dc34b1f579f71f24aa385d33112da4a91db7079 (commit)
      from  2b407d050868c24ee36172e1abcfbfa0f003a98d (commit)


- Log -----------------------------------------------------------------
commit 8dc34b1f579f71f24aa385d33112da4a91db7079
Author: Daniel Bevenius <daniel.bevenius at gmail.com>
Date:   Wed Nov 11 05:23:11 2020 +0100

    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 at openssl.org>
    Reviewed-by: Tomas Mraz <tmraz at fedoraproject.org>
    (Merged from https://github.com/openssl/openssl/pull/13374)

-----------------------------------------------------------------------

Summary of changes:
 crypto/evp/p_lib.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index a0c131d0c0..ad7a0ebee7 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;