From: Matt Caswell Date: Wed, 27 Jan 2021 17:18:27 +0000 (+0000) Subject: Ensure the EVP_PKEY operation_cache is appropriately locked X-Git-Tag: openssl-3.0.0-alpha12~158 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0b07db6f56e0240de6cc2ea122eee6431459ef20;p=thirdparty%2Fopenssl.git 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 (Merged from https://github.com/openssl/openssl/pull/13987) --- diff --git a/crypto/evp/keymgmt_lib.c b/crypto/evp/keymgmt_lib.c index 763982e58f7..0c643b3b49f 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 5df9b19eaea..21ce51d5737 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 */ diff --git a/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod b/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod index bb2ad9ba8eb..31f8b00e47d 100644 --- a/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod +++ b/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod @@ -20,9 +20,9 @@ evp_keymgmt_util_fromdata void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); - void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk); - void evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, - EVP_KEYMGMT *keymgmt, void *keydata); + int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking); + int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, + EVP_KEYMGMT *keymgmt, void *keydata); void evp_keymgmt_util_cache_keyinfo(EVP_PKEY *pk); void *evp_keymgmt_util_fromdata(EVP_PKEY *target, EVP_KEYMGMT *keymgmt, int selection, const OSSL_PARAM params[]); @@ -44,10 +44,13 @@ as this function ignores any legacy key data. evp_keymgmt_util_find_operation_cache_index() finds the location if I in I's cache of provided keys for operations. If I is NULL or couldn't be found in the cache, it finds the -first empty slot instead if there is any. +first empty slot instead if there is any. It should only be called while +holding I's lock (read or write). evp_keymgmt_util_clear_operation_cache() can be used to explicitly -clear the cache of operation key references. +clear the cache of operation key references. If I is set to 1 then +then I's lock will be obtained while doing the clear. Otherwise it will be +assumed that the lock has already been obtained or is not required. evp_keymgmt_util_cache_keydata() can be used to assign a provider key object to a specific cache slot in the given I. @@ -72,6 +75,9 @@ operation cache slot. If I is NULL, or if there is no slot with a match for I, the index of the first empty slot is returned, or the maximum number of slots if there isn't an empty one. +evp_keymgmt_util_cache_keydata() and evp_keymgmt_util_clear_operation_cache() +return 1 on success or 0 otherwise. + =head1 NOTES "Legacy key" is the term used for any key that has been assigned to an diff --git a/include/crypto/evp.h b/include/crypto/evp.h index 20335e9a321..bed75f406cc 100644 --- a/include/crypto/evp.h +++ b/include/crypto/evp.h @@ -729,7 +729,7 @@ int evp_keymgmt_util_export(const EVP_PKEY *pk, int selection, void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); -void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk); +int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking); int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, EVP_KEYMGMT *keymgmt, void *keydata); void evp_keymgmt_util_cache_keyinfo(EVP_PKEY *pk);