Don't do version neg on an HRR

Previously if a client received an HRR then we would do version negotiation
immediately - because we know we are going to get TLSv1.3. However this
causes a problem when we emit the 2nd ClientHello because we start changing
a whole load of stuff to ommit things that aren't relevant for < TLSv1.3.
The spec requires that the 2nd ClientHello is the same except for changes
required from the HRR. Therefore the simplest thing to do is to defer the
version negotiation until we receive the ServerHello.

Fixes #4292

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4527)

diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index ee026228ebc3e056ce52bd6bc6834d6bd5145e6c..e17b2f001a96a5abbfe8c27780d6db311bff7dd8 100644 (file)
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -271,8 +271,13 @@ int ssl3_get_record(SSL *s)
                 thisrr->type = type;
                 thisrr->rec_version = version;
 
-                /* Lets check version. In TLSv1.3 we ignore this field */
+                /*
+                 * Lets check version. In TLSv1.3 we ignore this field. For an
+                 * HRR we haven't actually selected TLSv1.3 yet, but we still
+                 * treat it as TLSv1.3, so we must check for that explicitly
+                 */
                 if (!s->first_packet && !SSL_IS_TLS13(s)
+                        && !s->hello_retry_request
                         && version != (unsigned int)s->version) {
                     SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_WRONG_VERSION_NUMBER);
                     if ((s->version & 0xFF00) == (version & 0xFF00)

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index b5091ac5d6e93b595d686626ab8dabeb06ea3cec..e16b75f683dcd3ee259dcca94fe5f627b1c8445c 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -408,13 +408,23 @@ static int verify_extension(SSL *s, unsigned int context, unsigned int type,
  */
 int extension_is_relevant(SSL *s, unsigned int extctx, unsigned int thisctx)
 {
+    int is_tls13;
+
+    /*
+     * For HRR we haven't selected the version yet but we know it will be
+     * TLSv1.3
+     */
+    if ((thisctx & SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST) != 0)
+        is_tls13 = 1;
+    else
+        is_tls13 = SSL_IS_TLS13(s);
+
     if ((SSL_IS_DTLS(s)
                 && (extctx & SSL_EXT_TLS_IMPLEMENTATION_ONLY) != 0)
             || (s->version == SSL3_VERSION
                     && (extctx & SSL_EXT_SSL3_ALLOWED) == 0)
-            || (SSL_IS_TLS13(s)
-                && (extctx & SSL_EXT_TLS1_2_AND_BELOW_ONLY) != 0)
-            || (!SSL_IS_TLS13(s) && (extctx & SSL_EXT_TLS1_3_ONLY) != 0)
+            || (is_tls13 && (extctx & SSL_EXT_TLS1_2_AND_BELOW_ONLY) != 0)
+            || (!is_tls13 && (extctx & SSL_EXT_TLS1_3_ONLY) != 0)
             || (s->hit && (extctx & SSL_EXT_IGNORE_ON_RESUMPTION) != 0))
         return 0;
 

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 88c08890234378f12f4cfac902edc3a9e584ad57..fdf5d451df317fc33981fa1c1ef269d3fd11478c 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -391,10 +391,6 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s)
         /* We only hit this in the case of HelloRetryRequest */
         return WRITE_TRAN_FINISHED;
 
-    case TLS_ST_CR_HELLO_RETRY_REQUEST:
-        st->hand_state = TLS_ST_CW_CLNT_HELLO;
-        return WRITE_TRAN_CONTINUE;
-
     case TLS_ST_CR_FINISHED:
         if (s->early_data_state == SSL_EARLY_DATA_WRITE_RETRY
                 || s->early_data_state == SSL_EARLY_DATA_FINISHED_WRITING)
@@ -500,6 +496,10 @@ WRITE_TRAN ossl_statem_client_write_transition(SSL *s)
          */
         return WRITE_TRAN_FINISHED;
 
+    case TLS_ST_CR_HELLO_RETRY_REQUEST:
+        st->hand_state = TLS_ST_CW_CLNT_HELLO;
+        return WRITE_TRAN_CONTINUE;
+
     case TLS_ST_EARLY_DATA:
         return WRITE_TRAN_FINISHED;
 
@@ -1558,7 +1558,6 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
 static MSG_PROCESS_RETURN tls_process_hello_retry_request(SSL *s, PACKET *pkt)
 {
     unsigned int sversion;
-    int errorcode;
     const unsigned char *cipherchars;
     RAW_EXTENSION *extensions = NULL;
     int al;
@@ -1579,13 +1578,6 @@ static MSG_PROCESS_RETURN tls_process_hello_retry_request(SSL *s, PACKET *pkt)
     EVP_CIPHER_CTX_free(s->enc_write_ctx);
     s->enc_write_ctx = NULL;
 
-    /* This will fail if it doesn't choose TLSv1.3+ */
-    errorcode = ssl_choose_client_version(s, sversion, 0, &al);
-    if (errorcode != 0) {
-        SSLerr(SSL_F_TLS_PROCESS_HELLO_RETRY_REQUEST, errorcode);
-        goto f_err;
-    }
-
     if (!PACKET_get_bytes(pkt, &cipherchars, TLS_CIPHER_LEN)) {
         SSLerr(SSL_F_TLS_PROCESS_HELLO_RETRY_REQUEST, SSL_R_LENGTH_MISMATCH);
         al = SSL_AD_DECODE_ERROR;

diff --git a/test/recipes/70-test_tls13kexmodes.t b/test/recipes/70-test_tls13kexmodes.t
index fe7415ac560744890ebbc85b5b120d543e4cf23b..3f3cfafa39fd4fdcbd6c030759e37d39df1cc7e8 100644 (file)
--- a/test/recipes/70-test_tls13kexmodes.t
+++ b/test/recipes/70-test_tls13kexmodes.t
@@ -99,12 +99,20 @@ $ENV{CTLOG_FILE} = srctop_file("test", "ct", "log_list.conf");
         checkhandshake::STATUS_REQUEST_CLI_EXTENSION],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SUPPORTED_GROUPS,
         checkhandshake::DEFAULT_EXTENSIONS],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_EC_POINT_FORMATS,
+        checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SIG_ALGS,
         checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_ALPN,
         checkhandshake::ALPN_CLI_EXTENSION],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SCT,
         checkhandshake::SCT_CLI_EXTENSION],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_ENCRYPT_THEN_MAC,
+        checkhandshake::DEFAULT_EXTENSIONS],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET,
+        checkhandshake::DEFAULT_EXTENSIONS],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SESSION_TICKET,
+        checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_KEY_SHARE,
         checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS,

diff --git a/test/recipes/70-test_tls13messages.t b/test/recipes/70-test_tls13messages.t
index 24ffb80b2ed596090b209167705344a36a01dcc7..239eabfd5e7ac2be9f03e4e192225c2c78d1b26e 100644 (file)
--- a/test/recipes/70-test_tls13messages.t
+++ b/test/recipes/70-test_tls13messages.t
@@ -99,12 +99,20 @@ $ENV{CTLOG_FILE} = srctop_file("test", "ct", "log_list.conf");
         checkhandshake::STATUS_REQUEST_CLI_EXTENSION],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SUPPORTED_GROUPS,
         checkhandshake::DEFAULT_EXTENSIONS],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_EC_POINT_FORMATS,
+        checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SIG_ALGS,
         checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_ALPN,
         checkhandshake::ALPN_CLI_EXTENSION],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SCT,
         checkhandshake::SCT_CLI_EXTENSION],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_ENCRYPT_THEN_MAC,
+        checkhandshake::DEFAULT_EXTENSIONS],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET,
+        checkhandshake::DEFAULT_EXTENSIONS],
+    [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SESSION_TICKET,
+        checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_KEY_SHARE,
         checkhandshake::DEFAULT_EXTENSIONS],
     [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS,