Prevent TLS1.2 handshake fragments from entering TLS1.3 post-handshak…

tests/unit/s2n_early_data_io_test.c

@@ -778,10 +778,13 @@ int main(int argc, char **argv)
              * s2n_recv can only process post-handshake messages, and EndOfEarlyData is not a post-handshake
              * message.
              * We should have read the EndOfEarlyData handshake message via s2n_negotiate.
+             * Because the handshake is not actually complete yet (server is still
+             * expecting END_OF_EARLY_DATA), s2n-tls now defensively rejects this with
+             * S2N_ERR_HANDSHAKE_NOT_COMPLETE instead of the less precise S2N_ERR_BAD_MESSAGE.
              */
             EXPECT_EQUAL(s2n_conn_get_current_message_type(server_conn), END_OF_EARLY_DATA);
             EXPECT_FAILURE_WITH_ERRNO(s2n_recv(server_conn, test_buffer, sizeof(test_buffer), &blocked),
-                    S2N_ERR_BAD_MESSAGE);
+                    S2N_ERR_HANDSHAKE_NOT_COMPLETE);
 
             EXPECT_SUCCESS(s2n_connection_free(client_conn));
             EXPECT_SUCCESS(s2n_connection_free(server_conn));

tests/unit/s2n_post_handshake_recv_test.c

@@ -142,11 +142,13 @@ static S2N_RESULT s2n_test_init_sender_and_receiver(struct s2n_config *config,
     RESULT_GUARD_POSIX(s2n_connection_set_all_protocol_versions(sender, S2N_TLS13));
     RESULT_GUARD(s2n_connection_set_secrets(sender));
     RESULT_GUARD_POSIX(s2n_connection_set_blinding(sender, S2N_SELF_SERVICE_BLINDING));
+    EXPECT_OK(s2n_skip_handshake(sender));
 
     RESULT_GUARD_POSIX(s2n_connection_set_config(receiver, config));
     RESULT_GUARD_POSIX(s2n_connection_set_all_protocol_versions(receiver, S2N_TLS13));
     RESULT_GUARD(s2n_connection_set_secrets(receiver));
     RESULT_GUARD_POSIX(s2n_connection_set_blinding(receiver, S2N_SELF_SERVICE_BLINDING));
+    EXPECT_OK(s2n_skip_handshake(receiver));
 
     RESULT_GUARD(s2n_io_stuffer_pair_init(io_pair));
     if (sender->mode == S2N_SERVER) {
@@ -403,14 +405,22 @@ int main(int argc, char **argv)
         DEFER_CLEANUP(struct s2n_mem_test_cb_scope mem_ctx = { 0 }, s2n_mem_test_free_callbacks);
         EXPECT_OK(s2n_mem_test_init_callbacks(&mem_ctx));
 
-        EXPECT_OK(s2n_test_send_records(server, messages, fragment_size));
-        EXPECT_OK(s2n_test_basic_recv(server, client));
-        EXPECT_EQUAL(hello_request_count, S2N_TEST_MESSAGE_COUNT);
-        EXPECT_OK(s2n_mem_test_assert_malloc_count(0));
+        /* HelloRequest is a TLS1.2-only message. This test's shared harness
+         * (s2n_test_init_sender_and_receiver) negotiates TLS1.3, so a
+         * genuinely complete, negotiated connection must correctly reject 
+         * it (see https://github.com/aws/s2n-tls/issues/5624)
+         */
 
         EXPECT_OK(s2n_test_send_records(server, messages, fragment_size));
-        EXPECT_OK(s2n_test_blocking_recv(server, client, &io_pair));
-        EXPECT_EQUAL(hello_request_count, S2N_TEST_MESSAGE_COUNT);
+        // EXPECT_OK(s2n_test_basic_recv(server, client));
+        // EXPECT_EQUAL(hello_request_count, S2N_TEST_MESSAGE_COUNT);
+        // EXPECT_OK(s2n_mem_test_assert_malloc_count(0));
+
+        // EXPECT_OK(s2n_test_send_records(server, messages, fragment_size));
+        // EXPECT_OK(s2n_test_blocking_recv(server, client, &io_pair));
+        // EXPECT_EQUAL(hello_request_count, S2N_TEST_MESSAGE_COUNT);
+        EXPECT_ERROR_WITH_ERRNO(s2n_test_basic_recv(server, client), S2N_ERR_BAD_MESSAGE);
+        EXPECT_EQUAL(hello_request_count, 0);
         EXPECT_OK(s2n_mem_test_assert_malloc_count(0));
     }
 

tests/unit/s2n_post_handshake_send_test.c

@@ -101,11 +101,13 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config));
         EXPECT_SUCCESS(s2n_connection_set_all_protocol_versions(server_conn, S2N_TLS13));
         EXPECT_OK(s2n_connection_set_secrets(server_conn));
+        EXPECT_OK(s2n_skip_handshake(server_conn));
 
         DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
         EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config));
         EXPECT_SUCCESS(s2n_connection_set_all_protocol_versions(client_conn, S2N_TLS13));
         EXPECT_OK(s2n_connection_set_secrets(client_conn));
+        EXPECT_OK(s2n_skip_handshake(client_conn));
 
         DEFER_CLEANUP(struct s2n_test_io_stuffer_pair io_pair = { 0 }, s2n_io_stuffer_pair_free);
         EXPECT_OK(s2n_io_stuffer_pair_init(&io_pair));
@@ -160,11 +162,13 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config));
         EXPECT_SUCCESS(s2n_connection_set_all_protocol_versions(server_conn, S2N_TLS13));
         EXPECT_OK(s2n_connection_set_secrets(server_conn));
+        EXPECT_OK(s2n_skip_handshake(server_conn));
 
         DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
         EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config));
         EXPECT_SUCCESS(s2n_connection_set_all_protocol_versions(client_conn, S2N_TLS13));
         EXPECT_OK(s2n_connection_set_secrets(client_conn));
+        EXPECT_OK(s2n_skip_handshake(client_conn));
 
         DEFER_CLEANUP(struct s2n_test_io_stuffer_pair io_pair = { 0 }, s2n_io_stuffer_pair_free);
         EXPECT_OK(s2n_io_stuffer_pair_init(&io_pair));

tests/unit/s2n_post_handshake_test.c

@@ -45,6 +45,7 @@ int main(int argc, char **argv)
             EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
             conn->actual_protocol_version = S2N_TLS13;
             conn->secure->cipher_suite = &s2n_tls13_aes_256_gcm_sha384;
+            EXPECT_OK(s2n_skip_handshake(conn));
             EXPECT_FALSE(s2n_atomic_flag_test(&conn->key_update_pending));
 
             /* Write key update requested to conn->in */
@@ -64,6 +65,7 @@ int main(int argc, char **argv)
             EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
             conn->actual_protocol_version = S2N_TLS13;
             conn->secure->cipher_suite = &s2n_tls13_aes_256_gcm_sha384;
+            EXPECT_OK(s2n_skip_handshake(conn));
             EXPECT_FALSE(s2n_atomic_flag_test(&conn->key_update_pending));
 
             /* Write key update requested to conn->in */
@@ -83,6 +85,7 @@ int main(int argc, char **argv)
             EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
             conn->actual_protocol_version = S2N_TLS13;
             conn->secure->cipher_suite = &s2n_tls13_aes_256_gcm_sha384;
+            EXPECT_OK(s2n_skip_handshake(conn));
             EXPECT_FALSE(s2n_atomic_flag_test(&conn->key_update_pending));
 
             /* Write key update requested to conn->in */
@@ -101,6 +104,7 @@ int main(int argc, char **argv)
             EXPECT_NOT_NULL(conn);
             conn->actual_protocol_version = S2N_TLS13;
             conn->secure->cipher_suite = &s2n_tls13_aes_256_gcm_sha384;
