Don't copy parameters on setting a key in libssl
authorMatt Caswell <matt@openssl.org>
Mon, 18 Jan 2021 16:50:07 +0000 (16:50 +0000)
committerMatt Caswell <matt@openssl.org>
Fri, 22 Jan 2021 09:30:53 +0000 (09:30 +0000)
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)

crypto/evp/p_lib.c
ssl/ssl_rsa.c

index 66487054aee871b3b664ae474f0825bb1f8864c8..f82e42c7e3d032443e44bfbdab8b6570face65ec 100644 (file)
@@ -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;
index bfdd5ff43ded5953f2de5eb64d7818fba1300c78..1c1053d316f0738b7bc81d6946912c7d2d43073c 100644 (file)
@@ -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);