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 c301b80..d802a6e 100644 (file)
--- 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: