Fix re-entrancy of non-MPS handshake state machine

Signed-off-by: Hanno Becker <[email protected]>
hannestschofenig · Aug 7, 2021 · 225b9ef · 225b9ef
1 parent 9957f8c
commit 225b9ef
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 70 deletions.
diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
@@ -590,6 +590,7 @@ typedef enum
     MBEDTLS_SSL_HANDSHAKE_WRAPUP,
     MBEDTLS_SSL_HANDSHAKE_OVER,
     MBEDTLS_SSL_SERVER_NEW_SESSION_TICKET,
+    MBEDTLS_SSL_SERVER_NEW_SESSION_TICKET_FLUSH,
     MBEDTLS_SSL_SERVER_HELLO_VERIFY_REQUEST_SENT,
 #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)
     MBEDTLS_SSL_HELLO_RETRY_REQUEST,

diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c
@@ -159,6 +159,9 @@ int ssl_write_early_data_process( mbedtls_ssl_context* ssl )
 
         MBEDTLS_SSL_PROC_CHK( mbedtls_mps_dispatch( &ssl->mps.l4 ) );
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_write_early_data_postprocess( ssl ) );
+
 #else  /* MBEDTLS_SSL_USE_MPS */
 
         /* Make sure we can write a new message. */
@@ -171,6 +174,9 @@ int ssl_write_early_data_process( mbedtls_ssl_context* ssl )
 
         ssl->out_msgtype = MBEDTLS_SSL_MSG_APPLICATION_DATA;
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_write_early_data_postprocess( ssl ) );
+
         /* Dispatch message */
         MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_record( ssl, SSL_FORCE_FLUSH ) );
 
@@ -186,10 +192,11 @@ int ssl_write_early_data_process( mbedtls_ssl_context* ssl )
 
 #endif /* MBEDTLS_ZERO_RTT */
     }
-
-
-    /* Update state */
-    MBEDTLS_SSL_PROC_CHK( ssl_write_early_data_postprocess( ssl ) );
+    else
+    {
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_write_early_data_postprocess( ssl ) );
+    }
 
 cleanup:
 
@@ -1367,6 +1374,7 @@ static int ssl_client_hello_write_partial( mbedtls_ssl_context* ssl,
                                            unsigned char* buf, size_t buflen,
                                            size_t* len_without_binders,
                                            size_t* len_with_binders );
+static int ssl_client_hello_postprocess( mbedtls_ssl_context* ssl );
 
 static int ssl_client_hello_process( mbedtls_ssl_context* ssl )
 {
@@ -1435,6 +1443,8 @@ static int ssl_client_hello_process( mbedtls_ssl_context* ssl )
 
     MBEDTLS_SSL_PROC_CHK( mbedtls_mps_dispatch( &ssl->mps.l4 ) );
 
+    MBEDTLS_SSL_PROC_CHK( ssl_client_hello_postprocess( ssl ) );
+
 #else /* MBEDTLS_SSL_USE_MPS */
 
     /* Make sure we can write a new message. */
@@ -1495,32 +1505,42 @@ static int ssl_client_hello_process( mbedtls_ssl_context* ssl )
 
     MBEDTLS_SSL_DEBUG_BUF( 3, "ClientHello", ssl->out_msg, ssl->out_msglen );
 
+    MBEDTLS_SSL_PROC_CHK( ssl_client_hello_postprocess( ssl ) );
 
     /* Dispatch message */
     MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg_ext(
                               ssl, 0 /* no checksum update */ ) );
 
 #endif /* MBEDTLS_SSL_USE_MPS */
 
-    /* NOTE: With the new messaging layer, the postprocessing
-     *       step might come after the dispatching step if the
-     *       latter doesn't send the message immediately.
-     *       At the moment, we must do the postprocessing
-     *       prior to the dispatching because if the latter
-     *       returns WANT_WRITE, we want the handshake state
-     *       to be updated in order to not enter
-     *       this function again on retry.
-     *
-     *       Further, once the two calls can be re-ordered, the two
-     *       calls can be consolidated.
-     */
-
 cleanup:
 
     MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= write client hello" ) );
     return( ret );
 }
 
+static int ssl_client_hello_postprocess( mbedtls_ssl_context* ssl )
+{
+    if( ssl->state == MBEDTLS_SSL_CLIENT_HELLO )
+    {
+#if defined(MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE)
+        mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CCS_AFTER_CLIENT_HELLO );
+#else
+        mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_EARLY_APP_DATA );
+#endif /* MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE */
+    }
+    else if( ssl->state == MBEDTLS_SSL_SECOND_CLIENT_HELLO )
+    {
+        mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_SECOND_SERVER_HELLO );
+    }
+    else
+    {
+        return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
+    }
+
+    return( 0 );
+}
+
 static int ssl_client_hello_prepare( mbedtls_ssl_context* ssl )
 {
     int ret;
@@ -4200,12 +4220,6 @@ int mbedtls_ssl_handshake_client_step_tls1_3( mbedtls_ssl_context *ssl )
                 break;
             }
 
