From f616ad4b022b8afa8416a7d9e475d02c49164192 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Mon, 28 Jun 2021 05:37:22 +0200 Subject: ENCODER & DECODER: Make a tighter coupling between en/decoders and keymgmt If there are keymgmts and en/decoders from the same provider, try to combine them first. This avoids unnecessary export/import dances, and also tries to avoid issues where the keymgmt doesn't fully support exporting and importing, which we can assume will be the case for HSM protected keys. Fixes #15932 Reviewed-by: Tomas Mraz Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/15933) --- crypto/encode_decode/decoder_pkey.c | 51 ++++++++++++++++++++++++++----------- crypto/encode_decode/encoder_pkey.c | 24 +++++++++++++++++ crypto/evp/keymgmt_meth.c | 7 ++++- include/crypto/evp.h | 1 + 4 files changed, 67 insertions(+), 16 deletions(-) diff --git a/crypto/encode_decode/decoder_pkey.c b/crypto/encode_decode/decoder_pkey.c index cb66ee4617..0270ba2e70 100644 --- a/crypto/encode_decode/decoder_pkey.c +++ b/crypto/encode_decode/decoder_pkey.c @@ -58,6 +58,7 @@ struct decoder_pkey_data_st { OSSL_LIB_CTX *libctx; char *propq; + STACK_OF(EVP_KEYMGMT) *keymgmts; char *object_type; /* recorded object data type, may be NULL */ void **object; /* Where the result should end up */ }; @@ -69,7 +70,10 @@ static int decoder_construct_pkey(OSSL_DECODER_INSTANCE *decoder_inst, struct decoder_pkey_data_st *data = construct_data; OSSL_DECODER *decoder = OSSL_DECODER_INSTANCE_get_decoder(decoder_inst); void *decoderctx = OSSL_DECODER_INSTANCE_get_decoder_ctx(decoder_inst); + const OSSL_PROVIDER *decoder_prov = OSSL_DECODER_get0_provider(decoder); EVP_KEYMGMT *keymgmt = NULL; + const OSSL_PROVIDER *keymgmt_prov = NULL; + int i, end; /* * |object_ref| points to a provider reference to an object, its exact * contents entirely opaque to us, but may be passed to any provider @@ -103,13 +107,33 @@ static int decoder_construct_pkey(OSSL_DECODER_INSTANCE *decoder_inst, object_ref = p->data; object_ref_sz = p->data_size; - keymgmt = EVP_KEYMGMT_fetch(data->libctx, data->object_type, data->propq); + /* + * First, we try to find a keymgmt that comes from the same provider as + * the decoder that passed the params. + */ + end = sk_EVP_KEYMGMT_num(data->keymgmts); + for (i = 0; i < end; i++) { + keymgmt = sk_EVP_KEYMGMT_value(data->keymgmts, i); + keymgmt_prov = EVP_KEYMGMT_get0_provider(keymgmt); + + if (keymgmt_prov == decoder_prov + && evp_keymgmt_has_load(keymgmt) + && EVP_KEYMGMT_is_a(keymgmt, data->object_type)) + break; + } + if (i < end) { + /* To allow it to be freed further down */ + if (!EVP_KEYMGMT_up_ref(keymgmt)) + return 0; + } else { + keymgmt = EVP_KEYMGMT_fetch(data->libctx, + data->object_type, data->propq); + keymgmt_prov = EVP_KEYMGMT_get0_provider(keymgmt); + } if (keymgmt != NULL) { EVP_PKEY *pkey = NULL; void *keydata = NULL; - const OSSL_PROVIDER *keymgmt_prov = EVP_KEYMGMT_get0_provider(keymgmt); - const OSSL_PROVIDER *decoder_prov = OSSL_DECODER_get0_provider(decoder); /* * If the EVP_KEYMGMT and the OSSL_DECODER are from the @@ -164,6 +188,7 @@ static void decoder_clean_pkey_construct_arg(void *construct_data) struct decoder_pkey_data_st *data = construct_data; if (data != NULL) { + sk_EVP_KEYMGMT_pop_free(data->keymgmts, EVP_KEYMGMT_free); OPENSSL_free(data->propq); OPENSSL_free(data->object_type); OPENSSL_free(data); @@ -315,12 +340,12 @@ int ossl_decoder_ctx_setup_for_pkey(OSSL_DECODER_CTX *ctx, const char *propquery) { struct decoder_pkey_data_st *process_data = NULL; - STACK_OF(EVP_KEYMGMT) *keymgmts = NULL; STACK_OF(OPENSSL_CSTRING) *names = NULL; const char *input_type = ctx->start_input_type; const char *input_structure = ctx->input_structure; int ok = 0; int isecoid = 0; + int i, end; if (keytype != NULL && (strcmp(keytype, "id-ecPublicKey") == 0 @@ -342,7 +367,7 @@ int ossl_decoder_ctx_setup_for_pkey(OSSL_DECODER_CTX *ctx, if ((process_data = OPENSSL_zalloc(sizeof(*process_data))) == NULL || (propquery != NULL && (process_data->propq = OPENSSL_strdup(propquery)) == NULL) - || (keymgmts = sk_EVP_KEYMGMT_new_null()) == NULL + || (process_data->keymgmts = sk_EVP_KEYMGMT_new_null()) == NULL || (names = sk_OPENSSL_CSTRING_new_null()) == NULL) { ERR_raise(ERR_LIB_OSSL_DECODER, ERR_R_MALLOC_FAILURE); goto err; @@ -352,11 +377,13 @@ int ossl_decoder_ctx_setup_for_pkey(OSSL_DECODER_CTX *ctx, process_data->libctx = libctx; /* First, find all keymgmts to form goals */ - EVP_KEYMGMT_do_all_provided(libctx, collect_keymgmt, keymgmts); + EVP_KEYMGMT_do_all_provided(libctx, collect_keymgmt, + process_data->keymgmts); /* Then, we collect all the keymgmt names */ - while (sk_EVP_KEYMGMT_num(keymgmts) > 0) { - EVP_KEYMGMT *keymgmt = sk_EVP_KEYMGMT_shift(keymgmts); + end = sk_EVP_KEYMGMT_num(process_data->keymgmts); + for (i = 0; i < end; i++) { + EVP_KEYMGMT *keymgmt = sk_EVP_KEYMGMT_value(process_data->keymgmts, i); /* * If the key type is given by the caller, we only use the matching @@ -373,15 +400,10 @@ int ossl_decoder_ctx_setup_for_pkey(OSSL_DECODER_CTX *ctx, goto err; } } - - EVP_KEYMGMT_free(keymgmt); } - sk_EVP_KEYMGMT_free(keymgmts); - keymgmts = NULL; OSSL_TRACE_BEGIN(DECODER) { - int i, end = sk_OPENSSL_CSTRING_num(names); - + end = sk_OPENSSL_CSTRING_num(names); BIO_printf(trc_out, " Found %d keytypes (possibly with duplicates)", end); @@ -429,7 +451,6 @@ int ossl_decoder_ctx_setup_for_pkey(OSSL_DECODER_CTX *ctx, ok = 1; err: decoder_clean_pkey_construct_arg(process_data); - sk_EVP_KEYMGMT_pop_free(keymgmts, EVP_KEYMGMT_free); sk_OPENSSL_CSTRING_free(names); return ok; diff --git a/crypto/encode_decode/encoder_pkey.c b/crypto/encode_decode/encoder_pkey.c index 4a1ffb3b3e..109dfa80cd 100644 --- a/crypto/encode_decode/encoder_pkey.c +++ b/crypto/encode_decode/encoder_pkey.c @@ -78,6 +78,7 @@ struct collected_encoder_st { const OSSL_PROVIDER *keymgmt_prov; OSSL_ENCODER_CTX *ctx; + unsigned int flag_find_same_provider:1; int error_occurred; }; @@ -101,6 +102,15 @@ static void collect_encoder(OSSL_ENCODER *encoder, void *arg) const OSSL_PROVIDER *prov = OSSL_ENCODER_get0_provider(encoder); void *provctx = OSSL_PROVIDER_get0_provider_ctx(prov); + /* + * collect_encoder() is called in two passes, one where the encoders + * from the same provider as the keymgmt are looked up, and one where + * the other encoders are looked up. |data->flag_find_same_provider| + * tells us which pass we're in. + */ + if ((data->keymgmt_prov == prov) != data->flag_find_same_provider) + continue; + if (!OSSL_ENCODER_is_a(encoder, name) || (encoder->does_selection != NULL && !encoder->does_selection(provctx, data->ctx->selection)) @@ -257,7 +267,21 @@ static int ossl_encoder_ctx_setup_for_pkey(OSSL_ENCODER_CTX *ctx, encoder_data.error_occurred = 0; encoder_data.keymgmt_prov = prov; encoder_data.ctx = ctx; + + /* + * Place the encoders with the a different provider as the keymgmt + * last (the chain is processed in reverse order) + */ + encoder_data.flag_find_same_provider = 0; + OSSL_ENCODER_do_all_provided(libctx, collect_encoder, &encoder_data); + + /* + * Place the encoders with the same provider as the keymgmt first + * (the chain is processed in reverse order) + */ + encoder_data.flag_find_same_provider = 1; OSSL_ENCODER_do_all_provided(libctx, collect_encoder, &encoder_data); + sk_OPENSSL_CSTRING_free(keymgmt_data.names); if (encoder_data.error_occurred) { ERR_raise(ERR_LIB_OSSL_ENCODER, ERR_R_MALLOC_FAILURE); diff --git a/crypto/evp/keymgmt_meth.c b/crypto/evp/keymgmt_meth.c index ca357b60ef..47a0350cc2 100644 --- a/crypto/evp/keymgmt_meth.c +++ b/crypto/evp/keymgmt_meth.c @@ -370,10 +370,15 @@ void evp_keymgmt_gen_cleanup(const EVP_KEYMGMT *keymgmt, void *genctx) keymgmt->gen_cleanup(genctx); } +int evp_keymgmt_has_load(const EVP_KEYMGMT *keymgmt) +{ + return keymgmt != NULL && keymgmt->load != NULL; +} + void *evp_keymgmt_load(const EVP_KEYMGMT *keymgmt, const void *objref, size_t objref_sz) { - if (keymgmt->load != NULL) + if (evp_keymgmt_has_load(keymgmt)) return keymgmt->load(objref, objref_sz); return NULL; } diff --git a/include/crypto/evp.h b/include/crypto/evp.h index 8438fe0e99..16e55cd9a2 100644 --- a/include/crypto/evp.h +++ b/include/crypto/evp.h @@ -825,6 +825,7 @@ void *evp_keymgmt_gen(const EVP_KEYMGMT *keymgmt, void *genctx, OSSL_CALLBACK *cb, void *cbarg); void evp_keymgmt_gen_cleanup(const EVP_KEYMGMT *keymgmt, void *genctx); +int evp_keymgmt_has_load(const EVP_KEYMGMT *keymgmt); void *evp_keymgmt_load(const EVP_KEYMGMT *keymgmt, const void *objref, size_t objref_sz); -- cgit v1.2.3