Do not assume that server_group != 0 or tlsext_supportedgroups != NULL

implies that we're dealing with a HRR in the extension handling code.

Explicitly check that we're in this situation by inspecting the flag in
the handshake context. Add missing error checks and send the appropriate
alerts. The hrr flag needs to be unset after parsing the client hello
retry to avoid breaking the server hello handling. All this is far from
ideal, but better than nothing.

The correct fix would likely be to make the message type available
but that would need to be part of a more extensive rearchitecture of
the extension handling.

Discussed at length with jsing

2 files changed, 15 insertions, 9 deletions

diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c
index 8949dc3a26..f5343c81ab 100644
--- a/src/lib/libssl/ssl_tlsext.c
+++ b/src/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.70 2020/05/19 02:16:16 beck Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.71 2020/05/23 08:47:19 tb Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -222,13 +222,15 @@ tlsext_supportedgroups_server_parse(SSL *s, CBS *cbs, int *alert)
                 uint16_t *groups;
                 int i;
 
-                if (SSI(s)->tlsext_supportedgroups != NULL) {
+                if (S3I(s)->hs_tls13.hrr) {
+                        if (SSI(s)->tlsext_supportedgroups == NULL) {
+                                *alert = SSL_AD_HANDSHAKE_FAILURE;
+                                return 0;
+                        }
                         /*
-                         * We should only end up here in the case of a TLSv1.3
-                         * HelloRetryRequest... and the client cannot change
-                         * supported groups.
+                         * In the case of TLSv1.3 the client cannot change
+                         * the supported groups.
                          */
-                        /* XXX - we should know this is a HRR. */
                         if (groups_len != SSI(s)->tlsext_supportedgroups_length) {
                                 *alert = SSL_AD_ILLEGAL_PARAMETER;
                                 return 0;
@@ -1410,9 +1412,11 @@ int
 tlsext_keyshare_server_build(SSL *s, CBB *cbb)
 {
         /* In the case of a HRR, we only send the server selected group. */
-        /* XXX - we should know this is a HRR. */
-        if (S3I(s)->hs_tls13.server_group != 0)
+        if (S3I(s)->hs_tls13.hrr) {
+                if (S3I(s)->hs_tls13.server_group == 0)
+                        return 0;
                 return CBB_add_u16(cbb, S3I(s)->hs_tls13.server_group);
+        }
 
         if (S3I(s)->hs_tls13.key_share == NULL)
                 return 0;
diff --git a/src/lib/libssl/tls13_server.c b/src/lib/libssl/tls13_server.c
index e0ea6b564d..e605ccd90f 100644
--- a/src/lib/libssl/tls13_server.c
+++ b/src/lib/libssl/tls13_server.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_server.c,v 1.51 2020/05/22 02:37:27 beck Exp $ */
+/* $OpenBSD: tls13_server.c,v 1.52 2020/05/23 08:47:19 tb Exp $ */
 /*
  * Copyright (c) 2019, 2020 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
@@ -365,6 +365,8 @@ tls13_client_hello_retry_recv(struct tls13_ctx *ctx, CBS *cbs)
         if (s->method->internal->version < TLS1_3_VERSION)
                 return 0;
 
+        ctx->hs->hrr = 0;
+
         return 1;
 }