Fix unbounded memory growth when using no-cached-fetch

When OpenSSL has been compiled with no-cached-fetch we do not cache
algorithms fetched from a provider. When we export an EVP_PKEY to a
provider we cache the details of that export in the operation cache for
that EVP_PKEY. Amoung the details we cache is the EVP_KEYMGMT that we used
for the export. When we come to reuse the key in the same provider that
we have previously exported the key to, we check the operation cache for
the cached key data. However because the EVP_KEYMGMT instance was not
cached then instance will be different every time and we were not
recognising that we had already exported the key to the provider.

This causes us to re-export the key to the same provider everytime the key
is used. Since this consumes memory we end up with unbounded memory growth.

The fix is to be more intelligent about recognising that we have already
exported key data to a given provider even if the EVP_KEYMGMT instance is
different.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/23841)

diff --git a/crypto/evp/keymgmt_lib.c b/crypto/evp/keymgmt_lib.c
index 6408076b1f3be45bcff8dd2911238441796c29ab..7029e8a0a4c66c2cdc15712b2ee7e1aa289d1314 100644 (file)
--- a/crypto/evp/keymgmt_lib.c
+++ b/crypto/evp/keymgmt_lib.c
@@ -239,10 +239,15 @@ OP_CACHE_ELEM *evp_keymgmt_util_find_operation_cache(EVP_PKEY *pk,
     /*
      * A comparison and sk_P_CACHE_ELEM_find() are avoided to not cause
      * problems when we've only a read lock.
+     * A keymgmt is a match if the |keymgmt| pointers are identical or if the
+     * provider and the name ID match
      */
     for (i = 0; i < end; i++) {
         p = sk_OP_CACHE_ELEM_value(pk->operation_cache, i);
-        if (keymgmt == p->keymgmt && (p->selection & selection) == selection)
+        if ((p->selection & selection) == selection
+                && (keymgmt == p->keymgmt
+                    || (keymgmt->name_id == p->keymgmt->name_id
+                        && keymgmt->prov == p->keymgmt->prov)))
             return p;
     }
     return NULL;

diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index 06a127a82068bed45001f44ca98d110ed7ccaeaa..f7893d453d332a7eaec95445c90c4ed19ea445cc 100644 (file)
--- a/crypto/evp/p_lib.c
+++ b/crypto/evp/p_lib.c
@@ -1916,7 +1916,15 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
              * If |tmp_keymgmt| is present in the operation cache, it means
              * that export doesn't need to be redone.  In that case, we take
              * token copies of the cached pointers, to have token success
-             * values to return.
+             * values to return. It is possible (e.g. in a no-cached-fetch
+             * build), for op->keymgmt to be a different pointer to tmp_keymgmt
+             * even though the name/provider must be the same. In other words
+             * the keymgmt instance may be different but still equivalent, i.e.
+             * same algorithm/provider instance - but we make the simplifying
+             * assumption that the keydata can be used with either keymgmt
+             * instance. Not doing so introduces significant complexity and
+             * probably requires refactoring - since we would have to ripple
+             * the change in keymgmt instance up the call chain.
              */
             if (op != NULL && op->keymgmt != NULL) {
                 keydata = op->keydata;