From 3861588b47d045afcc66d9ba8b73b8217e87d2ce Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 22 May 2026 18:10:49 +0200 Subject: [PATCH] tls13: Add record boundary checks RFC 8446 (TLS 1.3 specification), Section 5.1, states: Handshake messages MAY be coalesced into a single TLSPlaintext record or fragmented across several records, provided that: ... -Handshake messages MUST NOT span key changes. Implementations MUST verify that all messages immediately preceding a key change align with a record boundary; if not, then they MUST terminate the connection with an "unexpected_message" alert. Because the ClientHello, EndOfEarlyData, ServerHello, Finished, and KeyUpdate messages can immediately precede a key change, implementations MUST send these messages in alignment with a record boundary. Signed-off-by: Ronald Cron --- library/ssl_misc.h | 1 + library/ssl_tls13_client.c | 8 ++-- library/ssl_tls13_generic.c | 10 +++-- library/ssl_tls13_server.c | 4 +- tests/suites/test_suite_ssl.function | 65 ++++++++++------------------ 5 files changed, 36 insertions(+), 52 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index f0ca823f33..d85dbc6e1d 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2028,6 +2028,7 @@ static inline int mbedtls_ssl_tls13_key_exchange_mode_with_ephemeral( MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_tls13_fetch_handshake_msg(mbedtls_ssl_context *ssl, unsigned hs_type, + unsigned record_boundary, unsigned char **buf, size_t *buf_len); diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index ecdf507eab..e4138fa2c7 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2002,7 +2002,7 @@ static int ssl_tls13_process_server_hello(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> %s", __func__)); MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_SERVER_HELLO, &buf, &buf_len)); + ssl, MBEDTLS_SSL_HS_SERVER_HELLO, 1, &buf, &buf_len)); ret = ssl_tls13_preprocess_server_hello(ssl, buf, buf + buf_len); if (ret < 0) { @@ -2195,7 +2195,7 @@ static int ssl_tls13_process_encrypted_extensions(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> parse encrypted extensions")); MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, + ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, 0, &buf, &buf_len)); /* Process the message contents */ @@ -2530,7 +2530,7 @@ static int ssl_tls13_process_certificate_request(mbedtls_ssl_context *ssl) size_t buf_len; MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_CERTIFICATE_REQUEST, + ssl, MBEDTLS_SSL_HS_CERTIFICATE_REQUEST, 0, &buf, &buf_len)); MBEDTLS_SSL_PROC_CHK(ssl_tls13_parse_certificate_request( @@ -3025,7 +3025,7 @@ static int ssl_tls13_process_new_session_ticket(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> parse new session ticket")); MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_NEW_SESSION_TICKET, + ssl, MBEDTLS_SSL_HS_NEW_SESSION_TICKET, 0, &buf, &buf_len)); /* diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 078daea352..90891567ec 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -47,6 +47,7 @@ const uint8_t mbedtls_ssl_tls13_hello_retry_request_magic[ int mbedtls_ssl_tls13_fetch_handshake_msg(mbedtls_ssl_context *ssl, unsigned hs_type, + unsigned record_boundary, unsigned char **buf, size_t *buf_len) { @@ -58,7 +59,8 @@ int mbedtls_ssl_tls13_fetch_handshake_msg(mbedtls_ssl_context *ssl, } if (ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || - ssl->in_msg[0] != hs_type) { + ssl->in_msg[0] != hs_type || + (record_boundary && (ssl->in_hslen != ssl->in_msglen))) { MBEDTLS_SSL_DEBUG_MSG(1, ("Receive unexpected handshake message.")); MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE); @@ -339,7 +341,7 @@ int mbedtls_ssl_tls13_process_certificate_verify(mbedtls_ssl_context *ssl) MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len)); + ssl, MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, 0, &buf, &buf_len)); /* Need to calculate the hash of the transcript first * before reading the message since otherwise it gets @@ -691,7 +693,7 @@ int mbedtls_ssl_tls13_process_certificate(mbedtls_ssl_context *ssl) size_t buf_len; MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_CERTIFICATE, + ssl, MBEDTLS_SSL_HS_CERTIFICATE, 0, &buf, &buf_len)); /* Parse the certificate chain sent by the peer. */ @@ -1110,7 +1112,7 @@ int mbedtls_ssl_tls13_process_finished_message(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> parse finished message")); MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_FINISHED, &buf, &buf_len)); + ssl, MBEDTLS_SSL_HS_FINISHED, 1, &buf, &buf_len)); /* Preprocessing step: Compute handshake digest */ MBEDTLS_SSL_PROC_CHK(ssl_tls13_preprocess_finished_message(ssl)); diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 38f2fe9370..0db142c50a 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1936,7 +1936,7 @@ static int ssl_tls13_process_client_hello(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> parse client hello")); MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_CLIENT_HELLO, + ssl, MBEDTLS_SSL_HS_CLIENT_HELLO, 1, &buf, &buflen)); MBEDTLS_SSL_PROC_CHK_NEG(ssl_tls13_parse_client_hello(ssl, buf, @@ -3027,7 +3027,7 @@ static int ssl_tls13_process_end_of_early_data(mbedtls_ssl_context *ssl) size_t buf_len; MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_END_OF_EARLY_DATA, + ssl, MBEDTLS_SSL_HS_END_OF_EARLY_DATA, 1, &buf, &buf_len)); MBEDTLS_SSL_PROC_CHK(ssl_tls13_parse_end_of_early_data( diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 3b80db0a79..d058401148 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4124,7 +4124,7 @@ void tls13_server_certificate_msg_invalid_vector_len() ret = mbedtls_ssl_tls13_fetch_handshake_msg(&(client_ep.ssl), MBEDTLS_SSL_HS_CERTIFICATE, - &buf, &buf_len); + 0, &buf, &buf_len); TEST_EQUAL(ret, 0); end = buf + buf_len; @@ -6865,14 +6865,21 @@ void tls13_drop_early_data(int insert_end_of_early_data) TEST_EQUAL(ret, (int) sizeof(end_of_early_data)); } - /* - * The server read the record containing the ClientHello and possibly the - * EndOfEarlyData message and parse the ClientHello. - */ - TEST_EQUAL(mbedtls_test_move_handshake_to_state( - &(server_ep.ssl), &(client_ep.ssl), - MBEDTLS_SSL_SERVER_HELLO), 0); + /* The server read the record containing the ClientHello. */ + ret = mbedtls_test_move_handshake_to_state(&(server_ep.ssl), + &(client_ep.ssl), + MBEDTLS_SSL_SERVER_HELLO); + if (insert_end_of_early_data) { + /* + * If an EndOfEarlyData message was appended to the record containing + * the ClientHello then the end of the ClientHello does not align with + * the record boundary and the handshake fails. + */ + TEST_EQUAL(ret, MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE); + goto exit; + } + TEST_EQUAL(ret, 0); /* The client application writes early data */ ret = mbedtls_ssl_write_early_data(&(client_ep.ssl), (unsigned char *) early_data, @@ -6884,43 +6891,17 @@ void tls13_drop_early_data(int insert_end_of_early_data) mbedtls_test_ssl_buffer_get(client_ep.socket.output, buf, sizeof(buf)); TEST_EQUAL(server_ep.socket.input->content_length, 0); - /* - * Resume the handshake, up to the point where the client is about to write - * the EndOfEarlyData message. - */ - TEST_EQUAL(mbedtls_test_move_handshake_to_state( - &(client_ep.ssl), &(server_ep.ssl), - MBEDTLS_SSL_END_OF_EARLY_DATA), 0); - TEST_EQUAL(client_ep.ssl.state, MBEDTLS_SSL_END_OF_EARLY_DATA); - - if (insert_end_of_early_data) { - /* - * Let the client write the EndOfEarlyData message and then read it - * from the output socket buffer, preventing it to reach the server as - * we have already inserted one. - */ - ret = mbedtls_ssl_handshake_step(&client_ep.ssl); - TEST_EQUAL(ret, 0); - ret = mbedtls_ssl_flush_output(&client_ep.ssl); - TEST_EQUAL(ret, 0); - TEST_ASSERT(client_ep.socket.output->content_length <= sizeof(buf)); - mbedtls_test_ssl_buffer_get(client_ep.socket.output, buf, sizeof(buf)); - } - + /* Resume the handshake */ ret = mbedtls_test_move_handshake_to_state(&(server_ep.ssl), &(client_ep.ssl), MBEDTLS_SSL_HANDSHAKE_OVER); - if (insert_end_of_early_data) { - TEST_EQUAL(ret, 0); - } else { - /* - * If we have just dropped the early data then the EndOfEarlyData - * message is encrypted with the record number 1 while the server - * expects it to be encrypted with record number 0. Thus the - * handshake fails with MBEDTLS_ERR_SSL_INVALID_MAC. - */ - TEST_EQUAL(ret, MBEDTLS_ERR_SSL_INVALID_MAC); - } + /* + * The early data were dropped. The EndOfEarlyData message is encrypted + * with the record number 1, but the server expects it to be encrypted + * with record number 0. Thus the handshake fails with + * MBEDTLS_ERR_SSL_INVALID_MAC. + */ + TEST_EQUAL(ret, MBEDTLS_ERR_SSL_INVALID_MAC); exit: mbedtls_test_ssl_endpoint_free(&client_ep);