read lock store on ossl_method_store_do_all

Theres a data race between ossl_method_store_insert and
ossl_method_store_do_all, as the latter doesn't take the property lock
before iterating.

However, we can't lock in do_all, as the call stack in several cases
later attempts to take the write lock.

The choices to fix it are I think:
1) add an argument to indicate to ossl_method_store_do_all weather to
   take the read or write lock when doing iterations, and add an
   is_locked api to the ossl_property_[read|write] lock family so that
   subsequent callers can determine if they need to take a lock or not

2) Clone the algs sparse array in ossl_method_store_do_all and use the
   clone to iterate with no lock held, ensuring that updates to the
   parent copy of the sparse array are left untoucheTheres a data race
   between ossl_method_store_insert and ossl_method_store_do_all, as the
   latter doesn't take the property lock before iterating.

I think method (2), while being a bit more expensive, is probably the
far less invasive way to go here

Fixes #24672

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24782)

diff --git a/crypto/property/property.c b/crypto/property/property.c
index c551c825b19b31a3fab3453df5b2703c2e839569..8f31e5da9bef757463cdc87e270611629fb10e02 100644 (file)
--- a/crypto/property/property.c
+++ b/crypto/property/property.c
@@ -96,6 +96,8 @@ typedef struct {
 
 DEFINE_SPARSE_ARRAY_OF(ALGORITHM);
 
+DEFINE_STACK_OF(ALGORITHM)
+
 typedef struct ossl_global_properties_st {
     OSSL_PROPERTY_LIST *list;
 #ifndef FIPS_MODULE
@@ -461,33 +463,45 @@ static void alg_do_one(ALGORITHM *alg, IMPLEMENTATION *impl,
     fn(alg->nid, impl->method.method, fnarg);
 }
 
-struct alg_do_each_data_st {
-    void (*fn)(int id, void *method, void *fnarg);
-    void *fnarg;
-};
-
-static void alg_do_each(ossl_uintmax_t idx, ALGORITHM *alg, void *arg)
+static void alg_copy(ossl_uintmax_t idx, ALGORITHM *alg, void *arg)
 {
-    struct alg_do_each_data_st *data = arg;
-    int i, end = sk_IMPLEMENTATION_num(alg->impls);
-
-    for (i = 0; i < end; i++) {
-        IMPLEMENTATION *impl = sk_IMPLEMENTATION_value(alg->impls, i);
+    STACK_OF(ALGORITHM) *newalg = arg;
 
-        alg_do_one(alg, impl, data->fn, data->fnarg);
-    }
+    (void)sk_ALGORITHM_push(newalg, alg);
 }
 
 void ossl_method_store_do_all(OSSL_METHOD_STORE *store,
                               void (*fn)(int id, void *method, void *fnarg),
                               void *fnarg)
 {
-    struct alg_do_each_data_st data;
+    int i, j;
+    int numalgs, numimps;
+    STACK_OF(ALGORITHM) *tmpalgs;
+    ALGORITHM *alg;
 
-    data.fn = fn;
-    data.fnarg = fnarg;
-    if (store != NULL)
-        ossl_sa_ALGORITHM_doall_arg(store->algs, alg_do_each, &data);
+    if (store != NULL) {
+
+        if (!ossl_property_read_lock(store))
+            return;
+       
+        tmpalgs = sk_ALGORITHM_new_reserve(NULL,
+                                           ossl_sa_ALGORITHM_num(store->algs));
+        if (tmpalgs == NULL) {
+            ossl_property_unlock(store);
+            return;
+        }
+
+        ossl_sa_ALGORITHM_doall_arg(store->algs, alg_copy, tmpalgs);
+        ossl_property_unlock(store);
+        numalgs = sk_ALGORITHM_num(tmpalgs);
+        for (i = 0; i < numalgs; i++) {
+            alg = sk_ALGORITHM_value(tmpalgs, i);
+            numimps = sk_IMPLEMENTATION_num(alg->impls);
+            for (j = 0; j < numimps; j++)
+                alg_do_one(alg, sk_IMPLEMENTATION_value(alg->impls, j), fn, fnarg);
+        }
+        sk_ALGORITHM_free(tmpalgs);
+    }
 }
 
 int ossl_method_store_fetch(OSSL_METHOD_STORE *store,