DECODER EVP_PKEY: Don't store all the EVP_KEYMGMTs

OSSL_DECODER_CTX_new_by_EVP_PKEY() would keep copies of all the
EVP_KEYMGMTs it finds.
This turns out to be fragile in certain circumstances, so we switch to
fetch the appropriate EVP_KEYMGMT when it's time to construct an
EVP_PKEY from the decoded data instead.  This has the added benefit
that we now actually use the property query string that was given by
the caller for these fetches.

Fixes #13503

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/13661)

1 files changed, 100 insertions, 134 deletions

diff --git a/crypto/encode_decode/decoder_pkey.c b/crypto/encode_decode/decoder_pkey.c
index 016d6047bd..c515cb6d44 100644
--- a/crypto/encode_decode/decoder_pkey.c
+++ b/crypto/encode_decode/decoder_pkey.c
@@ -55,9 +55,11 @@ int OSSL_DECODER_CTX_set_passphrase_cb(OSSL_DECODER_CTX *ctx,
 DEFINE_STACK_OF(EVP_KEYMGMT)
 
 struct decoder_EVP_PKEY_data_st {
+    OSSL_LIB_CTX *libctx;
+    char *propq;
+
     char *object_type;           /* recorded object data type, may be NULL */
     void **object;               /* Where the result should end up */
-    STACK_OF(EVP_KEYMGMT) *keymgmts; /* The EVP_KEYMGMTs we handle */
 };
 
 static int decoder_construct_EVP_PKEY(OSSL_DECODER_INSTANCE *decoder_inst,
@@ -67,7 +69,7 @@ static int decoder_construct_EVP_PKEY(OSSL_DECODER_INSTANCE *decoder_inst,
     struct decoder_EVP_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);
-    size_t i, end_i;
+    EVP_KEYMGMT *keymgmt = NULL;
     /*
      * |object_ref| points to a provider reference to an object, its exact
      * contents entirely opaque to us, but may be passed to any provider
@@ -101,75 +103,54 @@ static int decoder_construct_EVP_PKEY(OSSL_DECODER_INSTANCE *decoder_inst,
     object_ref = p->data;
     object_ref_sz = p->data_size;
 
-    /* We may have reached one of the goals, let's find out! */
-    end_i = sk_EVP_KEYMGMT_num(data->keymgmts);
-    for (i = 0; end_i; i++) {
-        EVP_KEYMGMT *keymgmt = sk_EVP_KEYMGMT_value(data->keymgmts, i);
+    keymgmt = EVP_KEYMGMT_fetch(data->libctx, data->object_type, data->propq);
+
+    if (keymgmt != NULL) {
+        EVP_PKEY *pkey = NULL;
+        void *keydata = NULL;
+        const OSSL_PROVIDER *keymgmt_prov = EVP_KEYMGMT_provider(keymgmt);
+        const OSSL_PROVIDER *decoder_prov = OSSL_DECODER_provider(decoder);
 
         /*
-         * There are two ways to find a matching KEYMGMT:
-         *
-         * 1.  If the object data type (recorded in |data->object_type|)
-         *     is defined, by checking it using EVP_KEYMGMT_is_a().
-         * 2.  If the object data type is NOT defined, by comparing the
-         *     EVP_KEYMGMT and OSSL_DECODER method numbers.  Since
-         *     EVP_KEYMGMT and OSSL_DECODE operate with the same
-         *     namemap, we know that the method numbers must match.
+         * If the EVP_KEYMGMT and the OSSL_DECODER are from the
+         * same provider, we assume that the KEYMGMT has a key loading
+         * function that can handle the provider reference we hold.
          *
-         * This allows individual decoders to specify variants of keys,
-         * such as a DER to RSA decoder finding a RSA-PSS key, without
-         * having to decode the exact same DER blob into the exact same
-         * internal structure twice.  This is, of course, entirely at the
-         * discretion of the decoder implementations.
+         * Otherwise, we export from the decoder and import the
+         * result in the keymgmt.
          */
-        if (data->object_type != NULL
-            ? EVP_KEYMGMT_is_a(keymgmt, data->object_type)
-            : EVP_KEYMGMT_number(keymgmt) == OSSL_DECODER_number(decoder)) {
-            EVP_PKEY *pkey = NULL;
-            void *keydata = NULL;
-            const OSSL_PROVIDER *keymgmt_prov =
-                EVP_KEYMGMT_provider(keymgmt);
-            const OSSL_PROVIDER *decoder_prov =
-                OSSL_DECODER_provider(decoder);
+        if (keymgmt_prov == decoder_prov) {
+            keydata = evp_keymgmt_load(keymgmt, object_ref, object_ref_sz);
+        } else {
+            struct evp_keymgmt_util_try_import_data_st import_data;
+
+            import_data.keymgmt = keymgmt;
+            import_data.keydata = NULL;
+            import_data.selection = OSSL_KEYMGMT_SELECT_ALL;
 
             /*
-             * If the EVP_KEYMGMT and the OSSL_DECODER are from the
-             * same provider, we assume that the KEYMGMT has a key loading
-             * function that can handle the provider reference we hold.
-             *
-             * Otherwise, we export from the decoder and import the
-             * result in the keymgmt.
+             * No need to check for errors here, the value of
+             * |import_data.keydata| is as much an indicator.
              */
-            if (keymgmt_prov == decoder_prov) {
-                keydata = evp_keymgmt_load(keymgmt, object_ref, object_ref_sz);
-            } else {
-                struct evp_keymgmt_util_try_import_data_st import_data;
-
-                import_data.keymgmt = keymgmt;
-                import_data.keydata = NULL;
-                import_data.selection = OSSL_KEYMGMT_SELECT_ALL;
-
-                /*
-                 * No need to check for errors here, the value of
-                 * |import_data.keydata| is as much an indicator.
-                 */
-                (void)decoder->export_object(decoderctx,
-                                             object_ref, object_ref_sz,
-                                             &evp_keymgmt_util_try_import,
-                                             &import_data);
-                keydata = import_data.keydata;
-                import_data.keydata = NULL;
-            }
-
-            if (keydata != NULL
-                && (pkey =
-                    evp_keymgmt_util_make_pkey(keymgmt, keydata)) == NULL)
-                evp_keymgmt_freedata(keymgmt, keydata);
-
-            *data->object = pkey;
-
-            break;
+            (void)decoder->export_object(decoderctx,
+                                         object_ref, object_ref_sz,
+                                         &evp_keymgmt_util_try_import,
+                                         &import_data);
+            keydata = import_data.keydata;
+            import_data.keydata = NULL;
         }
+
+        if (keydata != NULL
+            && (pkey = evp_keymgmt_util_make_pkey(keymgmt, keydata)) == NULL)
+            evp_keymgmt_freedata(keymgmt, keydata);
+
+        *data->object = pkey;
+
+        /*
+         * evp_keymgmt_util_make_pkey() increments the reference count when
+         * assigning the EVP_PKEY, so we can free the keymgmt here.
+         */
+        EVP_KEYMGMT_free(keymgmt);
     }
     /*
      * We successfully looked through, |*ctx->object| determines if we
@@ -183,63 +164,37 @@ static void decoder_clean_EVP_PKEY_construct_arg(void *construct_data)
     struct decoder_EVP_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);
     }
 }
 
-struct collected_data_st {
-    struct decoder_EVP_PKEY_data_st *process_data;
-    const char *keytype;
-    STACK_OF(OPENSSL_CSTRING) *names;
-    OSSL_DECODER_CTX *ctx;
+static void collect_name(const char *name, void *arg)
+{
+    STACK_OF(OPENSSL_CSTRING) *names = arg;
 
-    unsigned int error_occured:1;
-};
+    sk_OPENSSL_CSTRING_push(names, name);
+}
 
 static void collect_keymgmt(EVP_KEYMGMT *keymgmt, void *arg)
 {
-    struct collected_data_st *data = arg;
-
-    if (data->keytype != NULL && !EVP_KEYMGMT_is_a(keymgmt, data->keytype))
-        return;
-    if (data->error_occured)
-        return;
-
-    data->error_occured = 1;         /* Assume the worst */
+    STACK_OF(EVP_KEYMGMT) *keymgmts = arg;
 
     if (!EVP_KEYMGMT_up_ref(keymgmt) /* ref++ */)
         return;
-    if (sk_EVP_KEYMGMT_push(data->process_data->keymgmts, keymgmt) <= 0) {
+    if (sk_EVP_KEYMGMT_push(keymgmts, keymgmt) <= 0) {
         EVP_KEYMGMT_free(keymgmt);   /* ref-- */
         return;
     }
-
-    data->error_occured = 0;         /* All is good now */
-}
-
-static void collect_name(const char *name, void *arg)
-{
-    struct collected_data_st *data = arg;
-
-    if (data->error_occured)
-        return;
-
-    data->error_occured = 1;         /* Assume the worst */
-
-    if (sk_OPENSSL_CSTRING_push(data->names, name) <= 0)
-        return;
-
-    data->error_occured = 0;         /* All is good now */
 }
 
 /*
  * The input structure check is only done on the initial decoder
  * implementations.
  */
-static int collect_decoder_check_input_structure(OSSL_DECODER_CTX *ctx,
-                                                 OSSL_DECODER_INSTANCE *di)
+static int decoder_check_input_structure(OSSL_DECODER_CTX *ctx,
+                                         OSSL_DECODER_INSTANCE *di)
 {
     int di_is_was_set = 0;
     const char *di_is =
@@ -255,15 +210,21 @@ static int collect_decoder_check_input_structure(OSSL_DECODER_CTX *ctx,
      * If the caller did give an input structure name, the decoder must have
      * a matching input structure to be accepted.
      */
-    if (di_is != NULL
-        && strcasecmp(ctx->input_structure, di_is) == 0)
+    if (di_is != NULL && strcasecmp(ctx->input_structure, di_is) == 0)
         return 1;
     return 0;
 }
 
+struct collect_decoder_data_st {
+    STACK_OF(OPENSSL_CSTRING) *names;
+    OSSL_DECODER_CTX *ctx;
+
+    unsigned int error_occured:1;
+};
+
 static void collect_decoder(OSSL_DECODER *decoder, void *arg)
 {
-    struct collected_data_st *data = arg;
+    struct collect_decoder_data_st *data = arg;
     size_t i, end_i;
     const OSSL_PROVIDER *prov = OSSL_DECODER_provider(decoder);
     void *provctx = OSSL_PROVIDER_get0_provider_ctx(prov);
@@ -295,7 +256,7 @@ static void collect_decoder(OSSL_DECODER *decoder, void *arg)
             /* If successful so far, don't free these directly */
             decoderctx = NULL;
 
-            if (collect_decoder_check_input_structure(data->ctx, di)
+            if (decoder_check_input_structure(data->ctx, di)
                 && ossl_decoder_ctx_add_decoder_inst(data->ctx, di))
                 di = NULL;      /* If successfully added, don't free it */
         }
@@ -313,66 +274,71 @@ int ossl_decoder_ctx_setup_for_EVP_PKEY(OSSL_DECODER_CTX *ctx,
                                         OSSL_LIB_CTX *libctx,
                                         const char *propquery)
 {
-    struct collected_data_st *data = NULL;
-    size_t i, end_i;
+    struct decoder_EVP_PKEY_data_st *process_data = NULL;
+    STACK_OF(EVP_KEYMGMT) *keymgmts = NULL;
+    STACK_OF(OPENSSL_CSTRING) *names = NULL;
     int ok = 0;
 
-    if ((data = OPENSSL_zalloc(sizeof(*data))) == NULL
-        || (data->process_data =
-            OPENSSL_zalloc(sizeof(*data->process_data))) == NULL
-        || (data->process_data->keymgmts = sk_EVP_KEYMGMT_new_null()) == NULL
-        || (data->names = sk_OPENSSL_CSTRING_new_null()) == NULL) {
+    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
+        || (names = sk_OPENSSL_CSTRING_new_null()) == NULL) {
         ERR_raise(ERR_LIB_OSSL_DECODER, ERR_R_MALLOC_FAILURE);
         goto err;
     }
-    data->process_data->object = (void **)pkey;
-    data->ctx = ctx;
-    data->keytype = keytype;
 
-    /* First, find all keymgmts to form goals */
-    EVP_KEYMGMT_do_all_provided(libctx, collect_keymgmt, data);
+    process_data->object = (void **)pkey;
+    process_data->libctx = libctx;
 
-    if (data->error_occured)
-        goto err;
+    /* First, find all keymgmts to form goals */
+    EVP_KEYMGMT_do_all_provided(libctx, collect_keymgmt, keymgmts);
 
     /* Then, we collect all the keymgmt names */
-    end_i = sk_EVP_KEYMGMT_num(data->process_data->keymgmts);
-    for (i = 0; i < end_i; i++) {
-        EVP_KEYMGMT *keymgmt =
-            sk_EVP_KEYMGMT_value(data->process_data->keymgmts, i);
+    while (sk_EVP_KEYMGMT_num(keymgmts) > 0) {
+        EVP_KEYMGMT *keymgmt = sk_EVP_KEYMGMT_shift(keymgmts);
 
-        EVP_KEYMGMT_names_do_all(keymgmt, collect_name, data);
+        /*
+         * If the key type is given by the caller, we only use the matching
+         * KEYMGMTs, otherwise we use them all.
+         */
+        if (keytype == NULL || EVP_KEYMGMT_is_a(keymgmt, keytype))
+            EVP_KEYMGMT_names_do_all(keymgmt, collect_name, names);
 
-        if (data->error_occured)
-            goto err;
+        EVP_KEYMGMT_free(keymgmt);
     }
+    sk_EVP_KEYMGMT_free(keymgmts);
 
     /*
      * Finally, find all decoders that have any keymgmt of the collected
      * keymgmt names
      */
-    OSSL_DECODER_do_all_provided(libctx, collect_decoder, data);
+    {
+        struct collect_decoder_data_st collect_decoder_data = { NULL, };
 
-    if (data->error_occured)
-        goto err;
+        collect_decoder_data.names = names;
+        collect_decoder_data.ctx = ctx;
+        OSSL_DECODER_do_all_provided(libctx,
+                                     collect_decoder, &collect_decoder_data);
+        sk_OPENSSL_CSTRING_free(names);
+
+        if (collect_decoder_data.error_occured)
+            goto err;
+    }
 
     if (OSSL_DECODER_CTX_get_num_decoders(ctx) != 0) {
         if (!OSSL_DECODER_CTX_set_construct(ctx, decoder_construct_EVP_PKEY)
-            || !OSSL_DECODER_CTX_set_construct_data(ctx, data->process_data)
+            || !OSSL_DECODER_CTX_set_construct_data(ctx, process_data)
             || !OSSL_DECODER_CTX_set_cleanup(ctx,
                                              decoder_clean_EVP_PKEY_construct_arg))
             goto err;
 
-        data->process_data = NULL; /* Avoid it being freed */
+        process_data = NULL; /* Avoid it being freed */
     }
 
     ok = 1;
  err:
-    if (data != NULL) {
-        decoder_clean_EVP_PKEY_construct_arg(data->process_data);
-        sk_OPENSSL_CSTRING_free(data->names);
-        OPENSSL_free(data);
-    }
+    decoder_clean_EVP_PKEY_construct_arg(process_data);
     return ok;
 }