Improve TLSv1.3 state machine for HelloRetryRequest handling.

The state machine currently handles the HelloRetryRequest case by using
WITH_HRR - in other words, we're explicitly indicating when we transition
to the alternate path. The problem here is that we do not know if we're
going to receive a ServerHello or a HelloRetryRequest until we process
the message. This means that the ServerHello processing code has to handle
both types of messages.

The state machine and associated processing code becomes cleaner if we flip
this around so that we assume we are going to receive a HelloRetryRequest
and upon discovering that it is not, trigger WITHOUT_HRR and hand off to
the ServerHello processing function. In particular, this makes the logic
much more straight forward on the server side, when adding support for HRR.

With feedback from tb@

ok tb@

5 files changed, 104 insertions, 66 deletions

diff --git a/src/lib/libssl/tls13_client.c b/src/lib/libssl/tls13_client.c
index 0da08f62c3..dffabf1753 100644
--- a/src/lib/libssl/tls13_client.c
+++ b/src/lib/libssl/tls13_client.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_client.c,v 1.50 2020/04/21 16:55:17 jsing Exp $ */
+/* $OpenBSD: tls13_client.c,v 1.51 2020/04/22 17:05:07 jsing Exp $ */
 /*
  * Copyright (c) 2018, 2019 Joel Sing <jsing@openbsd.org>
  *
@@ -281,6 +281,24 @@ tls13_server_hello_is_legacy(CBS *cbs)
 }
 
 static int
+tls13_server_hello_is_retry(CBS *cbs)
+{
+        CBS server_hello, server_random;
+        uint16_t legacy_version;
+
+        CBS_dup(cbs, &server_hello);
+
+        if (!CBS_get_u16(&server_hello, &legacy_version))
+                return 0;
+        if (!CBS_get_bytes(&server_hello, &server_random, SSL3_RANDOM_SIZE))
+                return 0;
+
+        /* See if this is a HelloRetryRequest. */
+        return CBS_mem_equal(&server_random, tls13_hello_retry_request_hash,
+            sizeof(tls13_hello_retry_request_hash));
+}
+
+static int
 tls13_server_hello_process(struct tls13_ctx *ctx, CBS *cbs)
 {
         CBS server_random, session_id;
@@ -331,7 +349,8 @@ tls13_server_hello_process(struct tls13_ctx *ctx, CBS *cbs)
         /* From here on in we know we are doing TLSv1.3. */
         tls13_record_layer_allow_legacy_alerts(ctx->rl, 0);
 
-        /* See if this is a Hello Retry Request. */
+        /* See if this is a HelloRetryRequest. */
+        /* XXX - see if we can avoid doing this twice. */
         if (CBS_mem_equal(&server_random, tls13_hello_retry_request_hash,
             sizeof(tls13_hello_retry_request_hash))) {
                 tlsext_msg_type = SSL_TLSEXT_MSG_HRR;
@@ -515,40 +534,75 @@ tls13_client_engage_record_protection(struct tls13_ctx *ctx)
 }
 
 int
+tls13_server_hello_retry_request_recv(struct tls13_ctx *ctx, CBS *cbs)
+{
+        /*
+         * The state machine has no way of knowing if we're going to receive a
+         * HelloRetryRequest or a ServerHello. As such, we have to handle
+         * this case here and hand off to the appropriate function.
+         */
+        if (!tls13_server_hello_is_retry(cbs)) {
+                ctx->handshake_stage.hs_type |= WITHOUT_HRR;
+                return tls13_server_hello_recv(ctx, cbs);
+        }
+
+        if (!tls13_server_hello_process(ctx, cbs))
+                return 0;
+
+        /*
+         * This may have been a TLSv1.2 or earlier ServerHello that just happened
+         * to have matching server random...
+         */
+        if (ctx->hs->use_legacy)
+                return tls13_use_legacy_client(ctx);
+
+        if (!ctx->hs->hrr)
+                return 0;
+
+        if (!tls13_client_synthetic_handshake_message(ctx))
+                return 0;
+        if (!tls13_handshake_msg_record(ctx))
+                return 0;
+
+        ctx->hs->hrr = 0;
+
+        return 1;
+}
+
+int
 tls13_server_hello_recv(struct tls13_ctx *ctx, CBS *cbs)
 {
         SSL *s = ctx->ssl;
 
         /*
-         * We may have received a legacy (pre-TLSv1.3) server hello,
-         * a TLSv1.3 server hello or a TLSv1.3 hello retry request.
+         * We may have received a legacy (pre-TLSv1.3) ServerHello or a TLSv1.3
+         * ServerHello. HelloRetryRequests have already been handled.
          */
         if (!tls13_server_hello_process(ctx, cbs))
                 return 0;
 
-        tls1_transcript_unfreeze(s);
-
-        if (ctx->hs->hrr) {
-                if (!tls13_client_synthetic_handshake_message(ctx))
+        if (ctx->handshake_stage.hs_type & WITHOUT_HRR) {
+                tls1_transcript_unfreeze(s);
+                if (!tls13_handshake_msg_record(ctx))
                         return 0;
         }
 
-        if (!tls13_handshake_msg_record(ctx))
-                return 0;
-
-        if (ctx->hs->use_legacy)
+        if (ctx->hs->use_legacy) {
+                if (!(ctx->handshake_stage.hs_type & WITHOUT_HRR))
+                        return 0;
                 return tls13_use_legacy_client(ctx);
+        }
 
-        if (!ctx->hs->hrr) {
-                if (!tls13_client_engage_record_protection(ctx))
-                        return 0;
+        if (ctx->hs->hrr) {
+                /* The server has sent two HelloRetryRequests. */
+                ctx->alert = SSL_AD_ILLEGAL_PARAMETER;
+                return 0;
         }
 
-        ctx->handshake_stage.hs_type |= NEGOTIATED;
-        if (ctx->hs->hrr)
-                ctx->handshake_stage.hs_type |= WITH_HRR;
+        if (!tls13_client_engage_record_protection(ctx))
+                return 0;
 
-        ctx->hs->hrr = 0;
+        ctx->handshake_stage.hs_type |= NEGOTIATED;
 
         return 1;
 }
@@ -581,24 +635,6 @@ tls13_client_hello_retry_send(struct tls13_ctx *ctx, CBB *cbb)
 }
 
 int
-tls13_server_hello_retry_recv(struct tls13_ctx *ctx, CBS *cbs)
-{
-        if (!tls13_server_hello_process(ctx, cbs))
-                return 0;
-
-        if (ctx->hs->use_legacy)
-                return 0; /* XXX alert */
-
-        if (ctx->hs->hrr)
-                return 0; /* XXX alert */
-
-        if (!tls13_client_engage_record_protection(ctx))
-                return 0;
-
-        return 1;
-}
-
-int
 tls13_server_encrypted_extensions_recv(struct tls13_ctx *ctx, CBS *cbs)
 {
         int alert_desc;
diff --git a/src/lib/libssl/tls13_handshake.c b/src/lib/libssl/tls13_handshake.c
index 51585d31ba..86046144de 100644
--- a/src/lib/libssl/tls13_handshake.c
+++ b/src/lib/libssl/tls13_handshake.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: tls13_handshake.c,v 1.52 2020/03/10 17:15:02 jsing Exp $      */
+/*      $OpenBSD: tls13_handshake.c,v 1.53 2020/04/22 17:05:07 jsing Exp $      */
 /*
  * Copyright (c) 2018-2019 Theo Buehler <tb@openbsd.org>
  * Copyright (c) 2019 Joel Sing <jsing@openbsd.org>
@@ -96,11 +96,11 @@ struct tls13_handshake_action state_machine[] = {
                 .sent = tls13_server_hello_sent,
                 .recv = tls13_server_hello_recv,
         },
-        [SERVER_HELLO_RETRY] = {
+        [SERVER_HELLO_RETRY_REQUEST] = {
                 .handshake_type = TLS13_MT_SERVER_HELLO,
                 .sender = TLS13_HS_SERVER,
-                .send = tls13_server_hello_retry_send,
-                .recv = tls13_server_hello_retry_recv,
+                .send = tls13_server_hello_retry_request_send,
+                .recv = tls13_server_hello_retry_request_recv,
         },
         [SERVER_ENCRYPTED_EXTENSIONS] = {
                 .handshake_type = TLS13_MT_ENCRYPTED_EXTENSIONS,
@@ -145,10 +145,14 @@ struct tls13_handshake_action state_machine[] = {
 enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
         [INITIAL] = {
                 CLIENT_HELLO,
+                SERVER_HELLO_RETRY_REQUEST,
+                CLIENT_HELLO_RETRY,
                 SERVER_HELLO,
         },
         [NEGOTIATED] = {
                 CLIENT_HELLO,
+                SERVER_HELLO_RETRY_REQUEST,
+                CLIENT_HELLO_RETRY,
                 SERVER_HELLO,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_CERTIFICATE_REQUEST,
@@ -159,11 +163,9 @@ enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
                 CLIENT_FINISHED,
                 APPLICATION_DATA,
         },
-        [NEGOTIATED | WITH_HRR] = {
+        [NEGOTIATED | WITHOUT_HRR] = {
                 CLIENT_HELLO,
                 SERVER_HELLO,
-                CLIENT_HELLO_RETRY,
-                SERVER_HELLO_RETRY,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_CERTIFICATE_REQUEST,
                 SERVER_CERTIFICATE,
@@ -175,6 +177,8 @@ enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
         },
         [NEGOTIATED | WITHOUT_CR] = {
                 CLIENT_HELLO,
+                SERVER_HELLO_RETRY_REQUEST,
+                CLIENT_HELLO_RETRY,
                 SERVER_HELLO,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_CERTIFICATE,
@@ -183,11 +187,9 @@ enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
                 CLIENT_FINISHED,
                 APPLICATION_DATA,
         },
-        [NEGOTIATED | WITH_HRR | WITHOUT_CR] = {
+        [NEGOTIATED | WITHOUT_HRR | WITHOUT_CR] = {
                 CLIENT_HELLO,
                 SERVER_HELLO,
-                CLIENT_HELLO_RETRY,
-                SERVER_HELLO_RETRY,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_CERTIFICATE,
                 SERVER_CERTIFICATE_VERIFY,
@@ -197,17 +199,17 @@ enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
         },
         [NEGOTIATED | WITH_PSK] = {
                 CLIENT_HELLO,
+                SERVER_HELLO_RETRY_REQUEST,
+                CLIENT_HELLO_RETRY,
                 SERVER_HELLO,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_FINISHED,
                 CLIENT_FINISHED,
                 APPLICATION_DATA,
         },
-        [NEGOTIATED | WITH_HRR | WITH_PSK] = {
+        [NEGOTIATED | WITHOUT_HRR | WITH_PSK] = {
                 CLIENT_HELLO,
                 SERVER_HELLO,
-                CLIENT_HELLO_RETRY,
-                SERVER_HELLO_RETRY,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_FINISHED,
                 CLIENT_FINISHED,
@@ -215,6 +217,8 @@ enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
         },
         [NEGOTIATED | WITH_CCV] = {
                 CLIENT_HELLO,
+                SERVER_HELLO_RETRY_REQUEST,
+                CLIENT_HELLO_RETRY,
                 SERVER_HELLO,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_CERTIFICATE_REQUEST,
@@ -226,11 +230,9 @@ enum tls13_message_type handshakes[][TLS13_NUM_MESSAGE_TYPES] = {
                 CLIENT_FINISHED,
                 APPLICATION_DATA,
         },
-        [NEGOTIATED | WITH_HRR | WITH_CCV] = {
+        [NEGOTIATED | WITHOUT_HRR | WITH_CCV] = {
                 CLIENT_HELLO,
                 SERVER_HELLO,
-                CLIENT_HELLO_RETRY,
-                SERVER_HELLO_RETRY,
                 SERVER_ENCRYPTED_EXTENSIONS,
                 SERVER_CERTIFICATE_REQUEST,
                 SERVER_CERTIFICATE,
diff --git a/src/lib/libssl/tls13_handshake.h b/src/lib/libssl/tls13_handshake.h
index 956d27c61a..8a08b9fd5b 100644
--- a/src/lib/libssl/tls13_handshake.h
+++ b/src/lib/libssl/tls13_handshake.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_handshake.h,v 1.4 2020/03/10 17:15:02 jsing Exp $ */
+/* $OpenBSD: tls13_handshake.h,v 1.5 2020/04/22 17:05:07 jsing Exp $ */
 /*
  * Copyright (c) 2019 Theo Buehler <tb@openbsd.org>
  *
@@ -24,7 +24,7 @@ __BEGIN_HIDDEN_DECLS
 
 #define INITIAL                 0x00
 #define NEGOTIATED              0x01
-#define WITH_HRR                0x02
+#define WITHOUT_HRR             0x02
 #define WITHOUT_CR              0x04
 #define WITH_PSK                0x08
 #define WITH_CCV                0x10
@@ -33,9 +33,9 @@ __BEGIN_HIDDEN_DECLS
 enum tls13_message_type {
         INVALID,
         CLIENT_HELLO,
-        SERVER_HELLO,
+        SERVER_HELLO_RETRY_REQUEST,
         CLIENT_HELLO_RETRY,
-        SERVER_HELLO_RETRY,
+        SERVER_HELLO,
         SERVER_ENCRYPTED_EXTENSIONS,
         SERVER_CERTIFICATE_REQUEST,
         SERVER_CERTIFICATE,
diff --git a/src/lib/libssl/tls13_internal.h b/src/lib/libssl/tls13_internal.h
index c5b893bc16..ee82a44693 100644
--- a/src/lib/libssl/tls13_internal.h
+++ b/src/lib/libssl/tls13_internal.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_internal.h,v 1.64 2020/04/21 16:55:17 jsing Exp $ */
+/* $OpenBSD: tls13_internal.h,v 1.65 2020/04/22 17:05:07 jsing Exp $ */
 /*
  * Copyright (c) 2018 Bob Beck <beck@openbsd.org>
  * Copyright (c) 2018 Theo Buehler <tb@openbsd.org>
@@ -309,8 +309,8 @@ int tls13_client_finished_sent(struct tls13_ctx *ctx);
 int tls13_server_hello_recv(struct tls13_ctx *ctx, CBS *cbs);
 int tls13_server_hello_send(struct tls13_ctx *ctx, CBB *cbb);
 int tls13_server_hello_sent(struct tls13_ctx *ctx);
-int tls13_server_hello_retry_recv(struct tls13_ctx *ctx, CBS *cbs);
-int tls13_server_hello_retry_send(struct tls13_ctx *ctx, CBB *cbb);
+int tls13_server_hello_retry_request_recv(struct tls13_ctx *ctx, CBS *cbs);
+int tls13_server_hello_retry_request_send(struct tls13_ctx *ctx, CBB *cbb);
 int tls13_server_encrypted_extensions_recv(struct tls13_ctx *ctx, CBS *cbs);
 int tls13_server_encrypted_extensions_send(struct tls13_ctx *ctx, CBB *cbb);
 int tls13_server_certificate_recv(struct tls13_ctx *ctx, CBS *cbs);
diff --git a/src/lib/libssl/tls13_server.c b/src/lib/libssl/tls13_server.c
index f3d21a7477..9bc4cb6170 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.30 2020/04/21 17:06:16 jsing Exp $ */
+/* $OpenBSD: tls13_server.c,v 1.31 2020/04/22 17:05:07 jsing Exp $ */
 /*
  * Copyright (c) 2019, 2020 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
@@ -280,11 +280,11 @@ tls13_client_hello_recv(struct tls13_ctx *ctx, CBS *cbs)
                 return 1;
 
         /*
-         * If no matching key share was provided, we need to send a
-         * HelloRetryRequest, if matching security parameters exist.
+         * If a matching key share was provided, we do not need to
+         * send a HelloRetryRequest.
          */
-        if (ctx->hs->key_share == NULL)
-                ctx->handshake_stage.hs_type |= WITH_HRR;
+        if (ctx->hs->key_share != NULL)
+                ctx->handshake_stage.hs_type |= WITHOUT_HRR;
 
         /* XXX - check this is the correct point */
         tls13_record_layer_allow_ccs(ctx->rl, 1);
@@ -608,7 +608,7 @@ tls13_server_hello_sent(struct tls13_ctx *ctx)
 }
 
 int
-tls13_server_hello_retry_send(struct tls13_ctx *ctx, CBB *cbb)
+tls13_server_hello_retry_request_send(struct tls13_ctx *ctx, CBB *cbb)
 {
         return 0;
 }