Cache legacy keys instead of downgrading them

mattcaswell · mattcaswell · commit b574c6a9ac96 · 2021-03-08T15:11:31.000Z
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 #14319)
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/include/crypto/evp.h b/include/crypto/evp.h

Original file line number	Diff line number	Diff line change
`@@ -33,15 +33,11 @@ int EVP_PKEY_set1_RSA(EVP_PKEY pkey, RSA key)`
`33`	`33`
`34`	`34`	`RSA EVP_PKEY_get0_RSA(const EVP_PKEY pkey)`
`35`	`35`	`{`
`36`		`- if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {`
`37`		`- ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);`
`38`		`- return NULL;`
`39`		`- }`
`40`	`36`	`if (pkey->type != EVP_PKEY_RSA && pkey->type != EVP_PKEY_RSA_PSS) {`
`41`	`37`	`ERR_raise(ERR_LIB_EVP, EVP_R_EXPECTING_AN_RSA_KEY);`
`42`	`38`	`return NULL;`
`43`	`39`	`}`
`44`		`- return pkey->pkey.rsa;`
	`40`	`+ return evp_pkey_get_legacy((EVP_PKEY *)pkey);`
`45`	`41`	`}`
`46`	`42`
`47`	`43`	`RSA EVP_PKEY_get1_RSA(EVP_PKEY pkey)`
`@@ -65,15 +61,11 @@ int EVP_PKEY_set1_EC_KEY(EVP_PKEY pkey, EC_KEY key)`
`65`	`61`
`66`	`62`	`EC_KEY EVP_PKEY_get0_EC_KEY(const EVP_PKEY pkey)`
`67`	`63`	`{`
`68`		`- if (!evp_pkey_downgrade((EVP_PKEY *)pkey)) {`
`69`		`- ERR_raise(ERR_LIB_EVP, EVP_R_INACCESSIBLE_KEY);`
`70`		`- return NULL;`
`71`		`- }`
`72`	`64`	`if (EVP_PKEY_base_id(pkey) != EVP_PKEY_EC) {`
`73`	`65`	`EVPerr(EVP_F_EVP_PKEY_GET0_EC_KEY, EVP_R_EXPECTING_A_EC_KEY);`
`74`	`66`	`return NULL;`
`75`	`67`	`}`
`76`		`- return pkey->pkey.ec;`
	`68`	`+ return evp_pkey_get_legacy((EVP_PKEY *)pkey);`
`77`	`69`	`}`
`78`	`70`
`79`	`71`	`EC_KEY EVP_PKEY_get1_EC_KEY(EVP_PKEY pkey)`