Fix a race in ossl_provider_add_to_store()

If two threads both attempt to load the same provider at the same time,
they will first both check to see if the provider already exists. If it
doesn't then they will both then create new provider objects and call the
init function. However only one of the threads will be successful in adding
the provider to the store. For the "losing" thread we should still return
"success", but we should deinitialise and free the no longer required
provider object, and return the object that exists in the store.

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

diff --git a/crypto/provider.c b/crypto/provider.c
index c8db329837c0332818d67e41fe2e5c87467fb2b9..82d980a8aee3358d66d5bafe66feb954ddd7d40d 100644 (file)
--- a/crypto/provider.c
+++ b/crypto/provider.c
@@ -18,7 +18,7 @@
 OSSL_PROVIDER *OSSL_PROVIDER_try_load(OSSL_LIB_CTX *libctx, const char *name,
                                       int retain_fallbacks)
 {
-    OSSL_PROVIDER *prov = NULL;
+    OSSL_PROVIDER *prov = NULL, *actual;
     int isnew = 0;
 
     /* Find it or create it */
@@ -33,13 +33,14 @@ OSSL_PROVIDER *OSSL_PROVIDER_try_load(OSSL_LIB_CTX *libctx, const char *name,
         return NULL;
     }
 
-    if (isnew && !ossl_provider_add_to_store(prov, retain_fallbacks)) {
+    actual = prov;
+    if (isnew && !ossl_provider_add_to_store(prov, &actual, retain_fallbacks)) {
         ossl_provider_deactivate(prov);
         ossl_provider_free(prov);
         return NULL;
     }
 
-    return prov;
+    return actual;
 }
 
 OSSL_PROVIDER *OSSL_PROVIDER_load(OSSL_LIB_CTX *libctx, const char *name)

diff --git a/crypto/provider_child.c b/crypto/provider_child.c
index 3cad1c564f46cf46fe2e4281e909b7ff5c8d9f4a..272d67a52d80ace5e503bc1b002751e2d0610118 100644 (file)
--- a/crypto/provider_child.c
+++ b/crypto/provider_child.c
@@ -127,9 +127,9 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata)
 
     if ((cprov = ossl_provider_find(ctx, provname, 1)) != NULL) {
         /*
-        * We free the newly created ref. We rely on the provider sticking around
-        * in the provider store.
-        */
+         * We free the newly created ref. We rely on the provider sticking around
+         * in the provider store.
+         */
         ossl_provider_free(cprov);
 
         /*
@@ -152,17 +152,11 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata)
             goto err;
 
         if (!ossl_provider_set_child(cprov, prov)
-            || !ossl_provider_add_to_store(cprov, 0)) {
+            || !ossl_provider_add_to_store(cprov, NULL, 0)) {
             ossl_provider_deactivate(cprov);
             ossl_provider_free(cprov);
             goto err;
         }
-
-        /*
-        * We free the newly created ref. We rely on the provider sticking around
-        * in the provider store.
-        */
-        ossl_provider_free(cprov);
     }
 
     ret = 1;

diff --git a/crypto/provider_conf.c b/crypto/provider_conf.c
index 14a2d62a7ef34f955f5963466de338d51353be61..1d4e695fb89441354db6b5f9be0c5fc5c10d73ce 100644 (file)
--- a/crypto/provider_conf.c
+++ b/crypto/provider_conf.c
@@ -113,7 +113,7 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name,
     int i;
     STACK_OF(CONF_VALUE) *ecmds;
     int soft = 0;
-    OSSL_PROVIDER *prov = NULL;
+    OSSL_PROVIDER *prov = NULL, *actual = NULL;
     const char *path = NULL;
     long activate = 0;
     int ok = 0;
@@ -173,13 +173,13 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name,
         if (ok) {
             if (!ossl_provider_activate(prov, 1, 0)) {
                 ok = 0;
-            } else if (!ossl_provider_add_to_store(prov, 0)) {
+            } else if (!ossl_provider_add_to_store(prov, &actual, 0)) {
                 ossl_provider_deactivate(prov);
                 ok = 0;
             } else {
                 if (pcgbl->activated_providers == NULL)
                     pcgbl->activated_providers = sk_OSSL_PROVIDER_new_null();
-                sk_OSSL_PROVIDER_push(pcgbl->activated_providers, prov);
+                sk_OSSL_PROVIDER_push(pcgbl->activated_providers, actual);
                 ok = 1;
             }
         }

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index 1878ffab7c5104e3df88882858a91f041015b2a5..1f688557c16e722717d03e162a6fcf05b05c7a5a 100644 (file)
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -511,33 +511,69 @@ static int create_provider_children(OSSL_PROVIDER *prov)
     return ret;
 }
 
