Fix up server processing of key shares.

Ensure that the client can not provide a duplicate key share
for any group, or send more key shares than groups they support.

Ensure that the key shares must be provided in the same order
as the client preference order specified in supported_groups.

Ensure we only will choose to use a key share that is for the
most preferred group by the client that we also support,
to avoid the client being downgraded by sending a less preferred
key share. If we do not end up with a key share for the most preferred
mutually supported group, will then do a hello retry request
selecting that group.

Add regress for this to regress/tlsext/tlsexttest.c

ok jsing@

1 files changed, 77 insertions, 8 deletions

diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index e4ba5498142..14cf6fce84b 100644
--- a/lib/libssl/ssl_tlsext.c
+++ b/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. */