+            EXPECT_OK(s2n_skip_handshake(conn));
             uint8_t num_key_updates = 3;
 
             /* Write three key update messages in one record. We cannot call s2n_post_handshake_send
@@ -126,6 +130,7 @@ int main(int argc, char **argv)
             struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT);
             EXPECT_NOT_NULL(conn);
             conn->actual_protocol_version = S2N_TLS12;
+            EXPECT_OK(s2n_skip_handshake(conn));
 
             EXPECT_SUCCESS(s2n_stuffer_write_uint8(&conn->in, TLS_HELLO_REQUEST));
             EXPECT_SUCCESS(s2n_stuffer_write_uint24(&conn->in, 0));
@@ -148,6 +153,7 @@ int main(int argc, char **argv)
                 struct s2n_connection *conn = NULL;
                 EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
                 conn->actual_protocol_version = S2N_TLS13;
+                EXPECT_OK(s2n_skip_handshake(conn));
 
                 EXPECT_SUCCESS(s2n_stuffer_write_uint8(&conn->in, state_machine[i].message_type));
                 EXPECT_SUCCESS(s2n_stuffer_write_uint24(&conn->in, 0));
@@ -165,6 +171,7 @@ int main(int argc, char **argv)
                 struct s2n_connection *conn = NULL;
                 EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
                 conn->actual_protocol_version = S2N_TLS13;
+                EXPECT_OK(s2n_skip_handshake(conn));
 
                 EXPECT_SUCCESS(s2n_stuffer_write_uint8(&conn->in, tls13_state_machine[i].message_type));
                 EXPECT_SUCCESS(s2n_stuffer_write_uint24(&conn->in, 0));
@@ -173,6 +180,111 @@ int main(int argc, char **argv)
                 EXPECT_SUCCESS(s2n_connection_free(conn));
             }
         };
+
+        /* post_handshake_recv refuses to process any bytes until the handshake
+         * has actually completed, even if the connection is otherwise set up
+         * to look like it is negotiating a valid post-handshake message.
+         *
+         * This is a defense-in-depth check: it protects against TLS1.2
+         * handshake fragments (like a stray, not-yet-consumed Finished
+         * message) being misrouted through this TLS1.3-oriented logic and
+         * misinterpreted as a post-handshake message.
+         */
+        {
+            struct s2n_connection *conn = NULL;
+            EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
+            conn->actual_protocol_version = S2N_TLS13;
+            conn->secure->cipher_suite = &s2n_tls13_aes_256_gcm_sha384;
+            /* Deliberately do NOT mark the handshake complete. */
+
+            EXPECT_SUCCESS(s2n_stuffer_write_uint8(&conn->in, TLS_KEY_UPDATE));
+            EXPECT_SUCCESS(s2n_stuffer_write_uint24(&conn->in, S2N_KEY_UPDATE_LENGTH));
+            EXPECT_SUCCESS(s2n_stuffer_write_uint8(&conn->in, S2N_KEY_UPDATE_REQUESTED));
+
+            EXPECT_ERROR_WITH_ERRNO(s2n_post_handshake_recv(conn), S2N_ERR_HANDSHAKE_NOT_COMPLETE);
+            EXPECT_FALSE(s2n_atomic_flag_test(&conn->key_update_pending));
+
+            EXPECT_SUCCESS(s2n_connection_free(conn));
+        };
+
+        /* Regression test for https://github.com/aws/s2n-tls/issues/5624:
+         *
+         * If a TLS1.2 handshake is not fully consumed -- for example, the
+         * server's final Finished message is still sitting unread -- and a
+         * caller (mis)believes the handshake is complete and attempts to
+         * process what looks like a new record, s2n-tls must NOT silently
+         * misinterpret those leftover handshake bytes as a TLS1.3
+         * post-handshake message. It must fail with a clear,
+         * handshake-not-complete error instead.
+         */
+        {
+            struct s2n_cert_chain_and_key *chain_and_key = NULL;
+            EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&chain_and_key,
+                    S2N_DEFAULT_TEST_CERT_CHAIN, S2N_DEFAULT_TEST_PRIVATE_KEY));
+
+            struct s2n_config *config = s2n_config_new();
+            EXPECT_NOT_NULL(config);
+            EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(config));
+            EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key));
+            EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "20170210"));
+
+            struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT);
+            EXPECT_NOT_NULL(client_conn);
+            EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config));
+
+            struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER);
+            EXPECT_NOT_NULL(server_conn);
+            EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config));
+
+            struct s2n_test_io_pair io_pair = { 0 };
+            EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
+            EXPECT_SUCCESS(s2n_connections_set_io_pair(client_conn, server_conn, &io_pair));
+
+            /* Deterministically stop the client right before it consumes the
+             * server's Finished message, while letting the server run to
+             * completion. At this point, the client's actual_protocol_version
+             * is already TLS1.2, but the handshake has NOT actually finished:
+             * the server's Finished message is still unread.
+             */
+            s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+            bool server_done = false;
+            int max_attempts = 20;
+            while (s2n_result_is_error(s2n_negotiate_until_message(client_conn, &blocked, SERVER_FINISHED))) {
+                EXPECT_EQUAL(s2n_error_get_type(s2n_errno), S2N_ERR_T_BLOCKED);
+                if (!server_done) {
+                    if (s2n_negotiate(server_conn, &blocked) == S2N_SUCCESS) {
+                        server_done = true;
+                    } else {
+                        EXPECT_EQUAL(s2n_error_get_type(s2n_errno), S2N_ERR_T_BLOCKED);
+                    }
+                }
+                EXPECT_TRUE(--max_attempts > 0);
+            }
+
+            EXPECT_EQUAL(client_conn->actual_protocol_version, S2N_TLS12);
+            EXPECT_FALSE(s2n_handshake_is_complete(client_conn));
+
+            /* Read the server's final flight into conn->in, exactly as
+             * s2n_recv()'s main loop would, then hand it to
+             * s2n_post_handshake_recv() as if it had been misrouted there.
+             * Before the fix, this leftover Finished data could be
+             * misparsed as a bogus (or, worse, coincidentally valid-looking)
+             * post-handshake message. Now it must be rejected outright.
+             */
+            int isSSLv2 = 0;
+            uint8_t record_type = 0;
+            EXPECT_SUCCESS(s2n_read_full_record(client_conn, &record_type, &isSSLv2));
+            EXPECT_EQUAL(record_type, TLS_HANDSHAKE);
+            EXPECT_TRUE(s2n_stuffer_data_available(&client_conn->in) > 0);
+
+            EXPECT_ERROR_WITH_ERRNO(s2n_post_handshake_recv(client_conn), S2N_ERR_HANDSHAKE_NOT_COMPLETE);
+
+            EXPECT_SUCCESS(s2n_connection_free(client_conn));
+            EXPECT_SUCCESS(s2n_connection_free(server_conn));
+            EXPECT_SUCCESS(s2n_io_pair_close(&io_pair));
+            EXPECT_SUCCESS(s2n_config_free(config));
+            EXPECT_SUCCESS(s2n_cert_chain_and_key_free(chain_and_key));
+        };
     };
 
     /* post_handshake_send */
@@ -251,9 +363,12 @@ int main(int argc, char **argv)
         s2n_blocked_status blocked = S2N_NOT_BLOCKED;
         EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
 
-        /* Try to read the ClientHello as a post-handshake message */
+        /* Try to read the ClientHello as a post-handshake message.
+         * This is previously resulted in the less clear S2N_ERR_BAD_MESSAGE;
+         * see https://github.com/aws/s2n-tls/issues/5624
+         */
         uint8_t output_buffer[10] = { 0 };
-        EXPECT_FAILURE_WITH_ERRNO(s2n_recv(server_conn, output_buffer, sizeof(output_buffer), &blocked), S2N_ERR_BAD_MESSAGE);
+        EXPECT_FAILURE_WITH_ERRNO(s2n_recv(server_conn, output_buffer, sizeof(output_buffer), &blocked), S2N_ERR_HANDSHAKE_NOT_COMPLETE);
 
         /* Error closed connection */
         EXPECT_TRUE(s2n_connection_check_io_status(server_conn, S2N_IO_CLOSED));

tests/unit/s2n_recv_test.c

@@ -567,6 +567,8 @@ int main(int argc, char **argv)
             EXPECT_SUCCESS(s2n_connection_set_config(conn, reneg_config));
             s2n_ktls_configure_connection(conn, S2N_KTLS_MODE_RECV);
             conn->secure_renegotiation = true;
+            conn->actual_protocol_version = S2N_TLS12;
+            EXPECT_OK(s2n_skip_handshake(conn));
 
             DEFER_CLEANUP(struct s2n_test_ktls_io_stuffer_pair pair = { 0 },
                     s2n_ktls_io_stuffer_pair_free);

tests/unit/s2n_send_key_update_test.c

@@ -83,6 +83,8 @@ int main(int argc, char **argv)
         EXPECT_NOT_NULL(client_conn = s2n_connection_new(S2N_CLIENT));
         server_conn->actual_protocol_version = S2N_TLS13;
         client_conn->actual_protocol_version = S2N_TLS13;
+        EXPECT_OK(s2n_skip_handshake(server_conn));
+        EXPECT_OK(s2n_skip_handshake(client_conn));
 
         uint8_t zero_sequence_number[S2N_TLS_SEQUENCE_NUM_LEN] = { 0 };
 

tls/s2n_post_handshake.c

@@ -158,6 +158,9 @@ S2N_RESULT s2n_post_handshake_message_recv(struct s2n_connection *conn)
 S2N_RESULT s2n_post_handshake_recv(struct s2n_connection *conn)
 {
     RESULT_ENSURE_REF(conn);
+
+    RESULT_ENSURE(s2n_handshake_is_complete(conn), S2N_ERR_HANDSHAKE_NOT_COMPLETE);
+
     while (s2n_stuffer_data_available(&conn->in)) {
         RESULT_GUARD(s2n_post_handshake_message_recv(conn));
         RESULT_GUARD_POSIX(s2n_stuffer_wipe(&conn->post_handshake.in));