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)

(cherry picked from commit 5375e940e22de80ad8c6e865a08db13762242eee)

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index ee9eb656787fad717e312b1ac322b12e7922ef7f..a60171c18732a182d9e8d669e917bad81f039d8c 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: