Update state machine to send CCS based on whether we did an HRR

The CCS may be sent at different times based on whether or not we
sent an HRR earlier. In order to make that decision this commit
also updates things to make sure we remember whether an HRR was
used or not.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4701)

diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index c1776e98f1cdff7bdf2b742dcc36b83cd70b2a8e..28ee2cc00587aa4d6af6371b24d777fbcb13394b 100644 (file)
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -276,7 +276,7 @@ int ssl3_get_record(SSL *s)
                  * that explicitly
                  */
                 if (!s->first_packet && !SSL_IS_TLS13(s)
-                        && !s->hello_retry_request
+                        && s->hello_retry_request != SSL_HRR_PENDING
                         && version != (unsigned int)s->version) {
                     if ((s->version & 0xFF00) == (version & 0xFF00)
                         && !s->enc_write_ctx && !s->write_hash) {
@@ -449,7 +449,7 @@ int ssl3_get_record(SSL *s)
 
     if (num_recs == 1
             && thisrr->type == SSL3_RT_CHANGE_CIPHER_SPEC
-            && SSL_IS_TLS13(s)
+            && (SSL_IS_TLS13(s) || s->hello_retry_request != SSL_HRR_NONE)
             && SSL_IS_FIRST_HANDSHAKE(s)) {
         /*
          * CCS messages must be exactly 1 byte long, containing the value 0x01

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 23cb31ab69080b69b1be78c844f40278be3997bc..6b899691f94e2d41bdc68597dd38c7a8a74bd639 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1117,7 +1117,8 @@ struct ssl_st {
     size_t cert_verify_hash_len;
 
     /* Flag to indicate whether we should send a HelloRetryRequest or not */
-    int hello_retry_request;
+    enum {SSL_HRR_NONE = 0, SSL_HRR_PENDING, SSL_HRR_COMPLETE}
+        hello_retry_request;
 
     /*
      * the session_id_context is used to ensure sessions are only reused in

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 988e91904438e6cfa16e94c650d820ce8f1eaf1e..026126d4d888fa703587922e91026fd112988561 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1235,7 +1235,7 @@ static int final_key_share(SSL *s, unsigned int context, int sent)
      */
     if (s->server && s->s3->peer_tmp == NULL) {
         /* No suitable share */
-        if (s->hello_retry_request == 0 && sent
+        if (s->hello_retry_request == SSL_HRR_NONE && sent
                 && (!s->hit
                     || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE)
                        != 0)) {
@@ -1260,7 +1260,7 @@ static int final_key_share(SSL *s, unsigned int context, int sent)
             if (i < num_groups) {
                 /* A shared group exists so send a HelloRetryRequest */
                 s->s3->group_id = group_id;
-                s->hello_retry_request = 1;
+                s->hello_retry_request = SSL_HRR_PENDING;
                 return 1;
             }
         }
@@ -1275,8 +1275,8 @@ static int final_key_share(SSL *s, unsigned int context, int sent)
     }
 
     /* We have a key_share so don't send any more HelloRetryRequest messages */
-    if (s->server)
-        s->hello_retry_request = 0;
+    if (s->server && s->hello_retry_request == SSL_HRR_PENDING)
+        s->hello_retry_request = SSL_HRR_COMPLETE;
 
     /*
      * For a client side resumption with no key_share we need to generate
@@ -1405,7 +1405,7 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
      * following a HelloRetryRequest then this includes the hash of the first
      * ClientHello and the HelloRetryRequest itself.
      */
-    if (s->hello_retry_request) {
+    if (s->hello_retry_request == SSL_HRR_PENDING) {
         size_t hdatalen;
         void *hdata;
 
@@ -1516,7 +1516,7 @@ static int final_early_data(SSL *s, unsigned int context, int sent)
             || s->session->ext.tick_identity != 0
             || s->early_data_state != SSL_EARLY_DATA_ACCEPTING
             || !s->ext.early_data_ok
-            || s->hello_retry_request) {
+            || s->hello_retry_request != SSL_HRR_NONE) {
         s->ext.early_data = SSL_EARLY_DATA_REJECTED;
     } else {
         s->ext.early_data = SSL_EARLY_DATA_ACCEPTED;

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 2640756134f14323dd74539a358fe5fa08eb279a..1fbf9f6e0e6580ae56851861456ef7dabb9319ce 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -599,7 +599,7 @@ static int add_key_share(SSL *s, WPACKET *pkt, unsigned int curve_id)
     size_t encodedlen;
 
     if (s->s3->tmp.pkey != NULL) {
-        if (!ossl_assert(s->hello_retry_request)) {
+        if (!ossl_assert(s->hello_retry_request == SSL_HRR_PENDING)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_ADD_KEY_SHARE,
                      ERR_R_INTERNAL_ERROR);
             return 0;
@@ -749,7 +749,7 @@ EXT_RETURN tls_construct_ctos_early_data(SSL *s, WPACKET *pkt,
     SSL_SESSION *edsess = NULL;
     const EVP_MD *handmd = NULL;
 
-    if (s->hello_retry_request)
+    if (s->hello_retry_request == SSL_HRR_PENDING)
         handmd = ssl_handshake_md(s);
 
     if (s->psk_use_session_cb != NULL
@@ -961,7 +961,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
             || (s->session->ext.ticklen == 0 && s->psksession == NULL))
         return EXT_RETURN_NOT_SENT;
 
-    if (s->hello_retry_request)
+    if (s->hello_retry_request == SSL_HRR_PENDING)
         handmd = ssl_handshake_md(s);
 
     if (s->session->ext.ticklen != 0) {
@@ -980,7 +980,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
             goto dopsksess;
         }
 
-        if (s->hello_retry_request && mdres != handmd) {
+        if (s->hello_retry_request == SSL_HRR_PENDING && mdres != handmd) {
             /*
              * Selected ciphersuite hash does not match the hash for the session
              * so we can't use it.
@@ -1044,7 +1044,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
             return EXT_RETURN_FAIL;
         }
 
-        if (s->hello_retry_request && mdpsk != handmd) {
+        if (s->hello_retry_request == SSL_HRR_PENDING && mdpsk != handmd) {
             /*
              * Selected ciphersuite hash does not match the hash for the PSK
              * session. This is an application bug.

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 93ac98f11608bd791248c3a6974ace4b2940b012..d34a7c5ee5dd7f30edc49a02f4b2061dc507cac3 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -704,7 +704,7 @@ int tls_parse_ctos_early_data(SSL *s, PACKET *pkt, unsigned int context,
         return 0;
     }
 
-    if (s->hello_retry_request) {
+    if (s->hello_retry_request != SSL_HRR_NONE) {
         SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
                  SSL_F_TLS_PARSE_CTOS_EARLY_DATA, SSL_R_BAD_EXTENSION);
         return 0;
@@ -1245,7 +1245,7 @@ EXT_RETURN tls_construct_stoc_key_share(SSL *s, WPACKET *pkt,
 
     if (ckey == NULL) {
         /* No key_share received from client */
-        if (s->hello_retry_request) {
+        if (s->hello_retry_request == SSL_HRR_PENDING) {
             if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share)
                     || !WPACKET_start_sub_packet_u16(pkt)
                     || !WPACKET_put_bytes_u16(pkt, s->s3->group_id)

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index af9e1dcd7d261494e7b6e8f557afd1251dbea599..80148fa5319ce6fb9fd1615059f213da1ed83dcb 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -387,7 +387,7 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s)
                 || s->early_data_state == SSL_EARLY_DATA_FINISHED_WRITING)
             st->hand_state = TLS_ST_PENDING_EARLY_DATA_END;
         else if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0
-                    && !s->hello_retry_request)
+                 && s->hello_retry_request == SSL_HRR_NONE)
             st->hand_state = TLS_ST_CW_CHANGE;
         else
             st->hand_state = (s->s3->tmp.cert_req != 0) ? TLS_ST_CW_CERT
@@ -1055,7 +1055,8 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
     if (sess == NULL
             || !ssl_version_supported(s, sess->ssl_version)
             || !SSL_SESSION_is_resumable(sess)) {
-        if (!s->hello_retry_request && !ssl_get_new_session(s, 0)) {
+        if (s->hello_retry_request == SSL_HRR_NONE
+                && !ssl_get_new_session(s, 0)) {
             /* SSLfatal() already called */
             return 0;
         }
@@ -1078,7 +1079,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
             }
         }
     } else {
-        i = s->hello_retry_request == 0;
+        i = (s->hello_retry_request == SSL_HRR_NONE);
     }
 
     if (i && ssl_fill_hello_random(s, 0, p, sizeof(s->s3->client_random),
@@ -1136,7 +1137,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
             sess_id_len = sizeof(s->tmp_session_id);
             s->tmp_session_id_len = sess_id_len;
             session_id = s->tmp_session_id;
-            if (!s->hello_retry_request
+            if (s->hello_retry_request == SSL_HRR_NONE
                     && ssl_randbytes(s, s->tmp_session_id,
                                      sess_id_len) <= 0) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR,
@@ -1360,7 +1361,8 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
             && sversion == TLS1_2_VERSION
             && PACKET_remaining(pkt) >= SSL3_RANDOM_SIZE
             && memcmp(hrrrandom, PACKET_data(pkt), SSL3_RANDOM_SIZE) == 0) {
-        s->hello_retry_request = hrr = 1;
+        s->hello_retry_request = SSL_HRR_PENDING;
+        hrr = 1;
         if (!PACKET_forward(pkt, SSL3_RANDOM_SIZE)) {
             SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PROCESS_SERVER_HELLO,
                      SSL_R_LENGTH_MISMATCH);

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index d64ddffffd03f81043a1b08c5d3ecfb641ba937d..b65dfa15872f0be350575a33c87d088fa1725316 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1656,7 +1656,7 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd)
     suppversions = &hello->pre_proc_exts[TLSEXT_IDX_supported_versions];
 
     /* If we did an HRR then supported versions is mandatory */
-    if (!suppversions->present && s->hello_retry_request)
+    if (!suppversions->present && s->hello_retry_request != SSL_HRR_NONE)
         return SSL_R_UNSUPPORTED_PROTOCOL;
 
     if (suppversions->present && !SSL_IS_DTLS(s)) {
@@ -1703,7 +1703,7 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd)
         }
 
         if (best_vers > 0) {
-            if (s->hello_retry_request) {
+            if (s->hello_retry_request != SSL_HRR_NONE) {
                 /*
                  * This is after a HelloRetryRequest so we better check that we
                  * negotiated TLSv1.3
@@ -1779,7 +1779,8 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions)
         return 0;
     }
 
-    if (s->hello_retry_request && s->version != TLS1_3_VERSION) {
+    if (s->hello_retry_request != SSL_HRR_NONE
+            && s->version != TLS1_3_VERSION) {
         s->version = origv;
         SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_F_SSL_CHOOSE_CLIENT_VERSION,
                  SSL_R_WRONG_SSL_VERSION);

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 3ebe76530abec8034c88c57ce88aa0c42b60a01c..7d1d15dcc1e442ccb21a7cf1a61c2c744183a052 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -48,7 +48,7 @@ static int ossl_statem_server13_read_transition(SSL *s, int mt)
         break;
 
     case TLS_ST_EARLY_DATA:
-        if (s->hello_retry_request) {
+        if (s->hello_retry_request == SSL_HRR_PENDING) {
             if (mt == SSL3_MT_CLIENT_HELLO) {
                 st->hand_state = TLS_ST_SR_CLNT_HELLO;
                 return 1;
@@ -395,16 +395,20 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_SRVR_HELLO:
-        if (s->hello_retry_request)
-            st->hand_state = TLS_ST_EARLY_DATA;
-        else if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
+        if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0
+                && s->hello_retry_request != SSL_HRR_COMPLETE)
             st->hand_state = TLS_ST_SW_CHANGE;
+        else if (s->hello_retry_request == SSL_HRR_PENDING)
+            st->hand_state = TLS_ST_EARLY_DATA;
         else
             st->hand_state = TLS_ST_SW_ENCRYPTED_EXTENSIONS;
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_CHANGE:
-        st->hand_state = TLS_ST_SW_ENCRYPTED_EXTENSIONS;
+        if (s->hello_retry_request == SSL_HRR_PENDING)
+            st->hand_state = TLS_ST_EARLY_DATA;
+        else
+            st->hand_state = TLS_ST_SW_ENCRYPTED_EXTENSIONS;
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_SW_ENCRYPTED_EXTENSIONS:
@@ -664,6 +668,8 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst)
         break;
 
     case TLS_ST_SW_CHANGE:
+        if (SSL_IS_TLS13(s))
+            break;
         s->session->cipher = s->s3->tmp.new_cipher;
         if (!s->method->ssl3_enc->setup_key_block(s)) {
             /* SSLfatal() already called */
@@ -733,7 +739,7 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
         break;
 
     case TLS_ST_SW_SRVR_HELLO:
-        if (SSL_IS_TLS13(s) && s->hello_retry_request) {
+        if (SSL_IS_TLS13(s) && s->hello_retry_request == SSL_HRR_PENDING) {
             if (statem_flush(s) != 1)
                 return WORK_MORE_A;
             break;
@@ -765,11 +771,14 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
         }
 #endif
         if (!SSL_IS_TLS13(s)
-                || (s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
+                || ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0
+                    && s->hello_retry_request != SSL_HRR_COMPLETE))
             break;
         /* Fall through */
 
     case TLS_ST_SW_CHANGE:
+        if (s->hello_retry_request == SSL_HRR_PENDING)
+            break;
         /*
          * TODO(TLS1.3): This actually causes a problem. We don't yet know
          * whether the next record we are going to receive is an unencrypted
@@ -1267,7 +1276,8 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
     if (clienthello->isv2) {
         unsigned int mt;
 
-        if (!SSL_IS_FIRST_HANDSHAKE(s) || s->hello_retry_request) {
+        if (!SSL_IS_FIRST_HANDSHAKE(s)
+                || s->hello_retry_request != SSL_HRR_NONE) {
             SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE,
                      SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_UNEXPECTED_MESSAGE);
             goto err;
@@ -1624,7 +1634,7 @@ static int tls_early_post_process_client_hello(SSL *s)
                      SSL_R_NO_SHARED_CIPHER);
             goto err;
         }
-        if (s->hello_retry_request
+        if (s->hello_retry_request == SSL_HRR_PENDING
                 && (s->s3->tmp.new_cipher == NULL
                     || s->s3->tmp.new_cipher->id != cipher->id)) {
             /*
@@ -2200,7 +2210,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
     size_t sl, len;
     int version;
     unsigned char *session_id;
-    int usetls13 = SSL_IS_TLS13(s) || s->hello_retry_request;
+    int usetls13 = SSL_IS_TLS13(s) || s->hello_retry_request == SSL_HRR_PENDING;
 
     version = usetls13 ? TLS1_2_VERSION : s->version;
     if (!WPACKET_put_bytes_u16(pkt, version)
@@ -2209,7 +2219,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
                 * tls_process_client_hello()
                 */
             || !WPACKET_memcpy(pkt,
-                               s->hello_retry_request
+                               s->hello_retry_request == SSL_HRR_PENDING
                                    ? hrrrandom : s->s3->server_random,
                                SSL3_RANDOM_SIZE)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_SERVER_HELLO,
@@ -2269,6 +2279,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
             || !WPACKET_put_bytes_u8(pkt, compm)
             || !tls_construct_extensions(s, pkt,
                                          s->hello_retry_request
+                                            == SSL_HRR_PENDING
                                             ? SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST
                                             : (SSL_IS_TLS13(s)
                                                 ? SSL_EXT_TLS1_3_SERVER_HELLO
@@ -2278,7 +2289,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
         return 0;
     }
 
-    if (s->hello_retry_request) {
+    if (s->hello_retry_request == SSL_HRR_PENDING) {
         /* Ditch the session. We'll create a new one next time around */
         SSL_SESSION_free(s->session);
         s->session = NULL;