From 4731bf884338f0aa4e147885091b1dc61c51756c Mon Sep 17 00:00:00 2001 From: FreezB11 Date: Sat, 15 Aug 2026 01:32:44 +0530 Subject: [PATCH] Prevent TLS1.2 handshake fragments from entering TLS1.3 post-handshake path (#5624) --- tests/unit/s2n_early_data_io_test.c | 5 +- tests/unit/s2n_post_handshake_recv_test.c | 22 ++-- tests/unit/s2n_post_handshake_send_test.c | 4 + tests/unit/s2n_post_handshake_test.c | 119 +++++++++++++++++++++- tests/unit/s2n_recv_test.c | 2 + tests/unit/s2n_send_key_update_test.c | 2 + tls/s2n_post_handshake.c | 3 + 7 files changed, 148 insertions(+), 9 deletions(-) diff --git a/tests/unit/s2n_early_data_io_test.c b/tests/unit/s2n_early_data_io_test.c index a388126f7e5..090f1291c4f 100644 --- a/tests/unit/s2n_early_data_io_test.c +++ b/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)); diff --git a/tests/unit/s2n_post_handshake_recv_test.c b/tests/unit/s2n_post_handshake_recv_test.c index b58bbed656e..0ca5bdcc0ae 100644 --- a/tests/unit/s2n_post_handshake_recv_test.c +++ b/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)); } diff --git a/tests/unit/s2n_post_handshake_send_test.c b/tests/unit/s2n_post_handshake_send_test.c index 967af96e064..5f6ddd2ac72 100644 --- a/tests/unit/s2n_post_handshake_send_test.c +++ b/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)); diff --git a/tests/unit/s2n_post_handshake_test.c b/tests/unit/s2n_post_handshake_test.c index eb696c8d69d..09f033ccbeb 100644 --- a/tests/unit/s2n_post_handshake_test.c +++ b/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)); diff --git a/tests/unit/s2n_recv_test.c b/tests/unit/s2n_recv_test.c index dd6650d15de..05d12aa49bf 100644 --- a/tests/unit/s2n_recv_test.c +++ b/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); diff --git a/tests/unit/s2n_send_key_update_test.c b/tests/unit/s2n_send_key_update_test.c index 76cd63d4997..cdbf0b7669d 100644 --- a/tests/unit/s2n_send_key_update_test.c +++ b/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 }; diff --git a/tls/s2n_post_handshake.c b/tls/s2n_post_handshake.c index 50ed44054dd..d2e67661bd2 100644 --- a/tls/s2n_post_handshake.c +++ b/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));