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 <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15933)

2 files changed, 60 insertions, 15 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);