From c1d56231ef6385b557ec72eec508e55ea26ca8b0 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Wed, 21 Aug 2019 10:08:44 +0200 Subject: [PATCH] Modify ossl_method_store_add() to accept an OSSL_PROVIDER and check for it If ossl_method_store_add() gets called with a method that already exists (i.e. the store has one with matching provider, nid and properties), that method should not be stored. We do this check inside ossl_method_store_add() because it has all the locking required to do so safely. Fixes #9561 Reviewed-by: Shane Lontis (Merged from https://github.com/openssl/openssl/pull/9650) --- crypto/core_fetch.c | 10 +++++----- crypto/evp/evp_fetch.c | 8 ++++---- crypto/property/property.c | 21 +++++++++++++++++---- doc/internal/man3/OSSL_METHOD_STORE.pod | 8 +++++--- include/internal/core.h | 4 ++-- include/internal/property.h | 12 ++++++------ test/property_test.c | 13 ++++++++----- 7 files changed, 47 insertions(+), 29 deletions(-) diff --git a/crypto/core_fetch.c b/crypto/core_fetch.c index c1c81588bb..6e4414d831 100644 --- a/crypto/core_fetch.c +++ b/crypto/core_fetch.c @@ -52,14 +52,14 @@ static void ossl_method_construct_this(OSSL_PROVIDER *provider, * If we haven't been told not to store, * add to the global store */ - data->mcm->put(data->libctx, NULL, method, data->operation_id, - algo->algorithm_name, + data->mcm->put(data->libctx, NULL, method, provider, + data->operation_id, algo->algorithm_name, algo->property_definition, data->mcm_data); } - data->mcm->put(data->libctx, data->store, method, data->operation_id, - algo->algorithm_name, algo->property_definition, - data->mcm_data); + data->mcm->put(data->libctx, data->store, method, provider, + data->operation_id, algo->algorithm_name, + algo->property_definition, data->mcm_data); /* refcnt-- because we're dropping the reference */ data->mcm->destruct(method, data->mcm_data); diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c index 41dee721a4..fa85178a7e 100644 --- a/crypto/evp/evp_fetch.c +++ b/crypto/evp/evp_fetch.c @@ -114,9 +114,9 @@ static void *get_method_from_store(OPENSSL_CTX *libctx, void *store, } static int put_method_in_store(OPENSSL_CTX *libctx, void *store, - void *method, int operation_id, - const char *name, const char *propdef, - void *data) + void *method, const OSSL_PROVIDER *prov, + int operation_id, const char *name, + const char *propdef, void *data) { struct method_data_st *methdata = data; OSSL_NAMEMAP *namemap; @@ -132,7 +132,7 @@ static int put_method_in_store(OPENSSL_CTX *libctx, void *store, && (store = get_default_method_store(libctx)) == NULL) return 0; - return ossl_method_store_add(store, methid, propdef, method, + return ossl_method_store_add(store, prov, methid, propdef, method, methdata->refcnt_up_method, methdata->destruct_method); } diff --git a/crypto/property/property.c b/crypto/property/property.c index 6576ba0fd2..182ea6454b 100644 --- a/crypto/property/property.c +++ b/crypto/property/property.c @@ -25,6 +25,7 @@ #define IMPL_CACHE_FLUSH_THRESHOLD 500 typedef struct { + const OSSL_PROVIDER *provider; OSSL_PROPERTY_LIST *properties; void *method; void (*method_destruct)(void *); @@ -173,7 +174,7 @@ static int ossl_method_store_insert(OSSL_METHOD_STORE *store, ALGORITHM *alg) return ossl_sa_ALGORITHM_set(store->algs, alg->nid, alg); } -int ossl_method_store_add(OSSL_METHOD_STORE *store, +int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov, int nid, const char *properties, void *method, int (*method_up_ref)(void *), void (*method_destruct)(void *)) @@ -181,6 +182,7 @@ int ossl_method_store_add(OSSL_METHOD_STORE *store, ALGORITHM *alg = NULL; IMPLEMENTATION *impl; int ret = 0; + int i; if (nid <= 0 || method == NULL || store == NULL) return 0; @@ -191,8 +193,11 @@ int ossl_method_store_add(OSSL_METHOD_STORE *store, impl = OPENSSL_malloc(sizeof(*impl)); if (impl == NULL) return 0; - if (method_up_ref != NULL && !method_up_ref(method)) + if (method_up_ref != NULL && !method_up_ref(method)) { + OPENSSL_free(impl); return 0; + } + impl->provider = prov; impl->method = method; impl->method_destruct = method_destruct; @@ -222,8 +227,16 @@ int ossl_method_store_add(OSSL_METHOD_STORE *store, goto err; } - /* Push onto stack */ - if (sk_IMPLEMENTATION_push(alg->impls, impl)) + /* Push onto stack if there isn't one there already */ + for (i = 0; i < sk_IMPLEMENTATION_num(alg->impls); i++) { + const IMPLEMENTATION *tmpimpl = sk_IMPLEMENTATION_value(alg->impls, i); + + if (tmpimpl->provider == impl->provider + && tmpimpl->properties == impl->properties) + break; + } + if (i == sk_IMPLEMENTATION_num(alg->impls) + && sk_IMPLEMENTATION_push(alg->impls, impl)) ret = 1; ossl_property_unlock(store); if (ret == 0) diff --git a/doc/internal/man3/OSSL_METHOD_STORE.pod b/doc/internal/man3/OSSL_METHOD_STORE.pod index be439f15b5..afd1dd5982 100644 --- a/doc/internal/man3/OSSL_METHOD_STORE.pod +++ b/doc/internal/man3/OSSL_METHOD_STORE.pod @@ -19,7 +19,7 @@ ossl_method_store_cache_get, ossl_method_store_cache_set void ossl_method_store_free(OSSL_METHOD_STORE *store); int ossl_method_store_init(OPENSSL_CTX *ctx); void ossl_method_store_cleanup(OPENSSL_CTX *ctx); - int ossl_method_store_add(OSSL_METHOD_STORE *store, + int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov, int nid, const char *properties, void *method, int (*method_up_ref)(void *), void (*method_destruct)(void *)); @@ -63,8 +63,10 @@ B to allow access to the required underlying property data. ossl_method_store_free() frees resources allocated to B. -ossl_method_store_add() adds the B to the B as an instance of an -algorithm indicated by B and the property definition B. +ossl_method_store_add() adds the B constructed from an implementation in +the provider B to the B as an instance of an algorithm indicated by +B and the property definition B, unless the B already +has a method from the same provider with the same B and B. If the B function is given, it's called to increment the reference count of the method. If the B function is given, it's called when this function diff --git a/include/internal/core.h b/include/internal/core.h index bd2f9a0989..a40d3c69af 100644 --- a/include/internal/core.h +++ b/include/internal/core.h @@ -37,8 +37,8 @@ typedef struct ossl_method_construct_method_st { void *data); /* Store a method in a store */ int (*put)(OPENSSL_CTX *libctx, void *store, void *method, - int operation_id, const char *name, const char *propdef, - void *data); + const OSSL_PROVIDER *prov, int operation_id, const char *name, + const char *propdef, void *data); /* Construct a new method */ void *(*construct)(const char *name, const OSSL_DISPATCH *fns, OSSL_PROVIDER *prov, void *data); diff --git a/include/internal/property.h b/include/internal/property.h index e7e7f574e8..3c6d6a9002 100644 --- a/include/internal/property.h +++ b/include/internal/property.h @@ -18,12 +18,12 @@ typedef struct ossl_method_store_st OSSL_METHOD_STORE; /* Implementation store functions */ OSSL_METHOD_STORE *ossl_method_store_new(OPENSSL_CTX *ctx); void ossl_method_store_free(OSSL_METHOD_STORE *store); -int ossl_method_store_add(OSSL_METHOD_STORE *store, int nid, - const char *properties, void *implementation, - int (*implementation_up_ref)(void *), - void (*implementation_destruct)(void *)); -int ossl_method_store_remove(OSSL_METHOD_STORE *store, - int nid, const void *implementation); +int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov, + int nid, const char *properties, void *method, + int (*method_up_ref)(void *), + void (*method_destruct)(void *)); +int ossl_method_store_remove(OSSL_METHOD_STORE *store, int nid, + const void *method); int ossl_method_store_fetch(OSSL_METHOD_STORE *store, int nid, const char *prop_query, void **result); int ossl_method_store_set_global_properties(OSSL_METHOD_STORE *store, diff --git a/test/property_test.c b/test/property_test.c index a47dcfc09b..29e8ac3f51 100644 --- a/test/property_test.c +++ b/test/property_test.c @@ -240,8 +240,9 @@ static int test_register_deregister(void) goto err; for (i = 0; i < OSSL_NELEM(impls); i++) - if (!TEST_true(ossl_method_store_add(store, impls[i].nid, impls[i].prop, - impls[i].impl, NULL, NULL))) { + if (!TEST_true(ossl_method_store_add(store, NULL, impls[i].nid, + impls[i].prop, impls[i].impl, + NULL, NULL))) { TEST_note("iteration %zd", i + 1); goto err; } @@ -307,8 +308,9 @@ static int test_property(void) goto err; for (i = 0; i < OSSL_NELEM(impls); i++) - if (!TEST_true(ossl_method_store_add(store, impls[i].nid, impls[i].prop, - impls[i].impl, NULL, NULL))) { + if (!TEST_true(ossl_method_store_add(store, NULL, impls[i].nid, + impls[i].prop, impls[i].impl, + NULL, NULL))) { TEST_note("iteration %zd", i + 1); goto err; } @@ -347,7 +349,8 @@ static int test_query_cache_stochastic(void) for (i = 1; i <= max; i++) { v[i] = 2 * i; BIO_snprintf(buf, sizeof(buf), "n=%d\n", i); - if (!TEST_true(ossl_method_store_add(store, i, buf, "abc", NULL, NULL)) + if (!TEST_true(ossl_method_store_add(store, NULL, i, buf, "abc", + NULL, NULL)) || !TEST_true(ossl_method_store_cache_set(store, i, buf, v + i)) || !TEST_true(ossl_method_store_cache_set(store, i, "n=1234", "miss"))) { -- 2.34.1