From: Richard Levitte Date: Fri, 7 Jun 2019 09:44:08 +0000 (+0200) Subject: EVP fetching: make operation_id part of the method identity X-Git-Tag: openssl-3.0.0-alpha1~1978 X-Git-Url: http://git.ipfire.org/?p=thirdparty%2Fopenssl.git;a=commitdiff_plain;h=2ccb1b4ecab2c3ac1dc2ff81a48869a79afa7839 EVP fetching: make operation_id part of the method identity Because the operation identity wasn't integrated with the created methods, the following code would give unexpected results: EVP_MD *md = EVP_MD_fetch(NULL, "MD5", NULL); EVP_CIPHER *cipher = EVP_CIPHER_fetch(NULL, "MD5", NULL); if (md != NULL) printf("MD5 is a digest\n"); if (cipher != NULL) printf("MD5 is a cipher\n"); The message is that MD5 is both a digest and a cipher. Partially fixes #9106 Reviewed-by: Shane Lontis (Merged from https://github.com/openssl/openssl/pull/9109) --- diff --git a/crypto/core_fetch.c b/crypto/core_fetch.c index a99f092486..56a3c5cadb 100644 --- a/crypto/core_fetch.c +++ b/crypto/core_fetch.c @@ -59,12 +59,12 @@ static int ossl_method_construct_this(OSSL_PROVIDER *provider, void *cbdata) * If we haven't been told not to store, * add to the global store */ - data->mcm->put(data->libctx, NULL, method, + data->mcm->put(data->libctx, NULL, method, data->operation_id, thismap->algorithm_name, thismap->property_definition, data->mcm_data); } - data->mcm->put(data->libctx, data->store, method, + data->mcm->put(data->libctx, data->store, method, data->operation_id, thismap->algorithm_name, thismap->property_definition, data->mcm_data); @@ -83,7 +83,8 @@ void *ossl_method_construct(OPENSSL_CTX *libctx, int operation_id, void *method = NULL; if ((method = - mcm->get(libctx, NULL, name, propquery, mcm_data)) == NULL) { + mcm->get(libctx, NULL, operation_id, name, propquery, mcm_data)) + == NULL) { struct construct_data_st cbdata; /* @@ -101,7 +102,8 @@ void *ossl_method_construct(OPENSSL_CTX *libctx, int operation_id, ossl_provider_forall_loaded(libctx, ossl_method_construct_this, &cbdata); - method = mcm->get(libctx, cbdata.store, name, propquery, mcm_data); + method = mcm->get(libctx, cbdata.store, operation_id, name, + propquery, mcm_data); mcm->dealloc_tmp_store(cbdata.store); } diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c index d3b5bcada2..1c9e27db87 100644 --- a/crypto/evp/evp_fetch.c +++ b/crypto/evp/evp_fetch.c @@ -39,7 +39,6 @@ static const OPENSSL_CTX_METHOD default_method_store_method = { struct method_data_st { OPENSSL_CTX *libctx; const char *name; - int id; OSSL_METHOD_CONSTRUCT_METHOD *mcm; void *(*method_from_dispatch)(const OSSL_DISPATCH *, OSSL_PROVIDER *); int (*refcnt_up_method)(void *method); @@ -66,24 +65,45 @@ static OSSL_METHOD_STORE *get_default_method_store(OPENSSL_CTX *libctx) &default_method_store_method); } +/* + * To identity the method in the method store, we mix the name identity + * with the operation identity, with the assumption that we don't have + * more than 2^24 names or more than 2^8 operation types. + * + * The resulting identity is a 32-bit integer, composed like this: + * + * +---------24 bits--------+-8 bits-+ + * | name identity | op id | + * +------------------------+--------+ + */ +static uint32_t method_id(unsigned int operation_id, unsigned int name_id) +{ + if (!ossl_assert(name_id < (1 << 24) || operation_id < (1 << 8)) + || !ossl_assert(name_id > 0 && operation_id > 0)) + return 0; + return ((name_id << 8) & 0xFFFFFF00) | (operation_id & 0x000000FF); +} + static void *get_method_from_store(OPENSSL_CTX *libctx, void *store, - const char *name, const char *propquery, - void *data) + int operation_id, const char *name, + const char *propquery, void *data) { struct method_data_st *methdata = data; void *method = NULL; OSSL_NAMEMAP *namemap; - int id; + int nameid; + uint32_t methid; if (store == NULL && (store = get_default_method_store(libctx)) == NULL) return NULL; if ((namemap = ossl_namemap_stored(libctx)) == NULL - || (id = ossl_namemap_add(namemap, name)) == 0) + || (nameid = ossl_namemap_add(namemap, name)) == 0 + || (methid = method_id(operation_id, nameid)) == 0) return NULL; - (void)ossl_method_store_fetch(store, id, propquery, &method); + (void)ossl_method_store_fetch(store, methid, propquery, &method); if (method != NULL && !methdata->refcnt_up_method(method)) { @@ -93,15 +113,18 @@ static void *get_method_from_store(OPENSSL_CTX *libctx, void *store, } static int put_method_in_store(OPENSSL_CTX *libctx, void *store, - void *method, const char *name, - const char *propdef, void *data) + void *method, int operation_id, + const char *name, const char *propdef, + void *data) { struct method_data_st *methdata = data; OSSL_NAMEMAP *namemap; - int id; + int nameid; + uint32_t methid; if ((namemap = ossl_namemap_stored(methdata->libctx)) == NULL - || (id = ossl_namemap_add(namemap, name)) == 0) + || (nameid = ossl_namemap_add(namemap, name)) == 0 + || (methid = method_id(operation_id, nameid)) == 0) return 0; if (store == NULL @@ -109,7 +132,7 @@ static int put_method_in_store(OPENSSL_CTX *libctx, void *store, return 0; if (methdata->refcnt_up_method(method) - && ossl_method_store_add(store, id, propdef, method, + && ossl_method_store_add(store, methid, propdef, method, methdata->destruct_method)) return 1; return 0; @@ -139,14 +162,32 @@ void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id, { OSSL_METHOD_STORE *store = get_default_method_store(libctx); OSSL_NAMEMAP *namemap = ossl_namemap_stored(libctx); - int id; + int nameid = 0; + uint32_t methid = 0; void *method = NULL; if (store == NULL || namemap == NULL) return NULL; - if ((id = ossl_namemap_number(namemap, name)) == 0 - || !ossl_method_store_cache_get(store, id, properties, &method)) { + /* + * If there's ever an operation_id == 0 passed, we have an internal + * programming error. + */ + if (!ossl_assert(operation_id > 0)) + return NULL; + + /* + * method_id returns 0 if we have too many operations (more than + * about 2^8) or too many names (more than about 2^24). In that + * case, we can't create any new method. + */ + if ((nameid = ossl_namemap_number(namemap, name)) != 0 + && (methid = method_id(operation_id, nameid)) == 0) + return NULL; + + if (nameid == 0 + || !ossl_method_store_cache_get(store, methid, properties, + &method)) { OSSL_METHOD_CONSTRUCT_METHOD mcm = { alloc_tmp_method_store, dealloc_tmp_method_store, @@ -164,10 +205,19 @@ void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id, mcmdata.destruct_method = free_method; mcmdata.refcnt_up_method = upref_method; mcmdata.destruct_method = free_method; - method = ossl_method_construct(libctx, operation_id, name, - properties, 0 /* !force_cache */, - &mcm, &mcmdata); - ossl_method_store_cache_set(store, id, properties, method); + if ((method = ossl_method_construct(libctx, operation_id, name, + properties, 0 /* !force_cache */, + &mcm, &mcmdata)) == NULL) { + /* + * If construction did create a method for us, we know that + * there is a correct nameid and methodid, since those have + * already been calculated in get_method_from_store() and + * put_method_in_store() above. + */ + nameid = ossl_namemap_number(namemap, name); + methid = method_id(operation_id, nameid); + ossl_method_store_cache_set(store, methid, properties, method); + } } else { upref_method(method); } diff --git a/doc/internal/man3/ossl_method_construct.pod b/doc/internal/man3/ossl_method_construct.pod index 60de260e4a..02ccc17574 100644 --- a/doc/internal/man3/ossl_method_construct.pod +++ b/doc/internal/man3/ossl_method_construct.pod @@ -15,11 +15,13 @@ OSSL_METHOD_CONSTRUCT_METHOD, ossl_method_construct /* Remove a store */ void (*dealloc_tmp_store)(void *store); /* Get an already existing method from a store */ - void *(*get)(OPENSSL_CTX *libctx, void *store, const char *name, - const char *propquery, void *data); + void *(*get)(OPENSSL_CTX *libctx, void *store, + int operation_id, const char *name, const char *propquery, + void *data); /* Store a method in a store */ int (*put)(OPENSSL_CTX *libctx, void *store, void *method, - const char *name, const char *propdef, void *data); + 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); @@ -41,11 +43,23 @@ on provider dispatch tables need to do so in exactly the same way. ossl_method_construct() does this while leaving it to the sub-systems to define more precisely how the methods are created, stored, etc. +It's important to keep in mind that a method is identified by three things: + +=over 4 + +=item The operation identity + +=item The name of the algorithm + +=item The properties associated with the algorithm implementation + +=back + =head2 Functions ossl_method_construct() creates a method by asking all available providers for a dispatch table given an C, an algorithm -C and a set of C, and then calling appropriate +C and a set of C, and then calling the appropriate functions given by the sub-system specific method creator through C and the data in C (which is passed by ossl_method_construct()). @@ -84,10 +98,10 @@ B is a valid value and means that a sub-system default store must be used. This default store should be stored in the library context C. -The method to be looked up should be identified with the given C and -data from C -(which is the C that was passed to ossl_construct_method()) -and the provided property query C. +The method to be looked up should be identified with the given +C, C, the provided property query C +and data from C (which is the C that was passed to +ossl_construct_method()). This function is expected to increment the method's reference count. @@ -101,9 +115,10 @@ B is a valid value and means that a sub-system default store must be used. This default store should be stored in the library context C. -The method should be associated with the given C and property definition -C as well as any identification data given through C (which is -the C that was passed to ossl_construct_method()). +The method should be associated with the given C, +C and property definition C as well as any +identification data given through C (which is the C +that was passed to ossl_construct_method()). This function is expected to increment the C's reference count. diff --git a/include/internal/core.h b/include/internal/core.h index 64547dca40..3f0cdfaeff 100644 --- a/include/internal/core.h +++ b/include/internal/core.h @@ -32,11 +32,13 @@ typedef struct ossl_method_construct_method_st { /* Remove a store */ void (*dealloc_tmp_store)(void *store); /* Get an already existing method from a store */ - void *(*get)(OPENSSL_CTX *libctx, void *store, const char *name, - const char *propquery, void *data); + void *(*get)(OPENSSL_CTX *libctx, void *store, + int operation_id, const char *name, const char *propquery, + void *data); /* Store a method in a store */ int (*put)(OPENSSL_CTX *libctx, void *store, void *method, - const char *name, const char *propdef, void *data); + 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);