Don't copy parameters on setting a key in libssl

Whenever we set a private key in libssl, we first found the certificate
that matched the key algorithm. Then we copied the key parameters from the
private key into the public key for the certficate before finally checking
that the private key matched the public key in the certificate. This makes
no sense! Part of checking the private key is to make sure that the
parameters match. It seems that this code has been present since SSLeay.
Perhaps at some point it made sense to do this - but it doesn't any more.

We remove that piece of code altogether. The previous code also had the
undocumented side effect of removing the certificate if the key didn't
match. This makes sense if you've just overwritten the parameters in the
certificate with bad values - but doesn't seem to otherwise. I've also
removed that error logic.

Due to issue #13893, the public key associated with the certificate is
always a legacy key. EVP_PKEY_copy_parameters will downgrade the "from"
key to legacy if the target is legacy, so this means that in libssl all
private keys were always downgraded to legacy when they are first set
in the SSL/SSL_CTX. Removing the EVP_PKEY_copy_parameters code has the
added benefit of removing that downgrade.

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

2 files changed, 5 insertions, 22 deletions

diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index 66487054ae..f82e42c7e3 100644
--- a/crypto/evp/p_lib.c
+++ b/crypto/evp/p_lib.c
@@ -1154,7 +1154,7 @@ int EVP_PKEY_print_params(BIO *out, const EVP_PKEY *pkey,
                       pctx);
 }
 
-static void find_md_nid(const char *mdname, void *data)
+static void mdname2nid(const char *mdname, void *data)
 {
     int *nid = (int *)data;
 
@@ -1199,7 +1199,7 @@ static int legacy_asn1_ctrl_to_param(EVP_PKEY *pkey, int op,
                  * We have the namemap number - now we need to find the
                  * associated nid
                  */
-                ossl_namemap_doall_names(namemap, mdnum, find_md_nid, &nid);
+                ossl_namemap_doall_names(namemap, mdnum, mdname2nid, &nid);
                 *(int *)arg2 = nid;
             }
             return rv;
diff --git a/ssl/ssl_rsa.c b/ssl/ssl_rsa.c
index bfdd5ff43d..1c1053d316 100644
--- a/ssl/ssl_rsa.c
+++ b/ssl/ssl_rsa.c
@@ -124,26 +124,9 @@ static int ssl_set_pkey(CERT *c, EVP_PKEY *pkey)
         return 0;
     }
 
-    if (c->pkeys[i].x509 != NULL) {
-        EVP_PKEY *pktmp;
-        pktmp = X509_get0_pubkey(c->pkeys[i].x509);
-        if (pktmp == NULL) {
-            ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE);
-            return 0;
-        }
-        /*
-         * The return code from EVP_PKEY_copy_parameters is deliberately
-         * ignored. Some EVP_PKEY types cannot do this.
-         */
-        EVP_PKEY_copy_parameters(pktmp, pkey);
-        ERR_clear_error();
-
-        if (!X509_check_private_key(c->pkeys[i].x509, pkey)) {
-            X509_free(c->pkeys[i].x509);
-            c->pkeys[i].x509 = NULL;
-            return 0;
-        }
-    }
+    if (c->pkeys[i].x509 != NULL
+            && !X509_check_private_key(c->pkeys[i].x509, pkey))
+        return 0;
 
     EVP_PKEY_free(c->pkeys[i].privatekey);
     EVP_PKEY_up_ref(pkey);