-int ossl_provider_add_to_store(OSSL_PROVIDER *prov, int retain_fallbacks)
+int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
+                               int retain_fallbacks)
 {
-    struct provider_store_st *store = NULL;
-    int ret = 1;
+    struct provider_store_st *store;
+    int idx;
+    OSSL_PROVIDER tmpl = { 0, };
+    OSSL_PROVIDER *actualtmp = NULL;
 
     if ((store = get_provider_store(prov->libctx)) == NULL)
         return 0;
 
-
-    if (!ossl_provider_up_ref(prov)) {
-        ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
+    if (!CRYPTO_THREAD_write_lock(store->lock))
         return 0;
+
+    tmpl.name = (char *)prov->name;
+    idx = sk_OSSL_PROVIDER_find(store->providers, &tmpl);
+    if (idx == -1)
+        actualtmp = prov;
+    else
+        actualtmp = sk_OSSL_PROVIDER_value(store->providers, idx);
+
+    if (actualprov != NULL) {
+        if (!ossl_provider_up_ref(actualtmp)) {
+            ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
+            actualtmp = NULL;
+            goto err;
+        }
+        *actualprov = actualtmp;
     }
-    if (!CRYPTO_THREAD_write_lock(store->lock)
-            || sk_OSSL_PROVIDER_push(store->providers, prov) == 0) {
-        ossl_provider_free(prov);
-        ret = 0;
-    }
-    prov->store = store;
-    if (!retain_fallbacks)
-        store->use_fallbacks = 0;
-    if (!create_provider_children(prov)) {
-        ret = 0;
+
+    if (idx == -1) {
+        if (sk_OSSL_PROVIDER_push(store->providers, prov) == 0)
+            goto err;
+        prov->store = store;
+        if (!create_provider_children(prov)) {
+            sk_OSSL_PROVIDER_delete_ptr(store->providers, prov);
+            goto err;
+        }
+        if (!retain_fallbacks)
+            store->use_fallbacks = 0;
     }
+
     CRYPTO_THREAD_unlock(store->lock);
 
-    return ret;
+    if (actualtmp != prov) {
+        /*
+         * The provider is already in the store. Probably two threads
+         * independently initialised their own provider objects with the same
+         * name and raced to put them in the store. This thread lost. We
+         * deactivate the one we just created and use the one that already
+         * exists instead.
+         */
+        ossl_provider_deactivate(prov);
+        ossl_provider_free(prov);
+    }
+
+    return 1;
+
+ err:
+    CRYPTO_THREAD_unlock(store->lock);
+    if (actualprov != NULL)
+        ossl_provider_free(actualtmp);
+    return 0;
 }
 
 void ossl_provider_free(OSSL_PROVIDER *prov)

diff --git a/doc/internal/man3/ossl_provider_new.pod b/doc/internal/man3/ossl_provider_new.pod
index 928cc9b844e6592c82b2ebd40b32bc95a152432f..09b2e041172f96c1563104cf892c3d3f7cb0fef2 100644 (file)
--- a/doc/internal/man3/ossl_provider_new.pod
+++ b/doc/internal/man3/ossl_provider_new.pod
@@ -55,7 +55,8 @@ ossl_provider_get_capabilities
   */
  int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild);
  int ossl_provider_deactivate(OSSL_PROVIDER *prov);
- int ossl_provider_add_to_store(OSSL_PROVIDER *prov, int retain_fallbacks);
+ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
+                                int retain_fallbacks);
 
  /* Return pointer to the provider's context */
  void *ossl_provider_ctx(const OSSL_PROVIDER *prov);
@@ -229,7 +230,13 @@ that count reaches zero, the activation flag is cleared.
 
 ossl_provider_add_to_store() adds the provider I<prov> to the provider store and
 makes it available to other threads. This will prevent future automatic loading
-of fallback providers, unless I<retain_fallbacks> is true.
+of fallback providers, unless I<retain_fallbacks> is true. If a provider of the
+same name already exists in the store then it is not added but this function
+still returns success. On success the I<actualprov> value is populated with a
+pointer to the provider of the given name that is now in the store. The
+reference passed in the I<prov> argument is consumed by this function. A
+reference to the provider that should be used is passed back in the
+I<actualprov> argument.
 
 ossl_provider_ctx() returns a context created by the provider.
 Outside of the provider, it's completely opaque, but it needs to be

diff --git a/include/internal/provider.h b/include/internal/provider.h
index 9b1d9495ddad8a506455cb279f28c7a426aee65b..237c852e8dcd91e1ebd3d902022683abe6d53657 100644 (file)
--- a/include/internal/provider.h
+++ b/include/internal/provider.h
@@ -58,7 +58,8 @@ int ossl_provider_disable_fallback_loading(OSSL_LIB_CTX *libctx);
  */
 int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild);
 int ossl_provider_deactivate(OSSL_PROVIDER *prov);
-int ossl_provider_add_to_store(OSSL_PROVIDER *prov, int retain_fallbacks);
+int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
+                               int retain_fallbacks);
 
 /* Return pointer to the provider's context */
 void *ossl_provider_ctx(const OSSL_PROVIDER *prov);