Cache legacy keys instead of downgrading them

If someone calls an EVP_PKEY_get0*() function then we create a legacy
key and cache it in the EVP_PKEY - but it doesn't become an "origin" and
it doesn't ever get updated. This will be documented as a restriction of
the EVP_PKEY_get0*() function with provided keys.

Fixes #14020

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14319)

7 files changed, 121 insertions, 166 deletions

diff --git a/crypto/evp/p_legacy.c b/crypto/evp/p_legacy.c
index 5d8468f949..e478814065 100644
--- a/crypto/evp/p_legacy.c
+++ b/crypto/evp/p_legacy.c
@@ -33,15 +33,11 @@ int EVP_PKEY_set1_RSA(EVP_PKEY *pkey, RSA *key)
 
 RSA *EVP_PKEY_get0_RSA(const EVP_PKEY *pkey)
 {
-    if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {
-        ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);
-        return NULL;
-    }
     if (pkey->type != EVP_PKEY_RSA && pkey->type != EVP_PKEY_RSA_PSS) {
         ERR_raise(ERR_LIB_EVP, EVP_R_EXPECTING_AN_RSA_KEY);
         return NULL;
     }
-    return pkey->pkey.rsa;
+    return evp_pkey_get_legacy((EVP_PKEY *)pkey);
 }
 
 RSA *EVP_PKEY_get1_RSA(EVP_PKEY *pkey)
@@ -65,15 +61,11 @@ int EVP_PKEY_set1_EC_KEY(EVP_PKEY *pkey, EC_KEY *key)
 
 EC_KEY *EVP_PKEY_get0_EC_KEY(const EVP_PKEY *pkey)
 {
-    if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {
-        ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);
-        return NULL;
-    }
     if (EVP_PKEY_base_id(pkey) != EVP_PKEY_EC) {
         EVPerr(EVP_F_EVP_PKEY_GET0_EC_KEY, EVP_R_EXPECTING_A_EC_KEY);
         return NULL;
     }
-    return pkey->pkey.ec;
+    return evp_pkey_get_legacy((EVP_PKEY *)pkey);
 }
 
 EC_KEY *EVP_PKEY_get1_EC_KEY(EVP_PKEY *pkey)
diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index ef38c5e333..2a78e6095a 100644
--- a/crypto/evp/p_lib.c
+++ b/crypto/evp/p_lib.c
@@ -13,6 +13,7 @@
  */
 #include "internal/deprecated.h"
 
+#include <assert.h>
 #include <stdio.h>
 #include "internal/cryptlib.h"
 #include "internal/refcount.h"
@@ -740,11 +741,8 @@ void *EVP_PKEY_get0(const EVP_PKEY *pkey)
 {
     if (pkey == NULL)
         return NULL;
-    if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {
-        ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);
-        return NULL;
-    }
-    return pkey->pkey.ptr;
+
+    return evp_pkey_get_legacy((EVP_PKEY *)pkey);
 }
 
 const unsigned char *EVP_PKEY_get0_hmac(const EVP_PKEY *pkey, size_t *len)
@@ -791,15 +789,11 @@ const unsigned char *EVP_PKEY_get0_siphash(const EVP_PKEY *pkey, size_t *len)
 # ifndef OPENSSL_NO_DSA
 DSA *EVP_PKEY_get0_DSA(const EVP_PKEY *pkey)
 {
-    if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {
-        ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);
-        return NULL;
-    }
     if (pkey->type != EVP_PKEY_DSA) {
         ERR_raise(ERR_LIB_EVP, EVP_R_EXPECTING_A_DSA_KEY);
         return NULL;
     }
-    return pkey->pkey.dsa;
+    return evp_pkey_get_legacy((EVP_PKEY *)pkey);
 }
 
 int EVP_PKEY_set1_DSA(EVP_PKEY *pkey, DSA *key)
@@ -823,15 +817,11 @@ DSA *EVP_PKEY_get1_DSA(EVP_PKEY *pkey)
 # ifndef OPENSSL_NO_EC
 static ECX_KEY *evp_pkey_get0_ECX_KEY(const EVP_PKEY *pkey, int type)
 {
-    if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {
-        ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);
-        return NULL;
-    }
     if (EVP_PKEY_base_id(pkey) != type) {
         ERR_raise(ERR_LIB_EVP, EVP_R_EXPECTING_A_ECX_KEY);
         return NULL;
     }
-    return pkey->pkey.ecx;
+    return evp_pkey_get_legacy((EVP_PKEY *)pkey);
 }
 
 static ECX_KEY *evp_pkey_get1_ECX_KEY(EVP_PKEY *pkey, int type)
@@ -868,15 +858,11 @@ int EVP_PKEY_set1_DH(EVP_PKEY *pkey, DH *key)
 
 DH *EVP_PKEY_get0_DH(const EVP_PKEY *pkey)
 {
-    if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {
-        ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);
-        return NULL;
-    }
     if (pkey->type != EVP_PKEY_DH && pkey->type != EVP_PKEY_DHX) {
         ERR_raise(ERR_LIB_EVP, EVP_R_EXPECTING_A_DH_KEY);
         return NULL;
     }
-    return pkey->pkey.dh;
+    return evp_pkey_get_legacy((EVP_PKEY *)pkey);
 }
 
 DH *EVP_PKEY_get1_DH(EVP_PKEY *pkey)
@@ -1310,36 +1296,6 @@ size_t EVP_PKEY_get1_encoded_public_key(EVP_PKEY *pkey, unsigned char **ppub)
 
 /*- All methods below can also be used in FIPS_MODULE */
 
-/*
- * This reset function must be used very carefully, as it literally throws
- * away everything in an EVP_PKEY without freeing them, and may cause leaks
- * of memory, what have you.
- * The only reason we have this is to have the same code for EVP_PKEY_new()
- * and evp_pkey_downgrade().
- */
-static int evp_pkey_reset_unlocked(EVP_PKEY *pk)
-{
-    if (pk == NULL)
-        return 0;
-
-    if (pk->lock != NULL) {
-      const size_t offset = (unsigned char *)&pk->lock - (unsigned char *)pk;
-
-      memset(pk, 0, offset);
-      memset((unsigned char *)pk + offset + sizeof(pk->lock),
-             0,
-             sizeof(*pk) - offset - sizeof(pk->lock));
-    }
-    /* EVP_PKEY_new uses zalloc so no need to call memset if pk->lock is NULL */
-
-    pk->type = EVP_PKEY_NONE;
-    pk->save_type = EVP_PKEY_NONE;
-    pk->references = 1;
-    pk->save_parameters = 1;
-
-    return 1;
-}
-
 EVP_PKEY *EVP_PKEY_new(void)
 {
     EVP_PKEY *ret = OPENSSL_zalloc(sizeof(*ret));
@@ -1349,8 +1305,10 @@ EVP_PKEY *EVP_PKEY_new(void)
         return NULL;
     }
 
-    if (!evp_pkey_reset_unlocked(ret))
-        goto err;
+    ret->type = EVP_PKEY_NONE;
+    ret->save_type = EVP_PKEY_NONE;
+    ret->references = 1;
+    ret->save_parameters = 1;
 
     ret->lock = CRYPTO_THREAD_lock_new();
     if (ret->lock == NULL) {
@@ -1559,12 +1517,32 @@ int EVP_PKEY_up_ref(EVP_PKEY *pkey)
 #ifndef FIPS_MODULE
 void evp_pkey_free_legacy(EVP_PKEY *x)
 {
-    if (x->ameth != NULL) {
-        if (x->ameth->pkey_free != NULL)
-            x->ameth->pkey_free(x);
+    const EVP_PKEY_ASN1_METHOD *ameth = x->ameth;
+    ENGINE *tmpe = NULL;
+
+    if (ameth == NULL && x->legacy_cache_pkey.ptr != NULL)
+        ameth = EVP_PKEY_asn1_find(&tmpe, x->type);
+
+    if (ameth != NULL) {
+        if (x->legacy_cache_pkey.ptr != NULL) {
+            /*
+             * We should never have both a legacy origin key, and a key in the
+             * legacy cache.
+             */
+            assert(x->pkey.ptr == NULL);
+            /*
+             * For the purposes of freeing we make the legacy cache look like
+             * a legacy origin key.
+             */
+            x->pkey = x->legacy_cache_pkey;
+            x->legacy_cache_pkey.ptr = NULL;
+        }
+        if (ameth->pkey_free != NULL)
+            ameth->pkey_free(x);
         x->pkey.ptr = NULL;
     }
 # ifndef OPENSSL_NO_ENGINE
+    ENGINE_finish(tmpe);
     ENGINE_finish(x->engine);
     x->engine = NULL;
     ENGINE_finish(x->pmeth_engine);
@@ -1877,78 +1855,57 @@ int evp_pkey_copy_downgraded(EVP_PKEY **dest, const EVP_PKEY *src)
     return 0;
 }
 
-int evp_pkey_downgrade(EVP_PKEY *pk)
+void *evp_pkey_get_legacy(EVP_PKEY *pk)
 {
-    EVP_PKEY tmp_copy;              /* Stack allocated! */
-    int rv = 0;
+    EVP_PKEY *tmp_copy = NULL;
+    void *ret = NULL;
 
     if (!ossl_assert(pk != NULL))
-        return 0;
+        return NULL;
 
     /*
-     * Throughout this whole function, we must ensure that we lock / unlock
-     * the exact same lock.  Note that we do pass it around a bit.
+     * If this isn't an assigned provider side key, we just use any existing
+     * origin legacy key.
      */
-    if (!CRYPTO_THREAD_write_lock(pk->lock))
-        return 0;
+    if (!evp_pkey_is_assigned(pk))
+        return NULL;
+    if (!evp_pkey_is_provided(pk))
+        return pk->pkey.ptr;
 
-    /* If this isn't an assigned provider side key, we're done */
-    if (!evp_pkey_is_assigned(pk) || !evp_pkey_is_provided(pk)) {
-        rv = 1;
-        goto end;
-    }
+    if (!CRYPTO_THREAD_read_lock(pk->lock))
+        return NULL;
 
-    /*
-     * To be able to downgrade, we steal the contents of |pk|, then reset
-     * it, and finally try to make it a downgraded copy.  If any of that
-     * fails, we restore the copied contents into |pk|.
-     */
-    tmp_copy = *pk;              /* |tmp_copy| now owns THE lock */
+    ret = pk->legacy_cache_pkey.ptr;
 
-    if (evp_pkey_reset_unlocked(pk)
-        && evp_pkey_copy_downgraded(&pk, &tmp_copy)) {
+    if (!CRYPTO_THREAD_unlock(pk->lock))
+        return NULL;
 
-        /* Restore the common attributes, then empty |tmp_copy| */
-        pk->references = tmp_copy.references;
-        pk->attributes = tmp_copy.attributes;
-        pk->save_parameters = tmp_copy.save_parameters;
-        pk->ex_data = tmp_copy.ex_data;
+    if (ret != NULL)
+        return ret;
 
-        /* Ensure that stuff we've copied won't be freed */
-        tmp_copy.lock = NULL;
-        tmp_copy.attributes = NULL;
-        memset(&tmp_copy.ex_data, 0, sizeof(tmp_copy.ex_data));
+    if (!evp_pkey_copy_downgraded(&tmp_copy, pk))
+        return NULL;
 
-        /*
-         * Save the provider side data in the operation cache, so they'll
-         * find it again.  |pk| is new, so it's safe to assume slot zero
-         * is free.
-         * Note that evp_keymgmt_util_cache_keydata() increments keymgmt's
-         * reference count, so we need to decrement it, or there will be a
-         * leak.
-         */
-        evp_keymgmt_util_cache_keydata(pk, tmp_copy.keymgmt,
-                                       tmp_copy.keydata);
-        EVP_KEYMGMT_free(tmp_copy.keymgmt);
+    if (!CRYPTO_THREAD_write_lock(pk->lock))
+        goto err;
 
-        /*
-         * Clear keymgmt and keydata from |tmp_copy|, or they'll get
-         * inadvertently freed.
-         */
-        tmp_copy.keymgmt = NULL;
-        tmp_copy.keydata = NULL;
+    /* Check again in case some other thread has updated it in the meantime */
+    ret = pk->legacy_cache_pkey.ptr;
+    if (ret == NULL) {
+        /* Steal the legacy key reference from the temporary copy */
+        ret = pk->legacy_cache_pkey.ptr = tmp_copy->pkey.ptr;
+        tmp_copy->pkey.ptr = NULL;
+    }
 
-        evp_pkey_free_it(&tmp_copy);
-        rv = 1;
-    } else {
-        /* Restore the original key */
-        *pk = tmp_copy;
+    if (!CRYPTO_THREAD_unlock(pk->lock)) {
+        ret = NULL;
+        goto err;
     }
 
- end:
-    if (!CRYPTO_THREAD_unlock(pk->lock))
-        return 0;
-    return rv;
+ err:
+    EVP_PKEY_free(tmp_copy);
+
+    return ret;
 }
 #endif  /* FIPS_MODULE */
 
diff --git a/crypto/evp/pmeth_gn.c b/crypto/evp/pmeth_gn.c
index 1e4078cfa7..1953e0f958 100644
--- a/crypto/evp/pmeth_gn.c
+++ b/crypto/evp/pmeth_gn.c
@@ -197,7 +197,7 @@ int EVP_PKEY_gen(EVP_PKEY_CTX *ctx, EVP_PKEY **ppkey)
 #endif
 
     /*
-     * Because we still have legacy keys, and evp_pkey_downgrade()
+     * Because we still have legacy keys
      * TODO remove this #legacy internal keys are gone
      */
     (*ppkey)->type = ctx->legacy_keytype;
@@ -208,8 +208,17 @@ int EVP_PKEY_gen(EVP_PKEY_CTX *ctx, EVP_PKEY **ppkey)
 #ifdef FIPS_MODULE
     goto not_supported;
 #else
-    if (ctx->pkey && !evp_pkey_downgrade(ctx->pkey))
+    /*
+     * If we get here then we're using legacy paramgen/keygen. In that case
+     * the pkey in ctx (if there is one) had better not be provided (because the
+     * legacy methods may not know how to handle it). However we can only get
+     * here if ctx->op.keymgmt.genctx == NULL, but that should never be the case
+     * if ctx->pkey is provided because we don't allow this when we initialise
+     * the ctx.
+     */
+    if (ctx->pkey != NULL && !ossl_assert(!evp_pkey_is_provided(ctx->pkey)))
         goto not_accessible;
+
     switch (ctx->operation) {
     case EVP_PKEY_OP_PARAMGEN:
         ret = ctx->pmeth->paramgen(ctx, *ppkey);
diff --git a/crypto/evp/pmeth_lib.c b/crypto/evp/pmeth_lib.c
index b08d0d2e3c..96d103544d 100644
--- a/crypto/evp/pmeth_lib.c
+++ b/crypto/evp/pmeth_lib.c
@@ -266,8 +266,7 @@ static EVP_PKEY_CTX *int_ctx_new(OSSL_LIB_CTX *libctx,
         /*
          * Chase down the legacy NID, as that might be needed for diverse
          * purposes, such as ensure that EVP_PKEY_type() can return sensible
-         * values, or that there's a better chance to "downgrade" a key when
-         * needed.  We go through all keymgmt names, because the keytype
+         * values. We go through all keymgmt names, because the keytype
          * that's passed to this function doesn't necessarily translate
          * directly.
          * TODO: Remove this when #legacy keys are gone.
diff --git a/doc/internal/man3/evp_pkey_export_to_provider.pod b/doc/internal/man3/evp_pkey_export_to_provider.pod
index 6cea8a9aab..65fb7109e0 100644
--- a/doc/internal/man3/evp_pkey_export_to_provider.pod
+++ b/doc/internal/man3/evp_pkey_export_to_provider.pod
@@ -53,10 +53,9 @@ evp_pkey_downgrade() returns 1 on success or 0 on error.
 
 =head1 NOTES
 
-Some functions calling evp_pkey_export_to_provider() or evp_pkey_downgrade()
-may have received a const key, and may therefore have to cast the key to
-non-const form to call this function.  Since B<EVP_PKEY> is always dynamically
-allocated, this is OK.
+Some functions calling evp_pkey_export_to_provider() may have received a const
+key, and may therefore have to cast the key to non-const form to call this
+function.  Since B<EVP_PKEY> is always dynamically allocated, this is OK.
 
 =head1 SEE ALSO
 
diff --git a/doc/internal/man7/EVP_PKEY.pod b/doc/internal/man7/EVP_PKEY.pod
index 022f3f0e4e..7088b6cc08 100644
--- a/doc/internal/man7/EVP_PKEY.pod
+++ b/doc/internal/man7/EVP_PKEY.pod
@@ -178,27 +178,20 @@ OSSL_FUNC_keymgmt_import() function.
 
 =back
 
-=head2 Upgrading and downgrading a key
-
-An B<EVP_PKEY> with a legacy origin will I<never> be upgraded to
-become an B<EVP_PKEY> with a provider native origin.  Instead, we have
-the operation cache as described above, that takes care of the needs
-of the diverse operation the application may want to perform.
-
-An B<EVP_PKEY> with a provider native origin, I<may> be downgraded to
-be I<transformed> into an B<EVP_PKEY> with a legacy origin.  Because
-an B<EVP_PKEY> can't have two origins, it means that it stops having a
-provider native origin.  The previous provider native key data is
-moved to the operation cache.  Downgrading is performed with the
-internal function L<evp_pkey_downgrade(3)>.
-
-I<Downgrading a key is understandably fragile>, and possibly surprising,
-and should therefore be done I<as little as possible>, but is needed
-to be able to support functions like L<EVP_PKEY_get0_RSA(3)>.
-The general recommendation is to use L<evp_pkey_copy_downgraded(3)>
-whenever possible, which it should be if the need for a legacy origin
-is only internal, or better yet, to remove the need for downgrade at
-all.
+=head2 Changing a key origin
+
+It is never possible to change the origin of a key. An B<EVP_PKEY> with a legacy
+origin will I<never> be upgraded to become an B<EVP_PKEY> with a provider
+native origin. Instead, we have the operation cache as described above, that
+takes care of the needs of the diverse operation the application may want to
+perform.
+
+Similarly an B<EVP_PKEY> with a provider native origin, will I<never> be
+downgraded to be I<transformed> into an B<EVP_PKEY> with a legacy origin.
+Instead we may have a cached copy of the provider key in legacy form. Once the
+cached copy is created it is never updated. Changes made to the provider key
+are not reflected back in the cached legacy copy. Similarly changes made to the
+cached legacy copy are not reflected back in the provider key.
 
 =head1 SEE ALSO
 
diff --git a/include/crypto/evp.h b/include/crypto/evp.h
index 41487d2af2..c1cce43f80 100644
--- a/include/crypto/evp.h
+++ b/include/crypto/evp.h
@@ -608,6 +608,21 @@ DEFINE_STACK_OF(OP_CACHE_ELEM)
 #define evp_pkey_is_provided(pk)                                \
     ((pk)->keymgmt != NULL)
 
+union legacy_pkey_st {
+    void *ptr;
+    struct rsa_st *rsa;     /* RSA */
+#  ifndef OPENSSL_NO_DSA
+    struct dsa_st *dsa;     /* DSA */
+#  endif
+#  ifndef OPENSSL_NO_DH
+    struct dh_st *dh;       /* DH */
+#  endif
+#  ifndef OPENSSL_NO_EC
+    struct ec_key_st *ec;   /* ECC */
+    ECX_KEY *ecx;           /* X25519, X448, Ed25519, Ed448 */
+#  endif
+};
+
 struct evp_pkey_st {
     /* == Legacy attributes == */
     int type;
@@ -621,24 +636,15 @@ struct evp_pkey_st {
     const EVP_PKEY_ASN1_METHOD *ameth;
     ENGINE *engine;
     ENGINE *pmeth_engine; /* If not NULL public key ENGINE to use */
-    union {
-        void *ptr;
-        struct rsa_st *rsa;     /* RSA */
-#  ifndef OPENSSL_NO_DSA
-        struct dsa_st *dsa;     /* DSA */
-#  endif
-#  ifndef OPENSSL_NO_DH
-        struct dh_st *dh;       /* DH */
-#  endif
-#  ifndef OPENSSL_NO_EC
-        struct ec_key_st *ec;   /* ECC */
-        ECX_KEY *ecx;           /* X25519, X448, Ed25519, Ed448 */
-#  endif
-    } pkey;
+
+    /* Union to store the reference to an origin legacy key */
+    union legacy_pkey_st pkey;
+
+    /* Union to store the reference to a non-origin legacy key */
+    union legacy_pkey_st legacy_cache_pkey;
 # endif
 
     /* == Common attributes == */
-    /* If these are modified, so must evp_pkey_downgrade() */
     CRYPTO_REF_COUNT references;
     CRYPTO_RWLOCK *lock;
     STACK_OF(X509_ATTRIBUTE) *attributes; /* [ 0 ] */
@@ -719,7 +725,7 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
                                   const char *propquery);
 #ifndef FIPS_MODULE
 int evp_pkey_copy_downgraded(EVP_PKEY **dest, const EVP_PKEY *src);
-int evp_pkey_downgrade(EVP_PKEY *pk);
+void *evp_pkey_get_legacy(EVP_PKEY *pk);
 void evp_pkey_free_legacy(EVP_PKEY *x);
 #endif