Partial revert and reimplement "Enable brainpool curves for TLS1.3"

This partially reverts commit 0a10825a0 in order to reimplement it in a
simpler way in the next commit. The reverted aspects are all related to
the TLSv1.3 brainpool curves in the supported_groups extension. Rather
than special casing the handling of these curves we simply add new entries
to the groups table to represent them. They can then be handled without
any additional special casing. This makes the code simpler to maintain.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/19315)

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 0896be023295f050ba08ccb23fa577ee695f945d..df9ceb1383366d5872b2e2f49251f19a1d8963bd 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3632,11 +3632,8 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg)
                 int *cptr = parg;
 
                 for (i = 0; i < clistlen; i++) {
-                    uint16_t cid = SSL_CONNECTION_IS_TLS13(sc)
-                                   ? ssl_group_id_tls13_to_internal(clist[i])
-                                   : clist[i];
                     const TLS_GROUP_INFO *cinf
-                        = tls1_group_id_lookup(s->ctx, cid);
+                        = tls1_group_id_lookup(s->ctx, clist[i]);
 
                     if (cinf != NULL)
                         cptr[i] = tls1_group_id2nid(cinf->group_id, 1);

diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h
index 2416def0b0efa919537c7545cd7145b411740c28..a01cc8cc713a112dc1e6102f162382772bfcfbc9 100644 (file)
--- a/ssl/ssl_local.h
+++ b/ssl/ssl_local.h
@@ -2736,8 +2736,6 @@ __owur int ssl_check_srvr_ecc_cert_and_alg(X509 *x, SSL_CONNECTION *s);
 
 SSL_COMP *ssl3_comp_find(STACK_OF(SSL_COMP) *sk, int n);
 
-__owur uint16_t ssl_group_id_internal_to_tls13(uint16_t curve_id);
-__owur uint16_t ssl_group_id_tls13_to_internal(uint16_t curve_id);
 __owur const TLS_GROUP_INFO *tls1_group_id_lookup(SSL_CTX *ctx, uint16_t curve_id);
 __owur int tls1_group_id2nid(uint16_t group_id, int include_unknown);
 __owur uint16_t tls1_nid2group_id(int nid);

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 6dc21ad42f23cb3b6cbb9f76aecd10197c4f6706..47ad5110aba11067a4ff64c1c291f9d5cc4e6f9d 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1409,7 +1409,7 @@ static int final_key_share(SSL_CONNECTION *s, unsigned int context, int sent)
                     group_id = pgroups[i];
 
                     if (check_in_list(s, group_id, clntgroups, clnt_num_groups,
-                                      2))
+                                      1))
                         break;
                 }
 

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 19f6561b179950524356105660a8092cdf900032..18bcba036fd51bc4f46fe2a488553fa10f0c0bc3 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -226,21 +226,6 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL_CONNECTION *s, WPACKET *pkt,
 
         if (tls_valid_group(s, ctmp, min_version, max_version, 0, &okfortls13)
                 && tls_group_allowed(s, ctmp, SSL_SECOP_CURVE_SUPPORTED)) {
-#ifndef OPENSSL_NO_TLS1_3
-            int ctmp13 = ssl_group_id_internal_to_tls13(ctmp);
-
-            if (ctmp13 != 0 && ctmp13 != ctmp
-                    && max_version == TLS1_3_VERSION) {
-                if (!WPACKET_put_bytes_u16(pkt, ctmp13)) {
-                    SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
-                    return EXT_RETURN_FAIL;
-                }
-                tls13added++;
-                added++;
-                if (min_version == TLS1_3_VERSION)
-                    continue;
-            }
-#endif
             if (!WPACKET_put_bytes_u16(pkt, ctmp)) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
                 return EXT_RETURN_FAIL;
@@ -646,7 +631,7 @@ static int add_key_share(SSL_CONNECTION *s, WPACKET *pkt, unsigned int curve_id)
     }
 
     /* Create KeyShareEntry */
