From 9c5f04d1a9cc067bb8a6a1181d3d42bfd0a62762 Mon Sep 17 00:00:00 2001
From: Viktor Dukhovni <openssl-users@dukhovni.org>
Date: Tue, 17 Feb 2026 18:37:06 +1100
Subject: [PATCH] Fix group tuple handling in DEFAULT expansion

Also fine-tune docs and add tests.

Fixes: #30109
Fixes: CVE-2026-2673

Reviewed-by: Matt Caswell <matt@openssl.foundation>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
MergeDate: Fri Mar 13 12:44:06 2026
(Merged from https://github.com/openssl/openssl/pull/30110)
---
 doc/man3/SSL_CTX_set1_curves.pod | 123 +++++++++++++++++++++----------
 ssl/t1_lib.c                     |  95 ++++++++++++++----------
 test/tls13groupselection_test.c  |  36 +++++++--
 3 files changed, 172 insertions(+), 82 deletions(-)

diff --git a/doc/man3/SSL_CTX_set1_curves.pod b/doc/man3/SSL_CTX_set1_curves.pod
index 017eefd317..472d385831 100755
--- a/doc/man3/SSL_CTX_set1_curves.pod
+++ b/doc/man3/SSL_CTX_set1_curves.pod
@@ -40,13 +40,13 @@ SSL_get1_curves, SSL_get_shared_curve, SSL_CTX_get0_implemented_groups
 
 For all of the functions below that set the supported groups there must be at
 least one group in the list. A number of these functions identify groups via a
-unique integer NID value. However, support for some groups may be added by
-external providers. In this case there will be no NID assigned for the group.
+unique integer B<NID> value. However, support for some groups may be added by
+external providers. In this case there will be no B<NID> assigned for the group.
 When setting such groups applications should use the "list" form of these
 functions (i.e. SSL_CTX_set1_groups_list() and SSL_set1_groups_list()).
 
 SSL_CTX_set1_groups() sets the supported groups for B<ctx> to B<glistlen>
-groups in the array B<glist>. The array consist of all NIDs of supported groups.
+groups in the array B<glist>. The array consist of all B<NIDs> of supported groups.
 The supported groups for B<TLSv1.3> include:
 B<NID_X9_62_prime256v1>,
 B<NID_secp384r1>,
@@ -73,20 +73,27 @@ B<SSL_OP_CIPHER_SERVER_PREFERENCE> is set, the order of the elements in the
 array determines the selected group. Otherwise, the order is ignored and the
 client's order determines the selection.
 
-For a TLS 1.3 server, the groups determine the selected group, but
-selection is more complex. A TLS 1.3 client sends both a group list as well as a
-predicted subset of groups. Choosing a group outside the predicted subset incurs
-an extra roundtrip. However, in some situations, the most preferred group may
-not be predicted. OpenSSL considers all supported groups in I<clist> to be comparable
-in security and prioritizes avoiding roundtrips above either client or server
-preference order. If an application uses an external provider to extend OpenSSL
-with, e.g., a post-quantum algorithm, this behavior may allow a network attacker
-to downgrade connections to a weaker algorithm. It is therefore recommended
-to use SSL_CTX_set1_groups_list() with the ability to specify group tuples.
+For a TLS 1.3 server, the groups determine the selected group, but selection is
+more complex.
+A TLS 1.3 client sends both a group list and predicted keyshares for a subset
+of groups.
+A server choosing a group outside the client's predicted subset incurs an extra
+roundtrip.
+However, in some situations, the most preferred group may not be predicted.
+
+When groups are specified via SSL_CTX_set1_groups() as a list of B<NID>
+values, OpenSSL considers all supported groups in I<clist> to be comparable in
+security and prioritises avoiding roundtrips above either client or server
+preference order.
+If an application uses an external provider to extend OpenSSL with, e.g., a
+post-quantum algorithm, this behavior may allow a network attacker to downgrade
+connections to a weaker algorithm.
+It is therefore recommended to use SSL_CTX_set1_groups_list() instead, making
+it possible to specify group tuples as described below.
 
 SSL_CTX_set1_groups_list() sets the supported groups for B<ctx> to
 string I<list>. In contrast to SSL_CTX_set1_groups(), the names of the
-groups, rather than their NIDs, are used.
+groups, rather than their B<NIDs>, are used.
 
 The commands below list the available groups for TLS 1.2 and TLS 1.3,
 respectively:
@@ -102,30 +109,72 @@ The preferred group names are those defined by
 L<IANA|https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8>.
 
 The I<list> can be used to define several group tuples of comparable security
-levels, and can specify which key shares should be sent by a client.
-The specified list elements can optionally be ignored, if not implemented
+levels, and can specify which predicted key shares should be sent by a client.
+Group tuples are used by OpenSSL TLS servers to decide whether to request a
+stronger keyshare than those predicted by sending a Hello Retry Request
+(B<HRR>) even if some of the predicted groups are supported.
+OpenSSL clients ignore tuple boundaries, and pay attenion only to the overall
+order of I<list> elements and which groups are selected as predicted keyshares
+as described below.
+
+The specified list elements can optionally be ignored if not implemented
 (listing unknown groups otherwise results in error).
-It is also possible to specify the built-in default set of groups, and to explicitly
-remove a group from that list.
-
-In its simplest form, the string I<list> is just a colon separated list
-of group names, for example "P-521:P-384:P-256:X25519:ffdhe2048". The first
-group listed will also be used for the B<key_share> sent by a client in a
-TLSv1.3 B<ClientHello>. For servers note the discussion above. The list should
-be in order of preference with the most preferred group first.
-
-Group tuples of comparable security are defined by separating them from each
-other by a tuple separator C</>. Keyshares to be sent by a client are specified
-by prepending a C<*> to the group name, while any C<*> will be ignored by a
-server. The following string I<list> for example defines three tuples when
-used on the server-side, and triggers the generation of three key shares
-when used on the client-side: P-521:*P-256/*P-384/*X25519:P-384:ffdhe2048.
-
-If a group name is preceded with the C<?> character, it will be ignored if an
-implementation is missing. If a group name is preceded with the C<-> character, it
-will be removed from the list of groups if present (including not sending a
-key share for this group), ignored otherwise. The pseudo group name
-C<DEFAULT> can be used to select the OpenSSL built-in default list of groups.
+It is also possible to specify the built-in default set of groups, and to
+explicitly remove a group from that list.
+
+In its simplest legacy form, the string I<list> is just a colon separated list
+of group names, for example "P-521:P-384:P-256:X25519:ffdhe2048".
+The first group listed will in this case be used as the sole predicted
+B<key_share> sent by a client in a TLSv1.3 B<ClientHello>.
+The list should be in order of preference with the most preferred group first.
+
+A more expressive syntax supports definition of group tuples of comparable
+security by separating them from each other with C</> characters.
+
+The predicted keyshares to be sent by clients can be explicitly specified by
+adding a C<*> prefix to the associated group name.
+These C<*> prefixes are ignored by servers.
+
+If a group name is prefixed with the C<?> character, it will be ignored if an
+implementation is missing.
+Otherwise, listing an unknown group name will cause a failure to parse the
+I<list>.
+Note that whether a group is known or not may depend on the OpenSSL version,
+how OpenSSL was compiled and/or which providers are loaded.
+Make sure you have the correct spelling of the group name and when in doubt
+prefix it with a C<?> to handle configurations in which it might nevertheless
+be unknown.
+
+If a group name is prefixed with the C<-> character, it will be removed from
+the list of groups specified up to that point.
+It can be added again if specified later.
+Removal of groups that have not been included earlier in the list is silently
+ignored.
+
+The pseudo group name C<DEFAULT> can be used to select the OpenSSL built-in
+default list of groups.
+Prepending one or more groups to C<DEFAULT> using only C<:> separators prepends those
+groups to the built-in default list's first tuple.
+Additional tuples can be prepended by use of the C</> separator.
+Appending a set of groups to C<DEFAULT> using only C<:> separators appends those
+groups to the built-in default list's last tuple.
+Additional tuples can be appended by use of the C</> separator.
+
+The B<DEFAULT> list selects B<X25519MLKEM768> as one of the predicted keyshares.
+In rare cases this can lead to failures or timeouts because the resulting
+larger TLS Client Hello message may no longer fit in a single TCP segment and
+firewall software may erroneously disrupt the TLS handshake.
+If this is an issue or concern, prepending C<?X25519MLKEM768:> without a C<*>
+prefix leads to its occurrence in the default list to be ignored as a duplicate,
+and along with that also the keyshare prediction.
+The group will then only be selected by servers that specifically expect it,
+after a Hello Retry Request (HRR).
+Servers that specifically prefer B<X25519MLKEM768>, are much less likely to be
+found behind problematic firewalls.
+
+The following string I<list> for example defines three tuples when used on the
+server-side, and triggers the generation of three key shares when used on the
+client-side: P-521:*P-256/*P-384/*X25519:P-384:ffdhe2048.
 
 For a TLS 1.3 client, all the groups in the string I<list> are added to the
 supported groups extension of a C<ClientHello>, in the order in which they are listed,
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 2f71f95438..8a8c9ba9d1 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -211,7 +211,7 @@ static const uint16_t suiteb_curves[] = {
 
 /* Group list string of the built-in pseudo group DEFAULT_SUITE_B */
 #define SUITE_B_GROUP_NAME "DEFAULT_SUITE_B"
-#define SUITE_B_GROUP_LIST "secp256r1:secp384r1",
+#define SUITE_B_GROUP_LIST "?secp256r1:?secp384r1",
 
 struct provider_ctx_data_st {
     SSL_CTX *ctx;
@@ -1237,8 +1237,8 @@ typedef struct {
     size_t ksidcnt; /* Number of key shares */
     uint16_t *ksid_arr; /* The IDs of the key share groups (flat list) */
     /* Variable to keep state between execution of callback or helper functions */
-    size_t tuple_mode; /* Keeps track whether tuple_cb called from 'the top' or from gid_cb */
-    int ignore_unknown_default; /* Flag such that unknown groups for DEFAULT[_XYZ] are ignored */
+    int inner; /* Are we expanding a DEFAULT list */
+    int first; /* First tuple of possibly nested expansion? */
 } gid_cb_st;
 
 /* Forward declaration of tuple callback function */
@@ -1313,16 +1313,16 @@ static int gid_cb(const char *elem, int len, void *arg)
             for (i = 0; i < OSSL_NELEM(default_group_strings); i++) {
                 if ((size_t)len == (strlen(default_group_strings[i].list_name))
                     && OPENSSL_strncasecmp(default_group_strings[i].list_name, elem, len) == 0) {
+                    int saved_first;
+
                     /*
                      * We're asked to insert an entire list of groups from a
                      * DEFAULT[_XYZ] 'pseudo group' which we do by
                      * recursively calling this function (indirectly via
                      * CONF_parse_list and tuple_cb); essentially, we treat a DEFAULT
                      * group string like a tuple which is appended to the current tuple
-                     * rather then starting a new tuple. Variable tuple_mode is the flag which
-                     * controls append tuple vs start new tuple.
+                     * rather then starting a new tuple.
                      */
-
                     if (ignore_unknown || remove_group)
                         return -1; /* removal or ignore not allowed here -> syntax error */
 
@@ -1347,15 +1347,17 @@ static int gid_cb(const char *elem, int len, void *arg)
                            strlen(default_group_strings[i].group_string));
                     restored_default_group_string[strlen(default_group_strings[i].group_string) +
                                                   restored_prefix_index] = '\0';
-                    /* We execute the recursive call */
-                    garg->ignore_unknown_default = 1; /* We ignore unknown groups for DEFAULT_XYZ */
-                    /* we enforce group mode (= append tuple) for DEFAULT_XYZ group lists */
-                    garg->tuple_mode = 0;
-                    /* We use the tuple_cb callback to process the pseudo group tuple */
+                    /*
+                     * Append first tuple of result to current tuple, and don't
+                     * terminate the last tuple until we return to a top-level
+                     * tuple_cb.
+                     */
+                    saved_first = garg->first;
+                    garg->inner = garg->first = 1;
                     retval = CONF_parse_list(restored_default_group_string,
-                                             TUPLE_DELIMITER_CHARACTER, 1, tuple_cb, garg);
-                    garg->tuple_mode = 1; /* next call to tuple_cb will again start new tuple */
-                    garg->ignore_unknown_default = 0; /* reset to original value */
+                        TUPLE_DELIMITER_CHARACTER, 1, tuple_cb, garg);
+                    garg->inner = 0;
+                    garg->first = saved_first;
                     /* We don't need the \0-terminated string anymore */
                     OPENSSL_free(restored_default_group_string);
 
@@ -1375,9 +1377,6 @@ static int gid_cb(const char *elem, int len, void *arg)
     if (len == 0)
         return -1; /* Seems we have prefxes without a group name -> syntax error */
 
-    if (garg->ignore_unknown_default == 1) /* Always ignore unknown groups for DEFAULT[_XYZ] */
-        ignore_unknown = 1;
-
     /* Memory management in case more groups are present compared to initial allocation */
     if (garg->gidcnt == garg->gidmax) {
         uint16_t *tmp =
@@ -1513,7 +1512,7 @@ static int gid_cb(const char *elem, int len, void *arg)
         /* and update the book keeping for the number of groups in current tuple */
         garg->tuplcnt_arr[garg->tplcnt]++;
 
-        /* We memorize if needed that we want to add a key share for the current group */
+        /* We want to add a key share for the current group */
         if (add_keyshare)
             garg->ksid_arr[garg->ksidcnt++] = gid;
     }
@@ -1522,6 +1521,39 @@ done:
     return retval;
 }
 
+static int grow_tuples(gid_cb_st *garg)
+{
+    static size_t max_tplcnt = (~(size_t)0) / sizeof(size_t);
+
+    /* This uses OPENSSL_realloc_array() in newer releases */
+    if (garg->tplcnt == garg->tplmax) {
+        size_t newcnt = garg->tplmax + GROUPLIST_INCREMENT;
+        size_t newsz = newcnt * sizeof(size_t);
+        size_t *tmp;
+
+        if (newsz > max_tplcnt
+            || (tmp = OPENSSL_realloc(garg->tuplcnt_arr, newsz)) == NULL)
+            return 0;
+
+        garg->tplmax = newcnt;
+        garg->tuplcnt_arr = tmp;
+    }
+    return 1;
+}
+
+static int close_tuple(gid_cb_st *garg)
+{
+    size_t gidcnt = garg->tuplcnt_arr[garg->tplcnt];
+
+    if (gidcnt == 0)
+        return 1;
+    if (!grow_tuples(garg))
+        return 0;
+
+    garg->tuplcnt_arr[++garg->tplcnt] = 0;
+    return 1;
+}
+
 /* Extract and process a tuple of groups */
 static int tuple_cb(const char *tuple, int len, void *arg)
 {
@@ -1535,17 +1567,9 @@ static int tuple_cb(const char *tuple, int len, void *arg)
         return 0;
     }
 
-    /* Memory management for tuples */
-    if (garg->tplcnt == garg->tplmax) {
-        size_t *tmp =
-            OPENSSL_realloc(garg->tuplcnt_arr,
-                            (garg->tplmax + GROUPLIST_INCREMENT) * sizeof(*garg->tuplcnt_arr));
-
-        if (tmp == NULL)
-            return 0;
-        garg->tplmax += GROUPLIST_INCREMENT;
-        garg->tuplcnt_arr = tmp;
-    }
+    if (garg->inner && !garg->first && !close_tuple(garg))
+        return 0;
+    garg->first = 0;
 
     /* Convert to \0-terminated string */
     restored_tuple_string = OPENSSL_malloc((len + 1 /* \0 */) * sizeof(char));
@@ -1560,15 +1584,8 @@ static int tuple_cb(const char *tuple, int len, void *arg)
     /* We don't need the \o-terminated string anymore */
     OPENSSL_free(restored_tuple_string);
 
-    if (garg->tuplcnt_arr[garg->tplcnt] > 0) { /* Some valid groups are present in current tuple... */
-        if (garg->tuple_mode) {
-            /* We 'close' the tuple */
-            garg->tplcnt++;
-            garg->tuplcnt_arr[garg->tplcnt] = 0; /* Next tuple is initialized to be empty */
-            garg->tuple_mode = 1; /* next call will start a tuple (unless overridden in gid_cb) */
-        }
-    }
-
+    if (!garg->inner && !close_tuple(garg))
+        return 0;
     return retval;
 }
 
@@ -1599,8 +1616,6 @@ int tls1_set_groups_list(SSL_CTX *ctx,
     }
 
     memset(&gcb, 0, sizeof(gcb));
-    gcb.tuple_mode = 1; /* We prepare to collect the first tuple */
-    gcb.ignore_unknown_default = 0;
     gcb.gidmax = GROUPLIST_INCREMENT;
     gcb.tplmax = GROUPLIST_INCREMENT;
     gcb.ksidmax = GROUPLIST_INCREMENT;
diff --git a/test/tls13groupselection_test.c b/test/tls13groupselection_test.c
index 351b3102c7..3c2814c54e 100644
--- a/test/tls13groupselection_test.c
+++ b/test/tls13groupselection_test.c
@@ -38,6 +38,12 @@ typedef enum SERVER_RESPONSE {
     SH   = 2
 } SERVER_RESPONSE;
 
+static const char *response_desc[] = {
+    "HRR",
+    "INIT",
+    "SH",
+};
+
 static char *cert = NULL;
 static char *privkey = NULL;
 
@@ -348,7 +354,26 @@ static const struct tls13groupselection_test_st tls13groupselection_tests[] =
           "X25519",
           SERVER_PREFERENCE,
           NEGOTIATION_FAILURE, INIT
-        }
+        },
+        /* DEFAULT retains tuple structure */
+        { "*X25519:secp256r1",
+          "secp256r1:DEFAULT", /* test 44 */
+          SERVER_PREFERENCE,
+          "secp256r1", HRR
+        },
+#ifndef OPENSSL_NO_DH
+        { "*ffdhe2048:secp256r1",
+          "DEFAULT:ffdhe4096", /* test 45 */
+          CLIENT_PREFERENCE,
+          "secp256r1", HRR
+        },
+        { "x25519:ffdhe2048:*ffdhe4096",
+          "DEFAULT:ffdhe4096", /* test 46 */
+          SERVER_PREFERENCE,
+          "x25519",
+          HRR
+        },
+#endif
     };
 
 static void server_response_check_cb(int write_p, int version,
@@ -492,15 +517,16 @@ static int test_groupnegotiation(const struct tls13groupselection_test_st *curre
         group_name_client = SSL_group_to_name(clientssl, negotiated_group_client);
         if (!TEST_int_eq(negotiated_group_client, negotiated_group_server))
             goto end;
-        if (!TEST_int_eq((int)current_test_vector->expected_server_response, (int)server_response))
+        if (!TEST_str_eq(response_desc[current_test_vector->expected_server_response],
+                response_desc[server_response]))
             goto end;
         if (TEST_str_eq(group_name_client, current_test_vector->expected_group))
             ok = 1;
     } else {
         TEST_false_or_end(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE));
-        if (test_type == TEST_NEGOTIATION_FAILURE &&
-                !TEST_int_eq((int)current_test_vector->expected_server_response,
-                             (int)server_response))
+        if (test_type == TEST_NEGOTIATION_FAILURE
+            && !TEST_str_eq(response_desc[current_test_vector->expected_server_response],
+                response_desc[server_response]))
             goto end;
         ok = 1;
     }
-- 
2.53.0

