Don't downgrade keys in libssl

We were downgrading to legacy keys at various points in libssl in
order to get or set an encoded point. Now that the encoded point
functions work with provided keys this is no longer necessary.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/11898)

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 764c52322d9b6ca75c6e06f542e761796e7c157b..ab2d98de608eda7d8ee2b3375eaa0a98c3a37238 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -648,21 +648,6 @@ static int add_key_share(SSL *s, WPACKET *pkt, unsigned int curve_id)
             /* SSLfatal() already called */
             return 0;
         }
-
-        /*
-         * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-         * knows how to get a key from an encoded point with the help of
-         * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-         * downgrades an EVP_PKEY to contain a legacy key.
-         *
-         * THIS IS TEMPORARY
-         */
-        EVP_PKEY_get0(key_share_key);
-        if (EVP_PKEY_id(key_share_key) == EVP_PKEY_NONE) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_ADD_KEY_SHARE,
-                     ERR_R_EC_LIB);
-            goto err;
-        }
     }
 
     /* Encode the public key. */
@@ -1926,22 +1911,6 @@ int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
         return 0;
     }
 
-    /*
-     * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-     * knows how to get a key from an encoded point with the help of
-     * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-     * downgrades an EVP_PKEY to contain a legacy key.
-     *
-     * THIS IS TEMPORARY
-     */
-    EVP_PKEY_get0(skey);
-    if (EVP_PKEY_id(skey) == EVP_PKEY_NONE) {
-        EVP_PKEY_free(skey);
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_STOC_KEY_SHARE,
-                 ERR_R_INTERNAL_ERROR);
-        return 0;
-    }
-
     if (!EVP_PKEY_set1_tls_encodedpoint(skey, PACKET_data(&encoded_pt),
                                         PACKET_remaining(&encoded_pt))) {
         SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PARSE_STOC_KEY_SHARE,

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index aa71cec7e901fe38317b8281b0b99a5e1c1adffb..3a0fee6ebc508d203f3a911c891f450a653a79d7 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -715,21 +715,6 @@ int tls_parse_ctos_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
             return 0;
         }
 
-        /*
-         * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-         * knows how to get a key from an encoded point with the help of
-         * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-         * downgrades an EVP_PKEY to contain a legacy key.
-         *
-         * THIS IS TEMPORARY
-         */
-        EVP_PKEY_get0(s->s3.peer_tmp);
-        if (EVP_PKEY_id(s->s3.peer_tmp) == EVP_PKEY_NONE) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_CTOS_KEY_SHARE,
-                     ERR_R_INTERNAL_ERROR);
-            return 0;
-        }
-
         s->s3.group_id = group_id;
 
         if (!EVP_PKEY_set1_tls_encodedpoint(s->s3.peer_tmp,
@@ -1757,21 +1742,6 @@ EXT_RETURN tls_construct_stoc_key_share(SSL *s, WPACKET *pkt,
         return EXT_RETURN_FAIL;
     }
 
-    /*
-     * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-     * knows how to get a key from an encoded point with the help of
-     * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-     * downgrades an EVP_PKEY to contain a legacy key.
-     *
-     * THIS IS TEMPORARY
-     */
-    EVP_PKEY_get0(skey);
-    if (EVP_PKEY_id(skey) == EVP_PKEY_NONE) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_STOC_KEY_SHARE,
-                 ERR_R_INTERNAL_ERROR);
-        return EXT_RETURN_FAIL;
-    }
-
     /* Generate encoding of server key */
     encoded_pt_len = EVP_PKEY_get1_tls_encodedpoint(skey, &encodedPoint);
     if (encoded_pt_len == 0) {

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 67d8ae8ce69744a2026b60d3c3bfea4e6aa49a68..7189940a62c49d5b2e18534268cb1657d7b6e436 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -2231,21 +2231,6 @@ static int tls_process_ske_ecdhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey)
         return 0;
     }
 
-    /*
-     * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-     * knows how to get a key from an encoded point with the help of
-     * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-     * downgrades an EVP_PKEY to contain a legacy key.
-     *
-     * THIS IS TEMPORARY
-     */
-    EVP_PKEY_get0(s->s3.peer_tmp);
-    if (EVP_PKEY_id(s->s3.peer_tmp) == EVP_PKEY_NONE) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_SKE_ECDHE,
-                 ERR_R_INTERNAL_ERROR);
-        return 0;
-    }
-
     if (!EVP_PKEY_set1_tls_encodedpoint(s->s3.peer_tmp,
                                         PACKET_data(&encoded_pt),
                                         PACKET_remaining(&encoded_pt))) {
@@ -3148,21 +3133,6 @@ static int tls_construct_cke_ecdhe(SSL *s, WPACKET *pkt)
         goto err;
     }
 
-    /*
-     * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-     * knows how to get a key from an encoded point with the help of
-     * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-     * downgrades an EVP_PKEY to contain a legacy key.
-     *
-     * THIS IS TEMPORARY
-     */
-    EVP_PKEY_get0(ckey);
-    if (EVP_PKEY_id(skey) == EVP_PKEY_NONE) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CKE_ECDHE,
-                 ERR_R_INTERNAL_ERROR);
-        goto err;
-    }
-
     if (ssl_derive(s, ckey, skey, 0) == 0) {
         /* SSLfatal() already called */
         goto err;

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index e5340b4e7fa4ab56c782f8edc775ee8a0394b59c..036bfadbe5c41bffa9b05b0eaf6b3fc1de9a5a5b 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2636,20 +2636,6 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt)
             goto err;
         }
 
-        /*
-         * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-         * knows how to get a key from an encoded point with the help of
-         * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-         * downgrades an EVP_PKEY to contain a legacy key.
-         *
-         * THIS IS TEMPORARY
-         */
-        EVP_PKEY_get0(s->s3.tmp.pkey);
-        if (EVP_PKEY_id(s->s3.tmp.pkey) == EVP_PKEY_NONE) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, 0, ERR_R_EC_LIB);
-            goto err;
-        }
-
         /* Encode the public key. */
         encodedlen = EVP_PKEY_get1_tls_encodedpoint(s->s3.tmp.pkey,
                                                     &encodedPoint);
@@ -3234,21 +3220,6 @@ static int tls_process_cke_ecdhe(SSL *s, PACKET *pkt)
             goto err;
         }
 
-        /*
-         * TODO(3.0) Remove this when EVP_PKEY_get1_tls_encodedpoint()
-         * knows how to get a key from an encoded point with the help of
-         * a OSSL_SERIALIZER deserializer.  We know that EVP_PKEY_get0()
-         * downgrades an EVP_PKEY to contain a legacy key.
-         *
-         * THIS IS TEMPORARY
-         */
-        EVP_PKEY_get0(ckey);
-        if (EVP_PKEY_id(ckey) == EVP_PKEY_NONE) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CKE_ECDHE,
-                     ERR_R_INTERNAL_ERROR);
-            goto err;
-        }
-
         if (EVP_PKEY_set1_tls_encodedpoint(ckey, data, i) == 0) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CKE_ECDHE,
                      ERR_R_EC_LIB);

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index a59d992e47b3a31b45c69a5c912baf9f78f31a42..68bd5f2611e23df81fcb5c9c7aa669bee55dad04 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1217,17 +1217,6 @@ int tls12_check_peer_sigalg(SSL *s, uint16_t sig, EVP_PKEY *pkey)
     const SIGALG_LOOKUP *lu;
     int secbits = 0;
 
-    /*
-     * TODO(3.0) Remove this when we adapted this function for provider
-     * side keys.  We know that EVP_PKEY_get0() downgrades an EVP_PKEY
-     * to contain a legacy key.
-     *
-     * THIS IS TEMPORARY
-     */
-    EVP_PKEY_get0(pkey);
-    if (EVP_PKEY_id(pkey) == EVP_PKEY_NONE)
-        return 0;
-
     pkeyid = EVP_PKEY_id(pkey);
     /* Should never happen */
     if (pkeyid == -1)