From: tb <tb@openbsd.org>
Date: Wed, 20 Jul 2022 15:16:06 +0000 (+0000)
Subject: Simplify tlsext_supported_groups_server_parse
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=90dae725fcf59f84132aec26dc3dd569b667c283;p=openbsd

Simplify tlsext_supported_groups_server_parse

Add an early return in the s->internal->hit case so that we can unindent
a lot of this code. In the HRR case, we do not need to check that the list
of supported groups is unmodified from the first CH. The CH extension
hashing already does that for us.

ok jsing
---

diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index c301b804d25..d802a6e1355 100644
--- a/lib/libssl/ssl_tlsext.c
+++ b/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.124 2022/07/20 14:15:50 tb Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.125 2022/07/20 15:16:06 tb Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -245,7 +245,9 @@ tlsext_supportedgroups_server_parse(SSL *s, uint16_t msg_type, CBS *cbs,
     int *alert)
 {
 	CBS grouplist;
+	uint16_t *groups;
 	size_t groups_len;
+	int i;
 
 	if (!CBS_get_u16_length_prefixed(cbs, &grouplist))
 		goto err;
@@ -257,62 +259,46 @@ tlsext_supportedgroups_server_parse(SSL *s, uint16_t msg_type, CBS *cbs,
 		goto err;
 	groups_len /= 2;
 
-	if (!s->internal->hit) {
-		uint16_t *groups;
-		int i;
-
-		if (s->s3->hs.tls13.hrr) {
-			if (s->session->tlsext_supportedgroups == NULL) {
-				*alert = SSL_AD_HANDSHAKE_FAILURE;
-				return 0;
-			}
-			/*
-			 * In the case of TLSv1.3 the client cannot change
-			 * the supported groups.
-			 */
-			if (groups_len != s->session->tlsext_supportedgroups_length) {
-				*alert = SSL_AD_ILLEGAL_PARAMETER;
-				return 0;
-			}
-			for (i = 0; i < groups_len; i++) {
-				uint16_t group;
-
-				if (!CBS_get_u16(&grouplist, &group))
-					goto err;
-				if (s->session->tlsext_supportedgroups[i] != group) {
-					*alert = SSL_AD_ILLEGAL_PARAMETER;
-					return 0;
-				}
-			}
+	if (s->internal->hit)
+		return 1;
 
-			return 1;
+	if (s->s3->hs.tls13.hrr) {
+		if (s->session->tlsext_supportedgroups == NULL) {
+			*alert = SSL_AD_HANDSHAKE_FAILURE;
+			return 0;
 		}
 
-		if (s->session->tlsext_supportedgroups != NULL)
-			goto err;
+		/*
+		 * The ClientHello extension hashing ensures that the client
+		 * did not change its list of supported groups.
+		 */
 
-		if ((groups = reallocarray(NULL, groups_len,
-		    sizeof(uint16_t))) == NULL) {
-			*alert = SSL_AD_INTERNAL_ERROR;
-			return 0;
-		}
+		return 1;
+	}
 
-		for (i = 0; i < groups_len; i++) {
-			if (!CBS_get_u16(&grouplist, &groups[i])) {
-				free(groups);
-				goto err;
-			}
-		}
+	if (s->session->tlsext_supportedgroups != NULL)
+		goto err;
 
-		if (CBS_len(&grouplist) != 0) {
+	if ((groups = reallocarray(NULL, groups_len, sizeof(uint16_t))) == NULL) {
+		*alert = SSL_AD_INTERNAL_ERROR;
+		return 0;
+	}
+
+	for (i = 0; i < groups_len; i++) {
+		if (!CBS_get_u16(&grouplist, &groups[i])) {
 			free(groups);
 			goto err;
 		}
+	}
 
-		s->session->tlsext_supportedgroups = groups;
-		s->session->tlsext_supportedgroups_length = groups_len;
+	if (CBS_len(&grouplist) != 0) {
+		free(groups);
+		goto err;
 	}
 
+	s->session->tlsext_supportedgroups = groups;
+	s->session->tlsext_supportedgroups_length = groups_len;
+
 	return 1;
 
  err: