From 19ad1e9d3737f48c0e1c5cc5397ff1827b6946b8 Mon Sep 17 00:00:00 2001
From: Matt Caswell <matt@openssl.org>
Date: Mon, 15 Mar 2021 16:21:45 +0000
Subject: [PATCH] Remove a TODO(3.0) from EVP_PKEY_derive_set_peer()

The TODO described a case where a legacy derive operation is called, but
the peer key is provider based. In practice this will almost never be a
problem. We should never end up in our own legacy EVP_PKEY_METHOD
implementations if no ENGINE has been configured. If an ENGINE has been
configured then we we will be using a third party EVP_PKEY_METHOD
implementation and public APIs will be used to obtain the key data from the
peer key so there will be no "reaching inside" the pkey.

There is a theoretical case where a third party ENGINE wraps our own
internal EVP_PKEY_METHODs using EVP_PKEY_meth_find() or
EVP_PKEY_meth_get0(). For these cases we just ensure all our
EVP_PKEY_METHODs never reach "inside" the implementation of a peer key. We
can never assume that it is a legacy key.

Fixes #14399

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14555)
---
 crypto/dh/dh_pmeth.c  | 19 +++++++++++++------
 crypto/ec/ec_pmeth.c  | 11 +++++++++--
 crypto/ec/ecx_meth.c  |  2 +-
 crypto/evp/exchange.c |  4 ----
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/crypto/dh/dh_pmeth.c b/crypto/dh/dh_pmeth.c
index fdd9194f1ac..584a174ae27 100644
--- a/crypto/dh/dh_pmeth.c
+++ b/crypto/dh/dh_pmeth.c
@@ -421,23 +421,30 @@ static int pkey_dh_derive(EVP_PKEY_CTX *ctx, unsigned char *key,
 {
     int ret;
     DH *dh;
+    const DH *dhpub;
     DH_PKEY_CTX *dctx = ctx->data;
-    BIGNUM *dhpub;
-    if (!ctx->pkey || !ctx->peerkey) {
+    BIGNUM *dhpubbn;
+
+    if (ctx->pkey == NULL || ctx->peerkey == NULL) {
         ERR_raise(ERR_LIB_DH, DH_R_KEYS_NOT_SET);
         return 0;
     }
     dh = ctx->pkey->pkey.dh;
-    dhpub = ctx->peerkey->pkey.dh->pub_key;
+    dhpub = EVP_PKEY_get0_DH(ctx->peerkey);
+    if (dhpub == NULL) {
+        ERR_raise(ERR_LIB_DH, DH_R_KEYS_NOT_SET);
+        return 0;
+    }
+    dhpubbn = dhpub->pub_key;
     if (dctx->kdf_type == EVP_PKEY_DH_KDF_NONE) {
         if (key == NULL) {
             *keylen = DH_size(dh);
             return 1;
         }
         if (dctx->pad)
-            ret = DH_compute_key_padded(key, dhpub, dh);
+            ret = DH_compute_key_padded(key, dhpubbn, dh);
         else
-            ret = DH_compute_key(key, dhpub, dh);
+            ret = DH_compute_key(key, dhpubbn, dh);
         if (ret < 0)
             return ret;
         *keylen = ret;
@@ -461,7 +468,7 @@ static int pkey_dh_derive(EVP_PKEY_CTX *ctx, unsigned char *key,
         if (Z == NULL) {
             goto err;
         }
-        if (DH_compute_key_padded(Z, dhpub, dh) <= 0)
+        if (DH_compute_key_padded(Z, dhpubbn, dh) <= 0)
             goto err;
         if (!DH_KDF_X9_42(key, *keylen, Z, Zlen, dctx->kdf_oid,
                           dctx->kdf_ukm, dctx->kdf_ukmlen, dctx->kdf_md))
diff --git a/crypto/ec/ec_pmeth.c b/crypto/ec/ec_pmeth.c
index 2280189e286..e4f3549d304 100644
--- a/crypto/ec/ec_pmeth.c
+++ b/crypto/ec/ec_pmeth.c
@@ -161,8 +161,15 @@ static int pkey_ec_derive(EVP_PKEY_CTX *ctx, unsigned char *key, size_t *keylen)
     size_t outlen;
     const EC_POINT *pubkey = NULL;
     EC_KEY *eckey;
+    const EC_KEY *eckeypub;
     EC_PKEY_CTX *dctx = ctx->data;
-    if (!ctx->pkey || !ctx->peerkey) {
+
+    if (ctx->pkey == NULL || ctx->peerkey == NULL) {
+        ERR_raise(ERR_LIB_EC, EC_R_KEYS_NOT_SET);
+        return 0;
+    }
+    eckeypub = EVP_PKEY_get0_EC_KEY(ctx->peerkey);
+    if (eckeypub == NULL) {
         ERR_raise(ERR_LIB_EC, EC_R_KEYS_NOT_SET);
         return 0;
     }
@@ -178,7 +185,7 @@ static int pkey_ec_derive(EVP_PKEY_CTX *ctx, unsigned char *key, size_t *keylen)
         *keylen = (EC_GROUP_get_degree(group) + 7) / 8;
         return 1;
     }
-    pubkey = EC_KEY_get0_public_key(ctx->peerkey->pkey.ec);
+    pubkey = EC_KEY_get0_public_key(eckeypub);
 
     /*
      * NB: unlike PKCS#3 DH, if *outlen is less than maximum size this is not
diff --git a/crypto/ec/ecx_meth.c b/crypto/ec/ecx_meth.c
index cd73a158471..9a812c875bc 100644
--- a/crypto/ec/ecx_meth.c
+++ b/crypto/ec/ecx_meth.c
@@ -766,7 +766,7 @@ static int validate_ecx_derive(EVP_PKEY_CTX *ctx, unsigned char *key,
         return 0;
     }
     ecxkey = ctx->pkey->pkey.ecx;
-    peerkey = ctx->peerkey->pkey.ecx;
+    peerkey = EVP_PKEY_get0(ctx->peerkey);
     if (ecxkey == NULL || ecxkey->privkey == NULL) {
         ERR_raise(ERR_LIB_EC, EC_R_INVALID_PRIVATE_KEY);
         return 0;
diff --git a/crypto/evp/exchange.c b/crypto/evp/exchange.c
index e0e0597b3bf..5e098f0c3f7 100644
--- a/crypto/evp/exchange.c
+++ b/crypto/evp/exchange.c
@@ -349,10 +349,6 @@ int EVP_PKEY_derive_set_peer(EVP_PKEY_CTX *ctx, EVP_PKEY *peer)
 #ifdef FIPS_MODULE
     return ret;
 #else
-    /*
-     * TODO(3.0) investigate the case where the operation is deemed legacy,
-     * but the given peer key is provider only.
-     */
     if (ctx->pmeth == NULL
         || !(ctx->pmeth->derive != NULL
              || ctx->pmeth->encrypt != NULL
-- 
2.47.3