2 files changed, 173 insertions, 13 deletions

diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c
index e4ba549814..14cf6fce84 100644
--- a/src/lib/libssl/ssl_tlsext.c
+++ b/src/lib/libssl/ssl_tlsext.c
@@ -1,8 +1,8 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.144 2024/03/27 10:44:17 beck Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.145 2024/03/27 22:27:09 beck Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
- * Copyright (c) 2018-2019 Bob Beck <beck@openbsd.org>
+ * Copyright (c) 2018-2019, 2024 Bob Beck <beck@openbsd.org>
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -1464,14 +1464,65 @@ tlsext_keyshare_client_build(SSL *s, uint16_t msg_type, CBB *cbb)
 static int
 tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
 {
-        CBS client_shares, key_exchange;
+        const uint16_t *client_groups = NULL, *server_groups = NULL;
+        size_t client_groups_len = 0, server_groups_len = 0;
+        size_t i, j, client_groups_index;
+        int preferred_group_found = 0;
         int decode_error;
-        uint16_t group;
+        uint16_t group, client_preferred_group;
+        CBS client_shares, key_exchange;
+
+        /*
+         * RFC 8446 section 4.2.8:
+         *
+         * Each KeyShareEntry value MUST correspond to a group offered in the
+         * "supported_groups" extension and MUST appear in the same order.
+         * However, the values MAY be a non-contiguous subset of the
+         * "supported_groups".
+         */
+
+        if (!tlsext_extension_seen(s, TLSEXT_TYPE_supported_groups)) {
+                *alert = SSL_AD_ILLEGAL_PARAMETER;
+                return 0;
+        }
+        if (!tlsext_extension_processed(s, TLSEXT_TYPE_supported_groups)) {
+                *alert = SSL_AD_INTERNAL_ERROR;
+                return 0;
+        }
+
+        /*
+         * XXX similar to tls1_get_supported_group, but client pref
+         * only - consider deduping later.
+         */
+        /*
+         * We are now assured of at least one client group.
+         * Get the client and server group preference orders.
+         */
+        tls1_get_group_list(s, 0, &server_groups, &server_groups_len);
+        tls1_get_group_list(s, 1, &client_groups, &client_groups_len);
+
+        /*
+         * Find the group that is most preferred by the client that
+         * we also support.
+         */
+        for (i = 0; i < client_groups_len && !preferred_group_found; i++) {
+                if (!ssl_security_supported_group(s, client_groups[i]))
+                        continue;
+                for (j = 0; j < server_groups_len; j++) {
+                        if (server_groups[j] == client_groups[i]) {
+                                client_preferred_group = client_groups[i];
+                                preferred_group_found = 1;
+                                break;
+                        }
+                }
+        }
 
         if (!CBS_get_u16_length_prefixed(cbs, &client_shares))
                 return 0;
 
+        client_groups_index = 0;
         while (CBS_len(&client_shares) > 0) {
+                int client_sent_group;
 
                 /* Unpack client share. */
                 if (!CBS_get_u16(&client_shares, &group))
@@ -1480,9 +1531,21 @@ tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
                         return 0;
 
                 /*
-                 * XXX - check key exchange against supported groups from client.
-                 * XXX - check that groups only appear once.
+                 * Ensure the client share group was sent in supported groups,
+                 * and was sent in the same order as supported groups. The
+                 * supported groups has already been checked for duplicates.
                  */
+                client_sent_group = 0;
+                while (client_groups_index < client_groups_len) {
+                        if (group == client_groups[client_groups_index++]) {
+                                client_sent_group = 1;
+                                break;
+                        }
+                }
+                if (!client_sent_group) {
+                        *alert = SSL_AD_ILLEGAL_PARAMETER;
+                        return 0;
+                }
 
                 /*
                  * Ignore this client share if we're using earlier than TLSv1.3
@@ -1493,8 +1556,14 @@ tlsext_keyshare_server_process(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
                 if (s->s3->hs.key_share != NULL)
                         continue;
 
-                /* XXX - consider implementing server preference. */
-                if (!tls1_check_group(s, group))
+                /*
+                 * Ignore this client share if it is not for the most client
+                 * preferred supported group. This avoids a potential downgrade
+                 * situation where the client sends a client share for something
+                 * less preferred, and we choose to to use it instead of
+                 * requesting the more preferred group.
+                 */
+                if (!preferred_group_found || group != client_preferred_group)
                         continue;
 
                 /* Decode and store the selected key share. */
diff --git a/src/regress/lib/libssl/tlsext/tlsexttest.c b/src/regress/lib/libssl/tlsext/tlsexttest.c
index c7983833dc..e61722542e 100644
--- a/src/regress/lib/libssl/tlsext/tlsexttest.c
+++ b/src/regress/lib/libssl/tlsext/tlsexttest.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tlsexttest.c,v 1.86 2024/03/26 02:43:56 beck Exp $ */
+/* $OpenBSD: tlsexttest.c,v 1.87 2024/03/27 22:27:09 beck Exp $ */
 /*
  * Copyright (c) 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -1109,7 +1109,6 @@ test_tlsext_ecpf_client(void)
                 goto err;
         }
 
-
         failure = 0;
 
  err:
@@ -3648,6 +3647,7 @@ test_tlsext_keyshare_client(void)
         const struct tls_extension_funcs *server_funcs;
         int failure;
         size_t dlen;
+        size_t idx;
         int alert;
         CBB cbb;
         CBS cbs;
@@ -3701,18 +3701,109 @@ test_tlsext_keyshare_client(void)
                 goto done;
         }
 
-        (ssl)->version = TLS1_3_VERSION;
-        CBS_init(&cbs, data, dlen);
+        ssl->version = TLS1_3_VERSION;
+
+        /* Fake up the ssl enough so the key share can process */
+        ssl->session = SSL_SESSION_new();
+        ssl->s3 = calloc(1, sizeof(*ssl->s3));
+        ssl->session->tlsext_supportedgroups = calloc(4,
+            sizeof(unsigned short));
+        if (ssl->session == NULL || ssl->s3 == NULL ||
+            ssl->session->tlsext_supportedgroups == NULL) {
+                FAIL("malloc");
+                goto done;
+        }
+        ssl->session->tlsext_supportedgroups[0] = 29;
+        ssl->session->tlsext_supportedgroups[1] = 23;
+        ssl->session->tlsext_supportedgroups[2] = 24;
+        ssl->session->tlsext_supportedgroups[3] = 25;
+        ssl->session->tlsext_supportedgroups_length = 4;
+        tls_extension_find(TLSEXT_TYPE_supported_groups, &idx);
+        ssl->s3->hs.extensions_processed |= (1 << idx);
+        ssl->s3->hs.extensions_seen |= (1 << idx);
+        ssl->s3->hs.our_max_tls_version = TLS1_3_VERSION;
 
+        /*
+         * We should select the key share for group 29, when group 29
+         * is the most preferred group
+         */
+        CBS_init(&cbs, data, dlen);
         if (!server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) {
-                FAIL("failed to parse client keyshare\n");
+                FAIL("failed to process client keyshare\n");
+                goto done;
+        }
+        if (CBS_len(&cbs) != 0) {
+                FAIL("extension data remaining\n");
+                goto done;
+        }
+        if (ssl->s3->hs.key_share == NULL) {
+                FAIL("Did not select a key share");
                 goto done;
         }
 
+        /*
+         * Pretend the client did not send the supported groups extension. We
+         * should fail to process.
+         */
+        ssl->s3->hs.extensions_seen = 0;
+        free(ssl->s3->hs.key_share);
+        ssl->s3->hs.key_share = NULL;
+        CBS_init(&cbs, data, dlen);
+        if (server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) {
+                FAIL("Processed key share when supported groups not provided");
+                goto done;
+        }
+        ssl->s3->hs.extensions_seen |= (1 << idx);
+
+        /*
+         * Pretend supported groups did not get processed. We should fail to
+         * process
+         */
+        ssl->s3->hs.extensions_processed = 0;
+        ssl->s3->hs.key_share = NULL;
+        CBS_init(&cbs, data, dlen);
+        if (server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) {
+                FAIL("Processed key share when supported groups unprocesed");
+                goto done;
+        }
+        ssl->s3->hs.extensions_processed |= (1 << idx);
+
+        /*
+         * Remove group 29 by making it 0xbeef, meaning 29 has not been sent in
+         * supported groups. This should fail to process.
+         */
+        ssl->session->tlsext_supportedgroups[0] = 0xbeef;
+        ssl->s3->hs.key_share = NULL;
+        CBS_init(&cbs, data, dlen);
+        if (server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) {
+                FAIL("Processed key share with invalid group!");
+                goto done;
+        }
+        ssl->session->tlsext_supportedgroups[0] = 29;
+
+        /*
+         * Make 29 least preferred, while server supports both 29 and 25.
+         * Client key share is for 29 but it prefers 25. We should successfully
+         * process, but should not select this key share.
+         */
+        ssl->session->tlsext_supportedgroups[0] = 25;
+        ssl->session->tlsext_supportedgroups[3] = 29;
+        ssl->s3->hs.key_share = NULL;
+        CBS_init(&cbs, data, dlen);
+        if (!server_funcs->process(ssl, SSL_TLSEXT_MSG_CH, &cbs, &alert)) {
+                FAIL("failed to process client keyshare\n");
+                goto done;
+        }
         if (CBS_len(&cbs) != 0) {
                 FAIL("extension data remaining\n");
                 goto done;
         }
+        if (ssl->s3->hs.key_share != NULL) {
+                FAIL("Selected a key share when I should not have!");
+                goto done;
+        }
+        ssl->session->tlsext_supportedgroups[0] = 29;
+        ssl->session->tlsext_supportedgroups[3] = 25;
 
         failure = 0;