Do not make key share choice in tls1_set_groups()

tls1_set_groups(), which is used by SSL_CTX_set1_groups() does not check
whether the NIDs passed as argument actually have an implementation
available in any of the currently loaded providers. It is not simple to
add this check, either, because it would require access to the SSL_CTX,
which this function does not receive. There are legacy callers that do
not have an SSL_CTX pointer and are public API.

This becomes a problem, when an application sets the first group to one
that is not supported by the current configuration, and can trigger
sending of an empty key share.

Set the first entry of the key share list to 0 (and the key share list
length to 1) to signal to tls1_construct_ctos_key_share that it should
pick the first supported group and generate a key share for that. See
also tls1_get_requested_keyshare_groups, which documents this special
case.

See: https://issues.redhat.com/browse/RHEL-128018
Signed-off-by: Clemens Lang <cllang@redhat.com>
Reviewed-by: Norbert Pocs <norbertp@openssl.org>
Reviewed-by: Simo Sorce <simo@redhat.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/29192)

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 9c63569e74e0615e3a5fccb3cec4d49bdb0118cd..838edcd3bc30340df5a94115d55646d320b8b4ac 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1129,7 +1129,13 @@ int tls1_set_groups(uint16_t **grpext, size_t *grpextlen,
     OPENSSL_free(*tplext);
     *grpext = glist;
     *grpextlen = ngroups;
-    kslist[0] = glist[0];
+    /*
+     * No * prefix was used, let tls_construct_ctos_key_share choose a key
+     * share. This has the advantage that it will filter unsupported groups
+     * before choosing one, which this function does not do. See also the
+     * comment for tls1_get_requested_keyshare_groups.
+     */
+    kslist[0] = 0;
     *ksext = kslist;
     *ksextlen = 1;
     tpllist[0] = ngroups;

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 02c78ece8e729488bd3d3488ba2a07d74e4ddc34..9105552bdb15aa13ed5cd109f8d2522201159336 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -13811,6 +13811,58 @@ static int test_ssl_trace(void)
 }
 #endif
 
+/*
+ * Test that SSL_CTX_set1_groups() when called with a list where the first
+ * entry is unsupported, will send a key_share that uses the next usable entry.
+ */
+static int test_ssl_set_groups_unsupported_keyshare(void)
+{
+#if !defined(OPENSSL_NO_EC) || !defined(OPENSSL_NO_DH)
+    int testresult = 0;
+    SSL_CTX *sctx = NULL, *cctx = NULL;
+    SSL *serverssl = NULL, *clientssl = NULL;
+    int client_groups[] = {
+        NID_brainpoolP256r1tls13,
+        NID_sect163k1,
+        NID_secp384r1,
+        NID_ffdhe2048,
+    };
+
+    if (!TEST_true(create_ssl_ctx_pair(libctx,
+                                       TLS_server_method(),
+                                       TLS_client_method(),
+                                       0, 0,
+                                       &sctx,
+                                       &cctx,
+                                       cert,
+                                       privkey)))
+        goto end;
+
+    if (!TEST_true(SSL_CTX_set1_groups(cctx,
+                                       client_groups,
+                                       OSSL_NELEM(client_groups))))
+        goto end;
+
+    if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, NULL,
+                                      NULL)))
+        goto end;
+
+    if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE)))
+        goto end;
+
+    testresult = 1;
+ end:
+    SSL_free(serverssl);
+    SSL_free(clientssl);
+    SSL_CTX_free(sctx);
+    SSL_CTX_free(cctx);
+
+    return testresult;
+#else /* !defined(OPENSSL_NO_EC) || !defined(OPENSSL_NO_DH) */
+    return TEST_skip("No EC and DH support.");
+#endif /* !defined(OPENSSL_NO_EC) || !defined(OPENSSL_NO_DH) */
+}
+
 OPT_TEST_DECLARE_USAGE("certfile privkeyfile srpvfile tmpfile provider config dhfile\n")
 
 int setup_tests(void)
@@ -14152,6 +14204,7 @@ int setup_tests(void)
     if (datadir != NULL)
         ADD_TEST(test_ssl_trace);
 #endif
+    ADD_TEST(test_ssl_set_groups_unsupported_keyshare);
     return 1;
 
  err: