From 2d5695016d880b9c6681f293ed5afb0379ce86b7 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 23 Apr 2021 16:18:28 +0100 Subject: [PATCH] Properly protect access to the provider flag_activated field This was not always locked when it should be. Fixes #15005 Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/15010) --- crypto/provider_core.c | 110 ++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 41 deletions(-) diff --git a/crypto/provider_core.c b/crypto/provider_core.c index f3a4f297bf..1ef2cd5ca7 100644 --- a/crypto/provider_core.c +++ b/crypto/provider_core.c @@ -48,7 +48,6 @@ struct ossl_provider_st { unsigned int flag_initialized:1; unsigned int flag_activated:1; unsigned int flag_fallback:1; /* Can be used as fallback */ - unsigned int flag_activated_as_fallback:1; /* Getting and setting the flags require synchronization */ CRYPTO_RWLOCK *flag_lock; @@ -56,8 +55,7 @@ struct ossl_provider_st { /* OpenSSL library side data */ CRYPTO_REF_COUNT refcnt; CRYPTO_RWLOCK *refcnt_lock; /* For the ref counter */ - CRYPTO_REF_COUNT activatecnt; - CRYPTO_RWLOCK *activatecnt_lock; /* For the activate counter */ + int activatecnt; char *name; char *path; DSO *module; @@ -263,7 +261,6 @@ static OSSL_PROVIDER *provider_new(const char *name, if ((prov = OPENSSL_zalloc(sizeof(*prov))) == NULL #ifndef HAVE_ATOMICS || (prov->refcnt_lock = CRYPTO_THREAD_lock_new()) == NULL - || (prov->activatecnt_lock = CRYPTO_THREAD_lock_new()) == NULL #endif || !ossl_provider_up_ref(prov) /* +1 One reference to be returned */ || (prov->opbits_lock = CRYPTO_THREAD_lock_new()) == NULL @@ -395,7 +392,6 @@ void ossl_provider_free(OSSL_PROVIDER *prov) CRYPTO_THREAD_lock_free(prov->flag_lock); #ifndef HAVE_ATOMICS CRYPTO_THREAD_lock_free(prov->refcnt_lock); - CRYPTO_THREAD_lock_free(prov->activatecnt_lock); #endif OPENSSL_free(prov); } @@ -479,7 +475,7 @@ int OSSL_PROVIDER_set_default_search_path(OSSL_LIB_CTX *libctx, * locking. Direct callers must remember to set the store flags when * appropriate. */ -static int provider_init(OSSL_PROVIDER *prov) +static int provider_init(OSSL_PROVIDER *prov, int flag_lock) { const OSSL_DISPATCH *provider_dispatch = NULL; void *tmp_provctx = NULL; /* safety measure */ @@ -496,7 +492,7 @@ static int provider_init(OSSL_PROVIDER *prov) * modifies a number of things in the provider structure that this * function needs to perform under lock anyway. */ - if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) + if (flag_lock && !CRYPTO_THREAD_write_lock(prov->flag_lock)) goto end; if (prov->flag_initialized) { ok = 1; @@ -675,48 +671,41 @@ static int provider_init(OSSL_PROVIDER *prov) ok = 1; end: - CRYPTO_THREAD_unlock(prov->flag_lock); + if (flag_lock) + CRYPTO_THREAD_unlock(prov->flag_lock); return ok; } static int provider_deactivate(OSSL_PROVIDER *prov) { - int ref = 0; - if (!ossl_assert(prov != NULL)) return 0; - if (CRYPTO_DOWN_REF(&prov->activatecnt, &ref, prov->activatecnt_lock) <= 0) + if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) return 0; - if (ref < 1) { - if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) - return 0; + if (--prov->activatecnt < 1) prov->flag_activated = 0; - CRYPTO_THREAD_unlock(prov->flag_lock); - } + + CRYPTO_THREAD_unlock(prov->flag_lock); /* We don't deinit here, that's done in ossl_provider_free() */ return 1; } -static int provider_activate(OSSL_PROVIDER *prov) +static int provider_activate(OSSL_PROVIDER *prov, int flag_lock) { - int ref = 0; - - if (CRYPTO_UP_REF(&prov->activatecnt, &ref, prov->activatecnt_lock) <= 0) - return 0; - - if (provider_init(prov)) { - if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) + if (provider_init(prov, flag_lock)) { + if (flag_lock && !CRYPTO_THREAD_write_lock(prov->flag_lock)) return 0; + prov->activatecnt++; prov->flag_activated = 1; - CRYPTO_THREAD_unlock(prov->flag_lock); + if (flag_lock) + CRYPTO_THREAD_unlock(prov->flag_lock); return 1; } - provider_deactivate(prov); return 0; } @@ -724,7 +713,7 @@ int ossl_provider_activate(OSSL_PROVIDER *prov, int retain_fallbacks) { if (prov == NULL) return 0; - if (provider_activate(prov)) { + if (provider_activate(prov, 1)) { if (!retain_fallbacks) { if (!CRYPTO_THREAD_write_lock(prov->store->lock)) { provider_deactivate(prov); @@ -784,10 +773,8 @@ static void provider_activate_fallbacks(struct provider_store_st *store) if (ossl_provider_up_ref(prov)) { if (prov->flag_fallback) { - if (provider_activate(prov)) { - prov->flag_activated_as_fallback = 1; + if (provider_activate(prov, 1)) activated_fallback_count++; - } } ossl_provider_free(prov); } @@ -810,7 +797,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx, void *cbdata), void *cbdata) { - int ret = 0, i, j; + int ret = 0, curr, max; struct provider_store_st *store = get_provider_store(ctx); STACK_OF(OSSL_PROVIDER) *provs = NULL; @@ -837,21 +824,48 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx, CRYPTO_THREAD_unlock(store->lock); return 0; } - j = sk_OSSL_PROVIDER_num(provs); - for (i = 0; i < j; i++) - if (!ossl_provider_up_ref(sk_OSSL_PROVIDER_value(provs, i))) + max = sk_OSSL_PROVIDER_num(provs); + /* + * We work backwards through the stack so that we can safely delete items + * as we go. + */ + for (curr = max - 1; curr >= 0; curr--) { + OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr); + + if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) goto err_unlock; + if (prov->flag_activated) { + if (!ossl_provider_up_ref(prov)){ + CRYPTO_THREAD_unlock(prov->flag_lock); + goto err_unlock; + } + /* + * It's already activated, but we up the activated count to ensure + * it remains activated until after we've called the user callback. + */ + if (!provider_activate(prov, 0)) { + ossl_provider_free(prov); + CRYPTO_THREAD_unlock(prov->flag_lock); + goto err_unlock; + } + } else { + sk_OSSL_PROVIDER_delete(provs, curr); + max--; + } + CRYPTO_THREAD_unlock(prov->flag_lock); + } CRYPTO_THREAD_unlock(store->lock); /* * Now, we sweep through all providers not under lock */ - for (j = 0; j < i; j++) { - OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, j); + for (curr = 0; curr < max; curr++) { + OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr); - if (prov->flag_activated && !cb(prov, cbdata)) + if (!cb(prov, cbdata)) goto finish; } + curr = -1; ret = 1; goto finish; @@ -859,19 +873,33 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx, err_unlock: CRYPTO_THREAD_unlock(store->lock); finish: - /* The pop_free call doesn't do what we want on an error condition */ - for (j = 0; j < i; j++) - ossl_provider_free(sk_OSSL_PROVIDER_value(provs, j)); + /* + * The pop_free call doesn't do what we want on an error condition. We + * either start from the first item in the stack, or part way through if + * we only processed some of the items. + */ + for (curr++; curr < max; curr++) { + OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr); + + provider_deactivate(prov); + ossl_provider_free(prov); + } sk_OSSL_PROVIDER_free(provs); return ret; } int ossl_provider_available(OSSL_PROVIDER *prov) { + int ret; + if (prov != NULL) { provider_activate_fallbacks(prov->store); - return prov->flag_activated; + if (!CRYPTO_THREAD_read_lock(prov->flag_lock)) + return 0; + ret = prov->flag_activated; + CRYPTO_THREAD_unlock(prov->flag_lock); + return ret; } return 0; } -- 2.39.2