Ensure the EVP_PKEY operation_cache is appropriately locked

The EVP_PKEY operation_cache caches references to provider side key
objects that have previously been exported for this EVP_PKEY, and their
associated key managers. The cache may be updated from time to time as the
EVP_PKEY is exported to more providers. Since an EVP_PKEY may be shared by
multiple threads simultaneously we must be careful to ensure the cache
updates are locked.

Fixes #13818

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/13987)

2 files changed, 55 insertions, 6 deletions

diff --git a/crypto/evp/keymgmt_lib.c b/crypto/evp/keymgmt_lib.c
index 763982e58f..0c643b3b49 100644
--- a/crypto/evp/keymgmt_lib.c
+++ b/crypto/evp/keymgmt_lib.c
@@ -102,10 +102,16 @@ void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt)
         return pk->keydata;
 
     /* If this key is already exported to |keymgmt|, no more to do */
+    CRYPTO_THREAD_read_lock(pk->lock);
     i = evp_keymgmt_util_find_operation_cache_index(pk, keymgmt);
     if (i < OSSL_NELEM(pk->operation_cache)
-        && pk->operation_cache[i].keymgmt != NULL)
-        return pk->operation_cache[i].keydata;
+        && pk->operation_cache[i].keymgmt != NULL) {
+        void *ret = pk->operation_cache[i].keydata;
+
+        CRYPTO_THREAD_unlock(pk->lock);
+        return ret;
+    }
+    CRYPTO_THREAD_unlock(pk->lock);
 
     /* If the "origin" |keymgmt| doesn't support exporting, give up */
     /*
@@ -153,20 +159,42 @@ void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt)
         return NULL;
     }
 
+    CRYPTO_THREAD_write_lock(pk->lock);
+    /* Check to make sure some other thread didn't get there first */
+    i = evp_keymgmt_util_find_operation_cache_index(pk, keymgmt);
+    if (i < OSSL_NELEM(pk->operation_cache)
+        && pk->operation_cache[i].keymgmt != NULL) {
+        void *ret = pk->operation_cache[i].keydata;
+
+        CRYPTO_THREAD_unlock(pk->lock);
+
+        /*
+         * Another thread seemms to have already exported this so we abandon
+         * all the work we just did.
+         */
+        evp_keymgmt_freedata(keymgmt, import_data.keydata);
+
+        return ret;
+    }
+
     /* Add the new export to the operation cache */
     if (!evp_keymgmt_util_cache_keydata(pk, i, keymgmt, import_data.keydata)) {
         evp_keymgmt_freedata(keymgmt, import_data.keydata);
         return NULL;
     }
 
+    CRYPTO_THREAD_unlock(pk->lock);
+
     return import_data.keydata;
 }
 
-void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk)
+int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking)
 {
     size_t i, end = OSSL_NELEM(pk->operation_cache);
 
     if (pk != NULL) {
+        if (locking && pk->lock != NULL && !CRYPTO_THREAD_write_lock(pk->lock))
+            return 0;
         for (i = 0; i < end && pk->operation_cache[i].keymgmt != NULL; i++) {
             EVP_KEYMGMT *keymgmt = pk->operation_cache[i].keymgmt;
             void *keydata = pk->operation_cache[i].keydata;
@@ -176,7 +204,11 @@ void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk)
             evp_keymgmt_freedata(keymgmt, keydata);
             EVP_KEYMGMT_free(keymgmt);
         }
+        if (locking && pk->lock != NULL)
+            CRYPTO_THREAD_unlock(pk->lock);
     }
+
+    return 1;
 }
 
 size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk,
@@ -198,6 +230,7 @@ int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index,
     if (keydata != NULL) {
         if (!EVP_KEYMGMT_up_ref(keymgmt))
             return 0;
+
         pk->operation_cache[index].keydata = keydata;
         pk->operation_cache[index].keymgmt = keymgmt;
     }
diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index 5df9b19eae..21ce51d573 100644
--- a/crypto/evp/p_lib.c
+++ b/crypto/evp/p_lib.c
@@ -1621,7 +1621,7 @@ static void evp_pkey_free_it(EVP_PKEY *x)
 {
     /* internal function; x is never NULL */
 
-    evp_keymgmt_util_clear_operation_cache(x);
+    evp_keymgmt_util_clear_operation_cache(x, 1);
 #ifndef FIPS_MODULE
     evp_pkey_free_legacy(x);
 #endif
@@ -1735,6 +1735,8 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
          * |i| remains zero, and we will clear the cache further down.
          */
         if (pk->ameth->dirty_cnt(pk) == pk->dirty_cnt_copy) {
+            if (!CRYPTO_THREAD_read_lock(pk->lock))
+                goto end;
             i = evp_keymgmt_util_find_operation_cache_index(pk, tmp_keymgmt);
 
             /*
@@ -1746,8 +1748,10 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
             if (i < OSSL_NELEM(pk->operation_cache)
                 && pk->operation_cache[i].keymgmt != NULL) {
                 keydata = pk->operation_cache[i].keydata;
+                CRYPTO_THREAD_unlock(pk->lock);
                 goto end;
             }
+            CRYPTO_THREAD_unlock(pk->lock);
         }
 
         /*
@@ -1782,12 +1786,22 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
             keydata = NULL;
             goto end;
         }
-        if (pk->ameth->dirty_cnt(pk) != pk->dirty_cnt_copy)
-            evp_keymgmt_util_clear_operation_cache(pk);
+
+        if (!CRYPTO_THREAD_write_lock(pk->lock))
+            goto end;
+        if (pk->ameth->dirty_cnt(pk) != pk->dirty_cnt_copy
+                && !evp_keymgmt_util_clear_operation_cache(pk, 0)) {
+            CRYPTO_THREAD_unlock(pk->lock);
+            evp_keymgmt_freedata(tmp_keymgmt, keydata);
+            keydata = NULL;
+            EVP_KEYMGMT_free(tmp_keymgmt);
+            goto end;
+        }
         EVP_KEYMGMT_free(tmp_keymgmt); /* refcnt-- */
 
         /* Add the new export to the operation cache */
         if (!evp_keymgmt_util_cache_keydata(pk, i, tmp_keymgmt, keydata)) {
+            CRYPTO_THREAD_unlock(pk->lock);
             evp_keymgmt_freedata(tmp_keymgmt, keydata);
             keydata = NULL;
             goto end;
@@ -1795,6 +1809,8 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
 
         /* Synchronize the dirty count */
         pk->dirty_cnt_copy = pk->ameth->dirty_cnt(pk);
+    
+        CRYPTO_THREAD_unlock(pk->lock);
         goto end;
     }
 #endif  /* FIPS_MODULE */