From 3ca9d210c94b9b88b89b224797aa403dfe97ccce Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Fri, 23 Aug 2019 14:03:28 +0200 Subject: [PATCH] Refactor how KEYMGMT methods get associated with other methods KEYMGMT methods were attached to other methods after those were fully created and registered, thereby creating a potential data race, if two threads tried to create the exact same method at the same time. Instead of this, we change the method creating function to take an extra data parameter, passed all the way from the public fetching function. In the case of EVP_KEYEXCH, we pass all the necessary data that evp_keyexch_from_dispatch() needs to be able to fetch the appropriate KEYMGMT method on the fly. Fixes #9592 Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/9678) --- crypto/err/openssl.txt | 1 + crypto/evp/digest.c | 6 +- crypto/evp/evp_enc.c | 7 ++- crypto/evp/evp_err.c | 2 + crypto/evp/evp_fetch.c | 21 ++++--- crypto/evp/evp_locl.h | 8 ++- crypto/evp/exchange.c | 74 +++++++++++++++---------- crypto/evp/keymgmt_meth.c | 4 +- crypto/evp/mac_meth.c | 6 +- doc/internal/man3/evp_generic_fetch.pod | 9 ++- include/openssl/evperr.h | 5 +- 11 files changed, 87 insertions(+), 56 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 58f6c4894f..9b682d5084 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2484,6 +2484,7 @@ EVP_R_NOT_XOF_OR_INVALID_LENGTH:178:not XOF or invalid length EVP_R_NO_CIPHER_SET:131:no cipher set EVP_R_NO_DEFAULT_DIGEST:158:no default digest EVP_R_NO_DIGEST_SET:139:no digest set +EVP_R_NO_KEYMGMT_AVAILABLE:199:no keymgmt available EVP_R_NO_KEYMGMT_PRESENT:196:no keymgmt present EVP_R_NO_KEY_SET:154:no key set EVP_R_NO_OPERATION_SET:149:no operation set diff --git a/crypto/evp/digest.c b/crypto/evp/digest.c index dc7f922a11..bb6d31bf4f 100644 --- a/crypto/evp/digest.c +++ b/crypto/evp/digest.c @@ -617,7 +617,7 @@ int EVP_MD_CTX_ctrl(EVP_MD_CTX *ctx, int cmd, int p1, void *p2) } static void *evp_md_from_dispatch(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov) + OSSL_PROVIDER *prov, void *unused) { EVP_MD *md = NULL; int fncnt = 0; @@ -744,7 +744,7 @@ EVP_MD *EVP_MD_fetch(OPENSSL_CTX *ctx, const char *algorithm, { EVP_MD *md = evp_generic_fetch(ctx, OSSL_OP_DIGEST, algorithm, properties, - evp_md_from_dispatch, evp_md_up_ref, + evp_md_from_dispatch, NULL, evp_md_up_ref, evp_md_free); return md; @@ -756,5 +756,5 @@ void EVP_MD_do_all_ex(OPENSSL_CTX *libctx, { evp_generic_do_all(libctx, OSSL_OP_DIGEST, (void (*)(void *, void *))fn, arg, - evp_md_from_dispatch, evp_md_free); + evp_md_from_dispatch, NULL, evp_md_free); } diff --git a/crypto/evp/evp_enc.c b/crypto/evp/evp_enc.c index 96a15ef897..142ffecfed 100644 --- a/crypto/evp/evp_enc.c +++ b/crypto/evp/evp_enc.c @@ -1246,7 +1246,8 @@ int EVP_CIPHER_CTX_copy(EVP_CIPHER_CTX *out, const EVP_CIPHER_CTX *in) static void *evp_cipher_from_dispatch(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov) + OSSL_PROVIDER *prov, + void *unused) { EVP_CIPHER *cipher = NULL; int fnciphcnt = 0, fnctxcnt = 0; @@ -1386,7 +1387,7 @@ EVP_CIPHER *EVP_CIPHER_fetch(OPENSSL_CTX *ctx, const char *algorithm, { EVP_CIPHER *cipher = evp_generic_fetch(ctx, OSSL_OP_CIPHER, algorithm, properties, - evp_cipher_from_dispatch, evp_cipher_up_ref, + evp_cipher_from_dispatch, NULL, evp_cipher_up_ref, evp_cipher_free); return cipher; @@ -1398,5 +1399,5 @@ void EVP_CIPHER_do_all_ex(OPENSSL_CTX *libctx, { evp_generic_do_all(libctx, OSSL_OP_CIPHER, (void (*)(void *, void *))fn, arg, - evp_cipher_from_dispatch, evp_cipher_free); + evp_cipher_from_dispatch, NULL, evp_cipher_free); } diff --git a/crypto/evp/evp_err.c b/crypto/evp/evp_err.c index 749f189be3..63174f98f6 100644 --- a/crypto/evp/evp_err.c +++ b/crypto/evp/evp_err.c @@ -99,6 +99,8 @@ static const ERR_STRING_DATA EVP_str_reasons[] = { {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_CIPHER_SET), "no cipher set"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_DEFAULT_DIGEST), "no default digest"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_DIGEST_SET), "no digest set"}, + {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_KEYMGMT_AVAILABLE), + "no keymgmt available"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_KEYMGMT_PRESENT), "no keymgmt present"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_KEY_SET), "no key set"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_OPERATION_SET), "no operation set"}, diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c index fa85178a7e..662195e4de 100644 --- a/crypto/evp/evp_fetch.c +++ b/crypto/evp/evp_fetch.c @@ -41,7 +41,8 @@ struct method_data_st { const char *name; OSSL_METHOD_CONSTRUCT_METHOD *mcm; void *(*method_from_dispatch)(const char *, const OSSL_DISPATCH *, - OSSL_PROVIDER *); + OSSL_PROVIDER *, void *); + void *method_data; int (*refcnt_up_method)(void *method); void (*destruct_method)(void *method); }; @@ -142,7 +143,8 @@ static void *construct_method(const char *name, const OSSL_DISPATCH *fns, { struct method_data_st *methdata = data; - return methdata->method_from_dispatch(name, fns, prov); + return methdata->method_from_dispatch(name, fns, prov, + methdata->method_data); } static void destruct_method(void *method, void *data) @@ -156,7 +158,9 @@ void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id, const char *name, const char *properties, void *(*new_method)(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov), + OSSL_PROVIDER *prov, + void *method_data), + void *method_data, int (*up_ref_method)(void *), void (*free_method)(void *)) { @@ -205,6 +209,7 @@ void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id, mcmdata.destruct_method = free_method; mcmdata.refcnt_up_method = up_ref_method; mcmdata.destruct_method = free_method; + mcmdata.method_data = method_data; if ((method = ossl_method_construct(libctx, operation_id, name, properties, 0 /* !force_cache */, &mcm, &mcmdata)) != NULL) { @@ -239,7 +244,7 @@ struct do_all_data_st { void (*user_fn)(void *method, void *arg); void *user_arg; void *(*new_method)(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov); + OSSL_PROVIDER *prov, void *method_data); void (*free_method)(void *); }; @@ -248,7 +253,7 @@ static void do_one(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *algo, { struct do_all_data_st *data = vdata; void *method = data->new_method(algo->algorithm_name, - algo->implementation, provider); + algo->implementation, provider, NULL); if (method != NULL) { data->user_fn(method, data->user_arg); @@ -261,7 +266,9 @@ void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id, void *user_arg, void *(*new_method)(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov), + OSSL_PROVIDER *prov, + void *method_data), + void *method_data, void (*free_method)(void *)) { struct do_all_data_st data; @@ -270,5 +277,5 @@ void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id, data.free_method = free_method; data.user_fn = user_fn; data.user_arg = user_arg; - ossl_algorithm_do_all(libctx, operation_id, NULL, do_one, &data); + ossl_algorithm_do_all(libctx, operation_id, method_data, do_one, &data); } diff --git a/crypto/evp/evp_locl.h b/crypto/evp/evp_locl.h index 3fd73212a4..a7b36dbc0e 100644 --- a/crypto/evp/evp_locl.h +++ b/crypto/evp/evp_locl.h @@ -141,7 +141,9 @@ void *evp_generic_fetch(OPENSSL_CTX *ctx, int operation_id, const char *algorithm, const char *properties, void *(*new_method)(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov), + OSSL_PROVIDER *prov, + void *method_data), + void *method_data, int (*up_ref_method)(void *), void (*free_method)(void *)); void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id, @@ -149,7 +151,9 @@ void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id, void *user_arg, void *(*new_method)(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov), + OSSL_PROVIDER *prov, + void *method_data), + void *method_data, void (*free_method)(void *)); /* Helper functions to avoid duplicating code */ diff --git a/crypto/evp/exchange.c b/crypto/evp/exchange.c index 20503b5f67..e69e4fd0b2 100644 --- a/crypto/evp/exchange.c +++ b/crypto/evp/exchange.c @@ -32,20 +32,45 @@ static EVP_KEYEXCH *evp_keyexch_new(OSSL_PROVIDER *prov) return exchange; } +struct keymgmt_data_st { + OPENSSL_CTX *ctx; + const char *properties; +}; + static void *evp_keyexch_from_dispatch(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov) + OSSL_PROVIDER *prov, + void *vkeymgmt_data) { + /* + * Key exchange cannot work without a key, and key management + * from the same provider to manage its keys. We therefore fetch + * a key management method using the same algorithm and properties + * and pass that down to evp_generic_fetch to be passed on to our + * evp_keyexch_from_dispatch, which will attach the key management + * method to the newly created key exchange method as long as the + * provider matches. + */ + struct keymgmt_data_st *keymgmt_data = vkeymgmt_data; + EVP_KEYMGMT *keymgmt = EVP_KEYMGMT_fetch(keymgmt_data->ctx, name, + keymgmt_data->properties); EVP_KEYEXCH *exchange = NULL; int fncnt = 0; + if (keymgmt == NULL || EVP_KEYMGMT_provider(keymgmt) != prov) { + ERR_raise(ERR_LIB_EVP, EVP_R_NO_KEYMGMT_AVAILABLE); + goto err; + } + if ((exchange = evp_keyexch_new(prov)) == NULL || (exchange->name = OPENSSL_strdup(name)) == NULL) { - EVP_KEYEXCH_free(exchange); - EVPerr(0, ERR_R_MALLOC_FAILURE); - return NULL; + ERR_raise(ERR_LIB_EVP, ERR_R_MALLOC_FAILURE); + goto err; } + exchange->keymgmt = keymgmt; + keymgmt = NULL; /* avoid double free on failure below */ + for (; fns->function_id != 0; fns++) { switch (fns->function_id) { case OSSL_FUNC_KEYEXCH_NEWCTX: @@ -96,13 +121,17 @@ static void *evp_keyexch_from_dispatch(const char *name, * and freectx. The dupctx, set_peer and set_params functions are * optional. */ - EVP_KEYEXCH_free(exchange); EVPerr(EVP_F_EVP_KEYEXCH_FROM_DISPATCH, EVP_R_INVALID_PROVIDER_FUNCTIONS); - return NULL; + goto err; } return exchange; + + err: + EVP_KEYEXCH_free(exchange); + EVP_KEYMGMT_free(keymgmt); + return NULL; } void EVP_KEYEXCH_free(EVP_KEYEXCH *exchange) @@ -137,31 +166,16 @@ OSSL_PROVIDER *EVP_KEYEXCH_provider(const EVP_KEYEXCH *exchange) EVP_KEYEXCH *EVP_KEYEXCH_fetch(OPENSSL_CTX *ctx, const char *algorithm, const char *properties) { - /* - * Key exchange cannot work without a key, and we key management - * from the same provider to manage its keys. - */ - EVP_KEYEXCH *keyexch = - evp_generic_fetch(ctx, OSSL_OP_KEYEXCH, algorithm, properties, - evp_keyexch_from_dispatch, - (int (*)(void *))EVP_KEYEXCH_up_ref, - (void (*)(void *))EVP_KEYEXCH_free); - - /* If the method is newly created, there's no keymgmt attached */ - if (keyexch->keymgmt == NULL) { - EVP_KEYMGMT *keymgmt = EVP_KEYMGMT_fetch(ctx, algorithm, properties); - - if (keymgmt == NULL - || (EVP_KEYEXCH_provider(keyexch) - != EVP_KEYMGMT_provider(keymgmt))) { - EVP_KEYEXCH_free(keyexch); - EVP_KEYMGMT_free(keymgmt); - EVPerr(EVP_F_EVP_KEYEXCH_FETCH, EVP_R_NO_KEYMGMT_PRESENT); - return NULL; - } + EVP_KEYEXCH *keyexch = NULL; + struct keymgmt_data_st keymgmt_data; + + keymgmt_data.ctx = ctx; + keymgmt_data.properties = properties; + keyexch = evp_generic_fetch(ctx, OSSL_OP_KEYEXCH, algorithm, properties, + evp_keyexch_from_dispatch, &keymgmt_data, + (int (*)(void *))EVP_KEYEXCH_up_ref, + (void (*)(void *))EVP_KEYEXCH_free); - keyexch->keymgmt = keymgmt; - } return keyexch; } diff --git a/crypto/evp/keymgmt_meth.c b/crypto/evp/keymgmt_meth.c index 67c33eb78b..72ef1bdb0c 100644 --- a/crypto/evp/keymgmt_meth.c +++ b/crypto/evp/keymgmt_meth.c @@ -34,7 +34,7 @@ static void *keymgmt_new(void) } static void *keymgmt_from_dispatch(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov) + OSSL_PROVIDER *prov, void *unused) { EVP_KEYMGMT *keymgmt = NULL; @@ -156,7 +156,7 @@ EVP_KEYMGMT *EVP_KEYMGMT_fetch(OPENSSL_CTX *ctx, const char *algorithm, { EVP_KEYMGMT *keymgmt = evp_generic_fetch(ctx, OSSL_OP_KEYMGMT, algorithm, properties, - keymgmt_from_dispatch, + keymgmt_from_dispatch, NULL, (int (*)(void *))EVP_KEYMGMT_up_ref, (void (*)(void *))EVP_KEYMGMT_free); diff --git a/crypto/evp/mac_meth.c b/crypto/evp/mac_meth.c index 1c0d6425f7..a317127e15 100644 --- a/crypto/evp/mac_meth.c +++ b/crypto/evp/mac_meth.c @@ -48,7 +48,7 @@ static void *evp_mac_new(void) } static void *evp_mac_from_dispatch(const char *name, const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov) + OSSL_PROVIDER *prov, void *unused) { EVP_MAC *mac = NULL; int fnmaccnt = 0, fnctxcnt = 0; @@ -154,7 +154,7 @@ EVP_MAC *EVP_MAC_fetch(OPENSSL_CTX *libctx, const char *algorithm, const char *properties) { return evp_generic_fetch(libctx, OSSL_OP_MAC, algorithm, properties, - evp_mac_from_dispatch, evp_mac_up_ref, + evp_mac_from_dispatch, NULL, evp_mac_up_ref, evp_mac_free); } @@ -205,5 +205,5 @@ void EVP_MAC_do_all_ex(OPENSSL_CTX *libctx, { evp_generic_do_all(libctx, OSSL_OP_MAC, (void (*)(void *, void *))fn, arg, - evp_mac_from_dispatch, evp_mac_free); + evp_mac_from_dispatch, NULL, evp_mac_free); } diff --git a/doc/internal/man3/evp_generic_fetch.pod b/doc/internal/man3/evp_generic_fetch.pod index 0688ac0170..b77391e386 100644 --- a/doc/internal/man3/evp_generic_fetch.pod +++ b/doc/internal/man3/evp_generic_fetch.pod @@ -11,8 +11,11 @@ evp_generic_fetch - generic algorithm fetcher and method creator for EVP void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id, const char *name, const char *properties, - void *(*new_method)(const OSSL_DISPATCH *fns, - OSSL_PROVIDER *prov), + void *(*new_method)(const char *name, + const OSSL_DISPATCH *fns, + OSSL_PROVIDER *prov, + void *method_data), + void *method_data, int (*up_ref_method)(void *), void (*free_method)(void *)); @@ -31,6 +34,8 @@ The three functions are supposed to: creates an internal method from function pointers found in the dispatch table C. +The algorithm I, provider I, and I are +also passed to be used as new_method() sees fit. =item up_ref_method() diff --git a/include/openssl/evperr.h b/include/openssl/evperr.h index 34966f84cd..714f170bd9 100644 --- a/include/openssl/evperr.h +++ b/include/openssl/evperr.h @@ -41,10 +41,6 @@ int ERR_load_EVP_strings(void); # define EVP_F_ARIA_GCM_INIT_KEY 0 # define EVP_F_ARIA_INIT_KEY 0 # define EVP_F_B64_NEW 0 -# define EVP_F_BLAKE2B_MAC_CTRL 0 -# define EVP_F_BLAKE2B_MAC_INIT 0 -# define EVP_F_BLAKE2S_MAC_CTRL 0 -# define EVP_F_BLAKE2S_MAC_INIT 0 # define EVP_F_CAMELLIA_INIT_KEY 0 # define EVP_F_CHACHA20_POLY1305_CTRL 0 # define EVP_F_CMLL_T4_INIT_KEY 0 @@ -218,6 +214,7 @@ int ERR_load_EVP_strings(void); # define EVP_R_NO_CIPHER_SET 131 # define EVP_R_NO_DEFAULT_DIGEST 158 # define EVP_R_NO_DIGEST_SET 139 +# define EVP_R_NO_KEYMGMT_AVAILABLE 199 # define EVP_R_NO_KEYMGMT_PRESENT 196 # define EVP_R_NO_KEY_SET 154 # define EVP_R_NO_OPERATION_SET 149 -- 2.25.1