-    if (!WPACKET_put_bytes_u16(pkt, ssl_group_id_internal_to_tls13(curve_id))
+    if (!WPACKET_put_bytes_u16(pkt, curve_id)
             || !WPACKET_sub_memcpy_u16(pkt, encoded_point, encodedlen)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         goto err;
@@ -699,9 +684,6 @@ EXT_RETURN tls_construct_ctos_key_share(SSL_CONNECTION *s, WPACKET *pkt,
         curve_id = s->s3.group_id;
     } else {
         for (i = 0; i < num_groups; i++) {
-            if (ssl_group_id_internal_to_tls13(pgroups[i]) == 0)
-                continue;
-
             if (!tls_group_allowed(s, pgroups[i], SSL_SECOP_CURVE_SUPPORTED))
                 continue;
 
@@ -1799,7 +1781,6 @@ int tls_parse_stoc_key_share(SSL_CONNECTION *s, PACKET *pkt,
         return 0;
     }
 
-    group_id = ssl_group_id_tls13_to_internal(group_id);
     if ((context & SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST) != 0) {
         const uint16_t *pgroups = NULL;
         size_t i, num_groups;

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 4f7321fd20da0ae2ad320054fec61eecbd03cd69..6a488a873774dd9f1e0a46352af0e0cf2f6cb03c 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -642,7 +642,7 @@ int tls_parse_ctos_key_share(SSL_CONNECTION *s, PACKET *pkt,
          * we requested, and must be the only key_share sent.
          */
         if (s->s3.group_id != 0
-                && (ssl_group_id_tls13_to_internal(group_id) != s->s3.group_id
+                && (group_id != s->s3.group_id
                     || PACKET_remaining(&key_share_list) != 0)) {
             SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_BAD_KEY_SHARE);
             return 0;
@@ -664,8 +664,6 @@ int tls_parse_ctos_key_share(SSL_CONNECTION *s, PACKET *pkt,
         /* Cache the selected group ID in the SSL_SESSION */
         s->session->kex_group = group_id;
 
-        group_id = ssl_group_id_tls13_to_internal(group_id);
-
         if ((s->s3.peer_tmp = ssl_generate_param_group(s, group_id)) == NULL) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR,
                    SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS);
@@ -1612,8 +1610,7 @@ EXT_RETURN tls_construct_stoc_key_share(SSL_CONNECTION *s, WPACKET *pkt,
         }
         if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share)
                 || !WPACKET_start_sub_packet_u16(pkt)
-                || !WPACKET_put_bytes_u16(pkt, ssl_group_id_internal_to_tls13(
-                                          s->s3.group_id))
+                || !WPACKET_put_bytes_u16(pkt, s->s3.group_id)
                 || !WPACKET_close(pkt)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
             return EXT_RETURN_FAIL;

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 07939ee9601ced3143ee7d544318ce16a2eae712..8b34c11048e02330d01d8bb639a9990a46648934 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -2222,15 +2222,9 @@ int check_in_list(SSL_CONNECTION *s, uint16_t group_id, const uint16_t *groups,
     if (groups == NULL || num_groups == 0)
         return 0;
 
-    if (checkallow == 1)
-        group_id = ssl_group_id_tls13_to_internal(group_id);
-
     for (i = 0; i < num_groups; i++) {
         uint16_t group = groups[i];
 
-        if (checkallow == 2)
-            group = ssl_group_id_tls13_to_internal(group);
-
         if (group_id == group
                 && (!checkallow
                     || tls_group_allowed(s, group, SSL_SECOP_CURVE_CHECK))) {

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index ab539513bf67cf0a01f10583c9199b9ab58f833a..dcd7b294a030a0cffc629d1d30a20cff834a4a8d 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -435,42 +435,6 @@ static uint16_t tls1_group_name2id(SSL_CTX *ctx, const char *name)
     return 0;
 }
 
-uint16_t ssl_group_id_internal_to_tls13(uint16_t curve_id)
-{
-    switch(curve_id) {
-    case OSSL_TLS_GROUP_ID_brainpoolP256r1:
-        return OSSL_TLS_GROUP_ID_brainpoolP256r1_tls13;
-    case OSSL_TLS_GROUP_ID_brainpoolP384r1:
-        return OSSL_TLS_GROUP_ID_brainpoolP384r1_tls13;
-    case OSSL_TLS_GROUP_ID_brainpoolP512r1:
-        return OSSL_TLS_GROUP_ID_brainpoolP512r1_tls13;
-    case OSSL_TLS_GROUP_ID_brainpoolP256r1_tls13:
-    case OSSL_TLS_GROUP_ID_brainpoolP384r1_tls13:
-    case OSSL_TLS_GROUP_ID_brainpoolP512r1_tls13:
-        return 0;
-    default:
-        return curve_id;
-    }
-}
-
-uint16_t ssl_group_id_tls13_to_internal(uint16_t curve_id)
-{
-    switch(curve_id) {
-    case OSSL_TLS_GROUP_ID_brainpoolP256r1:
-    case OSSL_TLS_GROUP_ID_brainpoolP384r1:
-    case OSSL_TLS_GROUP_ID_brainpoolP512r1:
-        return 0;
-    case OSSL_TLS_GROUP_ID_brainpoolP256r1_tls13:
-        return OSSL_TLS_GROUP_ID_brainpoolP256r1;
-    case OSSL_TLS_GROUP_ID_brainpoolP384r1_tls13:
-        return OSSL_TLS_GROUP_ID_brainpoolP384r1;
-    case OSSL_TLS_GROUP_ID_brainpoolP512r1_tls13:
-        return OSSL_TLS_GROUP_ID_brainpoolP512r1;
-    default:
-        return curve_id;
-    }
-}
-
 const TLS_GROUP_INFO *tls1_group_id_lookup(SSL_CTX *ctx, uint16_t group_id)
 {
     size_t i;
@@ -677,15 +641,8 @@ uint16_t tls1_shared_group(SSL_CONNECTION *s, int nmatch)
 
     for (k = 0, i = 0; i < num_pref; i++) {
         uint16_t id = pref[i];
-        uint16_t cid = id;
 
-        if (SSL_CONNECTION_IS_TLS13(s)) {
-            if (s->options & SSL_OP_CIPHER_SERVER_PREFERENCE)
-                cid = ssl_group_id_internal_to_tls13(id);
-            else
-                cid = id = ssl_group_id_tls13_to_internal(id);
-        }
-        if (!tls1_in_list(cid, supp, num_supp)
+        if (!tls1_in_list(id, supp, num_supp)
                 || !tls_group_allowed(s, id, SSL_SECOP_CURVE_SHARED))
             continue;
         if (nmatch == k)