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;
 }