-#if defined(MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE)
-            mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CCS_AFTER_CLIENT_HELLO );
-#else
-            mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_EARLY_APP_DATA );
-#endif /* MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE */
-
 #if defined(MBEDTLS_SSL_USE_MPS)
             ret = mbedtls_mps_flush( &ssl->mps.l4 );
             if( ret != 0 )
@@ -4305,7 +4319,6 @@ int mbedtls_ssl_handshake_client_step_tls1_3( mbedtls_ssl_context *ssl )
                 MBEDTLS_SSL_DEBUG_RET( 1, "ssl_write_client_hello", ret );
                 break;
             }
-            mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_SECOND_SERVER_HELLO );
 
 #if defined(MBEDTLS_SSL_USE_MPS)
             ret = mbedtls_mps_flush( &ssl->mps.l4 );

diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c
@@ -178,6 +178,7 @@ int mbedtls_ssl_write_change_cipher_spec_process( mbedtls_ssl_context* ssl )
         MBEDTLS_SSL_PROC_CHK( mbedtls_mps_flush( &ssl->mps.l4 ) );
         MBEDTLS_SSL_PROC_CHK( mbedtls_mps_write_ccs( &ssl->mps.l4 ) );
         MBEDTLS_SSL_PROC_CHK( mbedtls_mps_dispatch( &ssl->mps.l4 ) );
+        MBEDTLS_SSL_PROC_CHK( ssl_write_change_cipher_spec_postprocess( ssl ) );
 
 #else /* MBEDTLS_SSL_USE_MPS */
         /* Make sure we can write a new message. */
@@ -190,13 +191,13 @@ int mbedtls_ssl_write_change_cipher_spec_process( mbedtls_ssl_context* ssl )
 
         ssl->out_msgtype = MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC;
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_write_change_cipher_spec_postprocess( ssl ) );
+
         /* Dispatch message */
         MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_record( ssl, SSL_FORCE_FLUSH ) );
 
 #endif /* MBEDTLS_SSL_USE_MPS */
-
-        /* Update state */
-        MBEDTLS_SSL_PROC_CHK( ssl_write_change_cipher_spec_postprocess( ssl ) );
     }
     else
     {
@@ -592,6 +593,9 @@ int mbedtls_ssl_write_certificate_verify_process( mbedtls_ssl_context* ssl )
 
         MBEDTLS_SSL_PROC_CHK( mbedtls_mps_dispatch( &ssl->mps.l4 ) );
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_certificate_verify_postprocess( ssl ) );
+
 #else  /* MBEDTLS_SSL_USE_MPS */
 
         /* Make sure we can write a new message. */
@@ -605,27 +609,17 @@ int mbedtls_ssl_write_certificate_verify_process( mbedtls_ssl_context* ssl )
         ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE;
         ssl->out_msg[0] = MBEDTLS_SSL_HS_CERTIFICATE_VERIFY;
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_certificate_verify_postprocess( ssl ) );
+
         /* Dispatch message */
         MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg( ssl ) );
-
-        /* NOTE: With the new messaging layer, the postprocessing
-         *       step might come after the dispatching step if the
-         *       latter doesn't send the message immediately.
-         *       At the moment, we must do the postprocessing
-         *       prior to the dispatching because if the latter
-         *       returns WANT_WRITE, we want the handshake state
-         *       to be updated in order to not enter
-         *       this function again on retry.
-         *
-         *       Further, once the two calls can be re-ordered, the two
-         *       calls to ssl_certificate_verify_postprocess( ) can be
-         *       consolidated. */
-
 #endif /* MBEDTLS_SSL_USE_MPS */
     }
-
-    /* Update state */
-    MBEDTLS_SSL_PROC_CHK( ssl_certificate_verify_postprocess( ssl ) );
+    else
+    {
+        MBEDTLS_SSL_PROC_CHK( ssl_certificate_verify_postprocess( ssl ) );
+    }
 
 cleanup:
 
@@ -1305,6 +1299,9 @@ int mbedtls_ssl_write_certificate_process( mbedtls_ssl_context* ssl )
 
         MBEDTLS_SSL_PROC_CHK( mbedtls_mps_dispatch( &ssl->mps.l4 ) );
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_write_certificate_postprocess( ssl ) );
+
 #else  /* MBEDTLS_SSL_USE_MPS */
 
         /* Make sure we can write a new message. */
@@ -1318,6 +1315,9 @@ int mbedtls_ssl_write_certificate_process( mbedtls_ssl_context* ssl )
         ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE;
         ssl->out_msg[0] = MBEDTLS_SSL_HS_CERTIFICATE;
 
+        /* Update state */
+        MBEDTLS_SSL_PROC_CHK( ssl_write_certificate_postprocess( ssl ) );
+
         /* Dispatch message */
         MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg( ssl ) );
 
@@ -1328,11 +1328,9 @@ int mbedtls_ssl_write_certificate_process( mbedtls_ssl_context* ssl )
 #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */
     {
         MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate" ) );
+        MBEDTLS_SSL_PROC_CHK( ssl_write_certificate_postprocess( ssl ) );
     }
 
-    /* Update state */
-    MBEDTLS_SSL_PROC_CHK( ssl_write_certificate_postprocess( ssl ) );
-
 cleanup:
 
     MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= write certificate" ) );
@@ -2305,10 +2303,11 @@ int mbedtls_ssl_finished_out_process( mbedtls_ssl_context* ssl )
     ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE;
     ssl->out_msg[0] = MBEDTLS_SSL_HS_FINISHED;
 
-    /* TODO: This doesn't work if mbedtls_ssl_write_handshake_msg() fails
-     * because the underlying transport isn't ready. */
-    MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg( ssl ) );
+    mbedtls_ssl_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_FINISHED,
+                                        ssl->out_msg + 4, ssl->out_msglen - 4 );
+
     MBEDTLS_SSL_PROC_CHK( ssl_finished_out_postprocess( ssl ) );
+    MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg_ext( ssl, 0 ) );
 
 
 #endif /* MBEDTLS_SSL_USE_MPS */
@@ -2526,6 +2525,7 @@ int mbedtls_ssl_finished_in_process( mbedtls_ssl_context* ssl )
 #else /* MBEDTLS_SSL_USE_MPS */
 
     MBEDTLS_SSL_PROC_CHK( ssl_read_finished_fetch( ssl, &buf, &buflen ) );
+
     MBEDTLS_SSL_PROC_CHK( ssl_finished_in_parse( ssl, buf, buflen ) );
 
 #endif /* MBEDTLS_SSL_USE_MPS */

diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c
@@ -1445,11 +1445,11 @@ static int ssl_write_new_session_ticket_process( mbedtls_ssl_context *ssl )
                                         MBEDTLS_SSL_OUT_CONTENT_LEN,
                                         &ssl->out_msglen ) );
 
-        MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg( ssl ) );
-
         MBEDTLS_SSL_PROC_CHK(
             ssl_write_new_session_ticket_postprocess( ssl ) );
 
+        MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_handshake_msg( ssl ) );
+
 #endif /* MBEDTLS_SSL_USE_MPS */
     }
     else
@@ -1477,7 +1477,7 @@ static int ssl_write_new_session_ticket_coordinate( mbedtls_ssl_context* ssl )
 
 static int ssl_write_new_session_ticket_postprocess( mbedtls_ssl_context* ssl )
 {
-    mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_HANDSHAKE_OVER );
+    mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_SERVER_NEW_SESSION_TICKET_FLUSH );
     return( 0 );
 }
 
@@ -4043,10 +4043,15 @@ static int ssl_server_hello_write( mbedtls_ssl_context* ssl,
 
 static int ssl_server_hello_postprocess( mbedtls_ssl_context* ssl )
 {
-    int ret = 0;
-    ( (void ) ssl );
-
-    return( ret );
+#if defined(MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE)
+    if( ssl->handshake->ccs_sent > 1 )
+        mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_SERVER_CCS_AFTER_SERVER_HELLO );
+    else
+#endif /* MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE */
+    {
+        mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_ENCRYPTED_EXTENSIONS );
+    }
+    return( 0 );
 }
 
 /*
@@ -4389,22 +4394,9 @@ int mbedtls_ssl_handshake_server_step_tls1_3( mbedtls_ssl_context *ssl )
 
         case MBEDTLS_SSL_SERVER_HELLO:
             ret = ssl_server_hello_process( ssl );
-
             if( ret != 0 )
                 break;
 
-#if defined(MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE)
-            if( ssl->handshake->ccs_sent > 1 )
-            {
-                mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_SERVER_CCS_AFTER_SERVER_HELLO );
-            }
-            else
-            {
-                mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_ENCRYPTED_EXTENSIONS );
-            }
-#else
-            mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_ENCRYPTED_EXTENSIONS );
-#endif /* MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE */
 
             break;
 
@@ -4533,6 +4525,13 @@ int mbedtls_ssl_handshake_server_step_tls1_3( mbedtls_ssl_context *ssl )
 
             break;
 
+        case MBEDTLS_SSL_SERVER_NEW_SESSION_TICKET_FLUSH:
+            ret = mbedtls_ssl_flush_output( ssl );
+            if( ret != 0 )
+                return( ret );
+            mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_HANDSHAKE_OVER );
+            break;
+
         default:
             MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid state %d", ssl->state ) );
             return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );