From 3fc5a4dc86cfd43278d1891cb3d178e14c6254d2 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Sun, 10 Mar 2024 02:11:03 +0000 Subject: [PATCH 01/17] Defragment incoming TLS handshake messages Signed-off-by: Deomid rojer Ryabkov --- ChangeLog.d/tls-hs-defrag-in.txt | 2 + include/mbedtls/ssl.h | 2 + library/ssl_misc.h | 8 ++- library/ssl_msg.c | 99 ++++++++++++++++++++++++++++---- library/ssl_tls.c | 17 +++++- 5 files changed, 113 insertions(+), 15 deletions(-) create mode 100644 ChangeLog.d/tls-hs-defrag-in.txt diff --git a/ChangeLog.d/tls-hs-defrag-in.txt b/ChangeLog.d/tls-hs-defrag-in.txt new file mode 100644 index 0000000000..8c57200119 --- /dev/null +++ b/ChangeLog.d/tls-hs-defrag-in.txt @@ -0,0 +1,2 @@ +Change + * Defragment incoming TLS handshake messages. diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 42fffbf860..26ea791208 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1817,6 +1817,8 @@ struct mbedtls_ssl_context { size_t MBEDTLS_PRIVATE(in_hslen); /*!< current handshake message length, including the handshake header */ + unsigned char *MBEDTLS_PRIVATE(in_hshdr); /*!< original handshake header start */ + size_t MBEDTLS_PRIVATE(in_hsfraglen); /*!< accumulated hs fragments length */ int MBEDTLS_PRIVATE(nb_zero); /*!< # of 0-length encrypted messages */ int MBEDTLS_PRIVATE(keep_current_message); /*!< drop or reuse current message diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 7495ae3bec..89947e9575 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1830,7 +1830,13 @@ void mbedtls_ssl_set_timer(mbedtls_ssl_context *ssl, uint32_t millisecs); MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_check_timer(mbedtls_ssl_context *ssl); -void mbedtls_ssl_reset_in_out_pointers(mbedtls_ssl_context *ssl); +void mbedtls_ssl_reset_in_pointers(mbedtls_ssl_context *ssl); +void mbedtls_ssl_reset_out_pointers(mbedtls_ssl_context *ssl); +static inline void mbedtls_ssl_reset_in_out_pointers(mbedtls_ssl_context *ssl) +{ + mbedtls_ssl_reset_in_pointers(ssl); + mbedtls_ssl_reset_out_pointers(ssl); +} void mbedtls_ssl_update_out_pointers(mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform); void mbedtls_ssl_update_in_pointers(mbedtls_ssl_context *ssl); diff --git a/library/ssl_msg.c b/library/ssl_msg.c index dcda1d3f21..99b60f382e 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3226,7 +3226,11 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) return MBEDTLS_ERR_SSL_INVALID_RECORD; } - ssl->in_hslen = mbedtls_ssl_hs_hdr_len(ssl) + ssl_get_hs_total_len(ssl); + if (ssl->in_hslen == 0) { + ssl->in_hslen = mbedtls_ssl_hs_hdr_len(ssl) + ssl_get_hs_total_len(ssl); + ssl->in_hsfraglen = 0; + ssl->in_hshdr = ssl->in_hdr; + } MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen =" " %" MBEDTLS_PRINTF_SIZET ", type = %u, hslen = %" @@ -3292,10 +3296,59 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) } } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ - /* With TLS we don't handle fragmentation (for now) */ - if (ssl->in_msglen < ssl->in_hslen) { - MBEDTLS_SSL_DEBUG_MSG(1, ("TLS handshake fragmentation not supported")); - return MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; + { + int ret; + const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; + const size_t msg_hslen = (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen); + + MBEDTLS_SSL_DEBUG_MSG(3, + ("handshake fragment: %" MBEDTLS_PRINTF_SIZET " .. %" + MBEDTLS_PRINTF_SIZET " of %" + MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET, + ssl->in_hsfraglen, ssl->in_hsfraglen + msg_hslen, + ssl->in_hslen, ssl->in_msglen)); + (void) msg_hslen; + if (ssl->in_msglen < hs_remain) { + ssl->in_hsfraglen += ssl->in_msglen; + ssl->in_hdr = ssl->in_msg + ssl->in_msglen; + ssl->in_msglen = 0; + mbedtls_ssl_update_in_pointers(ssl); + return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; + } + if (ssl->in_hshdr != ssl->in_hdr) { + /* + * At ssl->in_hshdr we have a sequence of records that cover the next handshake + * record, each with its own record header that we need to remove. + * Note that the reassembled record size may not equal the size of the message, + * there maybe bytes from the next message following it. + */ + size_t merged_rec_len = 0; + unsigned char *p = ssl->in_hshdr, *q = NULL; + do { + mbedtls_record rec; + ret = ssl_parse_record_header(ssl, p, mbedtls_ssl_in_hdr_len(ssl), &rec); + if (ret != 0) { + return ret; + } + merged_rec_len += rec.data_len; + p = rec.buf + rec.buf_len; + if (q != NULL) { + memmove(q, rec.buf + rec.data_offset, rec.data_len); + q += rec.data_len; + } else { + q = p; + } + } while (merged_rec_len < ssl->in_hslen); + ssl->in_hdr = ssl->in_hshdr; + mbedtls_ssl_update_in_pointers(ssl); + ssl->in_msglen = merged_rec_len; + /* Adjust message length. */ + MBEDTLS_PUT_UINT16_BE(merged_rec_len, ssl->in_len, 0); + ssl->in_hsfraglen = 0; + ssl->in_hshdr = NULL; + MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record", + ssl->in_hdr, mbedtls_ssl_in_hdr_len(ssl) + merged_rec_len); + } } return 0; @@ -4640,6 +4693,16 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) return MBEDTLS_ERR_SSL_INTERNAL_ERROR; } + if (ssl->in_hsfraglen != 0) { + /* Not all handshake fragments have arrived, do not consume. */ + MBEDTLS_SSL_DEBUG_MSG(3, + ("waiting for more fragments (%" MBEDTLS_PRINTF_SIZET " of %" + MBEDTLS_PRINTF_SIZET ", %" MBEDTLS_PRINTF_SIZET " left)", + ssl->in_hsfraglen, ssl->in_hslen, + ssl->in_hslen - ssl->in_hsfraglen)); + return 0; + } + /* * Get next Handshake message in the current record */ @@ -4665,6 +4728,7 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) ssl->in_msglen -= ssl->in_hslen; memmove(ssl->in_msg, ssl->in_msg + ssl->in_hslen, ssl->in_msglen); + MBEDTLS_PUT_UINT16_BE(ssl->in_msglen, ssl->in_len, 0); MBEDTLS_SSL_DEBUG_BUF(4, "remaining content in record", ssl->in_msg, ssl->in_msglen); @@ -5339,7 +5403,7 @@ void mbedtls_ssl_update_in_pointers(mbedtls_ssl_context *ssl) } else #endif { - ssl->in_ctr = ssl->in_hdr - MBEDTLS_SSL_SEQUENCE_NUMBER_LEN; + ssl->in_ctr = ssl->in_buf; ssl->in_len = ssl->in_hdr + 3; #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) ssl->in_cid = ssl->in_len; @@ -5355,24 +5419,35 @@ void mbedtls_ssl_update_in_pointers(mbedtls_ssl_context *ssl) * Setup an SSL context */ -void mbedtls_ssl_reset_in_out_pointers(mbedtls_ssl_context *ssl) +void mbedtls_ssl_reset_in_pointers(mbedtls_ssl_context *ssl) +{ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { + ssl->in_hdr = ssl->in_buf; + } else +#endif + { + ssl->in_hdr = ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN; + } + + /* Derive other internal pointers. */ + mbedtls_ssl_update_in_pointers(ssl); +} + +void mbedtls_ssl_reset_out_pointers(mbedtls_ssl_context *ssl) { /* Set the incoming and outgoing record pointers. */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { ssl->out_hdr = ssl->out_buf; - ssl->in_hdr = ssl->in_buf; } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ { ssl->out_ctr = ssl->out_buf; - ssl->out_hdr = ssl->out_buf + 8; - ssl->in_hdr = ssl->in_buf + 8; + ssl->out_hdr = ssl->out_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN; } - /* Derive other internal pointers. */ mbedtls_ssl_update_out_pointers(ssl, NULL /* no transform enabled */); - mbedtls_ssl_update_in_pointers(ssl); } /* diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c773365bf6..4bb170b15e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -344,12 +344,17 @@ static void handle_buffer_resizing(mbedtls_ssl_context *ssl, int downsizing, size_t out_buf_new_len) { int modified = 0; - size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0; + size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0, hdr_in = 0; size_t written_out = 0, iv_offset_out = 0, len_offset_out = 0; + size_t hshdr_in = 0; if (ssl->in_buf != NULL) { written_in = ssl->in_msg - ssl->in_buf; iv_offset_in = ssl->in_iv - ssl->in_buf; len_offset_in = ssl->in_len - ssl->in_buf; + hdr_in = ssl->in_hdr - ssl->in_buf; + if (ssl->in_hshdr != NULL) { + hshdr_in = ssl->in_hshdr - ssl->in_buf; + } if (downsizing ? ssl->in_buf_len > in_buf_new_len && ssl->in_left < in_buf_new_len : ssl->in_buf_len < in_buf_new_len) { @@ -381,7 +386,10 @@ static void handle_buffer_resizing(mbedtls_ssl_context *ssl, int downsizing, } if (modified) { /* Update pointers here to avoid doing it twice. */ - mbedtls_ssl_reset_in_out_pointers(ssl); + ssl->in_hdr = ssl->in_buf + hdr_in; + mbedtls_ssl_update_in_pointers(ssl); + mbedtls_ssl_reset_out_pointers(ssl); + /* Fields below might not be properly updated with record * splitting or with CID, so they are manually updated here. */ ssl->out_msg = ssl->out_buf + written_out; @@ -391,6 +399,9 @@ static void handle_buffer_resizing(mbedtls_ssl_context *ssl, int downsizing, ssl->in_msg = ssl->in_buf + written_in; ssl->in_len = ssl->in_buf + len_offset_in; ssl->in_iv = ssl->in_buf + iv_offset_in; + if (ssl->in_hshdr != NULL) { + ssl->in_hshdr = ssl->in_buf + hshdr_in; + } } } #endif /* MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH */ @@ -1484,6 +1495,8 @@ void mbedtls_ssl_session_reset_msg_layer(mbedtls_ssl_context *ssl, ssl->in_hslen = 0; ssl->keep_current_message = 0; ssl->transform_in = NULL; + ssl->in_hshdr = NULL; + ssl->in_hsfraglen = 0; #if defined(MBEDTLS_SSL_PROTO_DTLS) ssl->next_record_offset = 0; From db2da526ff6ae3e05619d975e6b8af78f5b12336 Mon Sep 17 00:00:00 2001 From: Deomid Ryabkov Date: Wed, 15 Jan 2025 19:26:47 +0000 Subject: [PATCH 02/17] Update ChangeLog.d/tls-hs-defrag-in.txt Co-authored-by: minosgalanakis <30719586+minosgalanakis@users.noreply.github.com> Signed-off-by: Deomid Ryabkov --- ChangeLog.d/tls-hs-defrag-in.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/tls-hs-defrag-in.txt b/ChangeLog.d/tls-hs-defrag-in.txt index 8c57200119..3555a789d8 100644 --- a/ChangeLog.d/tls-hs-defrag-in.txt +++ b/ChangeLog.d/tls-hs-defrag-in.txt @@ -1,2 +1,2 @@ -Change +Changes * Defragment incoming TLS handshake messages. From 1f4088ceda88bcf411b04982efeeb2df634dc848 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Sat, 18 Jan 2025 15:58:57 +0200 Subject: [PATCH 03/17] Review comments Signed-off-by: Deomid rojer Ryabkov --- library/ssl_msg.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 99b60f382e..50dda51cad 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3299,15 +3299,14 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) { int ret; const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; - const size_t msg_hslen = (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen); - MBEDTLS_SSL_DEBUG_MSG(3, ("handshake fragment: %" MBEDTLS_PRINTF_SIZET " .. %" MBEDTLS_PRINTF_SIZET " of %" MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET, - ssl->in_hsfraglen, ssl->in_hsfraglen + msg_hslen, + ssl->in_hsfraglen, + ssl->in_hsfraglen + + (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen), ssl->in_hslen, ssl->in_msglen)); - (void) msg_hslen; if (ssl->in_msglen < hs_remain) { ssl->in_hsfraglen += ssl->in_msglen; ssl->in_hdr = ssl->in_msg + ssl->in_msglen; @@ -5425,7 +5424,7 @@ void mbedtls_ssl_reset_in_pointers(mbedtls_ssl_context *ssl) if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { ssl->in_hdr = ssl->in_buf; } else -#endif +#endif /* MBEDTLS_SSL_PROTO_DTLS */ { ssl->in_hdr = ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN; } From 96e2290e3d7e1a8a502163fed190971021570780 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Sun, 26 Jan 2025 10:43:42 +0200 Subject: [PATCH 04/17] Remove mbedtls_ssl_reset_in_out_pointers Signed-off-by: Deomid rojer Ryabkov --- library/ssl_misc.h | 7 +------ library/ssl_tls.c | 6 ++++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 89947e9575..348c3197dd 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1831,15 +1831,10 @@ MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_check_timer(mbedtls_ssl_context *ssl); void mbedtls_ssl_reset_in_pointers(mbedtls_ssl_context *ssl); +void mbedtls_ssl_update_in_pointers(mbedtls_ssl_context *ssl); void mbedtls_ssl_reset_out_pointers(mbedtls_ssl_context *ssl); -static inline void mbedtls_ssl_reset_in_out_pointers(mbedtls_ssl_context *ssl) -{ - mbedtls_ssl_reset_in_pointers(ssl); - mbedtls_ssl_reset_out_pointers(ssl); -} void mbedtls_ssl_update_out_pointers(mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform); -void mbedtls_ssl_update_in_pointers(mbedtls_ssl_context *ssl); MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_session_reset_int(mbedtls_ssl_context *ssl, int partial); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4bb170b15e..f7554d202d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1420,7 +1420,8 @@ int mbedtls_ssl_setup(mbedtls_ssl_context *ssl, goto error; } - mbedtls_ssl_reset_in_out_pointers(ssl); + mbedtls_ssl_reset_in_pointers(ssl); + mbedtls_ssl_reset_out_pointers(ssl); #if defined(MBEDTLS_SSL_DTLS_SRTP) memset(&ssl->dtls_srtp_info, 0, sizeof(ssl->dtls_srtp_info)); @@ -1485,7 +1486,8 @@ void mbedtls_ssl_session_reset_msg_layer(mbedtls_ssl_context *ssl, /* Cancel any possibly running timer */ mbedtls_ssl_set_timer(ssl, 0); - mbedtls_ssl_reset_in_out_pointers(ssl); + mbedtls_ssl_reset_in_pointers(ssl); + mbedtls_ssl_reset_out_pointers(ssl); /* Reset incoming message parsing */ ssl->in_offt = NULL; From 5c853ea2c557c8b68c8ae75ea068ff8073b1185e Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Sun, 26 Jan 2025 11:10:54 +0200 Subject: [PATCH 05/17] Allow fragments less HS msg header size (4 bytes) Except the first Signed-off-by: Deomid rojer Ryabkov --- library/ssl_msg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 50dda51cad..83920f6327 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3220,7 +3220,8 @@ static uint32_t ssl_get_hs_total_len(mbedtls_ssl_context const *ssl) int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) { - if (ssl->in_msglen < mbedtls_ssl_hs_hdr_len(ssl)) { + /* First handshake fragment must at least include the header. */ + if (ssl->in_msglen < mbedtls_ssl_hs_hdr_len(ssl) && ssl->in_hslen == 0) { MBEDTLS_SSL_DEBUG_MSG(1, ("handshake message too short: %" MBEDTLS_PRINTF_SIZET, ssl->in_msglen)); return MBEDTLS_ERR_SSL_INVALID_RECORD; From 85ec2b36326cdefdbc11bd57c002c70fd9fe1d70 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Mon, 27 Jan 2025 22:37:37 +0400 Subject: [PATCH 06/17] Add a safety check for in_hsfraglen Signed-off-by: Deomid rojer Ryabkov --- library/ssl_msg.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 83920f6327..fcab63eef5 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3298,6 +3298,9 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ { + if (ssl->in_hsfraglen > ssl->in_hslen) { + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + } int ret; const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; MBEDTLS_SSL_DEBUG_MSG(3, From bbe8745d193e2daa60d0c350e335f7ea7d289050 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Thu, 13 Feb 2025 13:41:51 +0300 Subject: [PATCH 07/17] Remove in_hshdr The first fragment of a fragmented handshake message always starts at the beginning of the buffer so there's no need to store it. Signed-off-by: Deomid rojer Ryabkov --- include/mbedtls/ssl.h | 4 ++-- library/ssl_msg.c | 20 +++++++++----------- library/ssl_tls.c | 10 +--------- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 26ea791208..8f7bb1feb9 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1817,8 +1817,8 @@ struct mbedtls_ssl_context { size_t MBEDTLS_PRIVATE(in_hslen); /*!< current handshake message length, including the handshake header */ - unsigned char *MBEDTLS_PRIVATE(in_hshdr); /*!< original handshake header start */ - size_t MBEDTLS_PRIVATE(in_hsfraglen); /*!< accumulated hs fragments length */ + size_t MBEDTLS_PRIVATE(in_hsfraglen); /*!< accumulated length of hs fragments + (up to in_hslen) */ int MBEDTLS_PRIVATE(nb_zero); /*!< # of 0-length encrypted messages */ int MBEDTLS_PRIVATE(keep_current_message); /*!< drop or reuse current message diff --git a/library/ssl_msg.c b/library/ssl_msg.c index fcab63eef5..8b1aa23415 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3230,7 +3230,6 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) if (ssl->in_hslen == 0) { ssl->in_hslen = mbedtls_ssl_hs_hdr_len(ssl) + ssl_get_hs_total_len(ssl); ssl->in_hsfraglen = 0; - ssl->in_hshdr = ssl->in_hdr; } MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen =" @@ -3297,10 +3296,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) } } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ - { - if (ssl->in_hsfraglen > ssl->in_hslen) { - return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - } + if (ssl->in_hsfraglen <= ssl->in_hslen) { int ret; const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; MBEDTLS_SSL_DEBUG_MSG(3, @@ -3318,15 +3314,16 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) mbedtls_ssl_update_in_pointers(ssl); return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; } - if (ssl->in_hshdr != ssl->in_hdr) { + if (ssl->in_hsfraglen > 0) { /* - * At ssl->in_hshdr we have a sequence of records that cover the next handshake + * At in_first_hdr we have a sequence of records that cover the next handshake * record, each with its own record header that we need to remove. * Note that the reassembled record size may not equal the size of the message, - * there maybe bytes from the next message following it. + * there may be more messages after it, complete or partial. */ + unsigned char *in_first_hdr = ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN; + unsigned char *p = in_first_hdr, *q = NULL; size_t merged_rec_len = 0; - unsigned char *p = ssl->in_hshdr, *q = NULL; do { mbedtls_record rec; ret = ssl_parse_record_header(ssl, p, mbedtls_ssl_in_hdr_len(ssl), &rec); @@ -3342,16 +3339,17 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) q = p; } } while (merged_rec_len < ssl->in_hslen); - ssl->in_hdr = ssl->in_hshdr; + ssl->in_hdr = in_first_hdr; mbedtls_ssl_update_in_pointers(ssl); ssl->in_msglen = merged_rec_len; /* Adjust message length. */ MBEDTLS_PUT_UINT16_BE(merged_rec_len, ssl->in_len, 0); ssl->in_hsfraglen = 0; - ssl->in_hshdr = NULL; MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record", ssl->in_hdr, mbedtls_ssl_in_hdr_len(ssl) + merged_rec_len); } + } else { + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; } return 0; diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f7554d202d..0c394946a5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -346,15 +346,11 @@ static void handle_buffer_resizing(mbedtls_ssl_context *ssl, int downsizing, int modified = 0; size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0, hdr_in = 0; size_t written_out = 0, iv_offset_out = 0, len_offset_out = 0; - size_t hshdr_in = 0; if (ssl->in_buf != NULL) { written_in = ssl->in_msg - ssl->in_buf; iv_offset_in = ssl->in_iv - ssl->in_buf; len_offset_in = ssl->in_len - ssl->in_buf; hdr_in = ssl->in_hdr - ssl->in_buf; - if (ssl->in_hshdr != NULL) { - hshdr_in = ssl->in_hshdr - ssl->in_buf; - } if (downsizing ? ssl->in_buf_len > in_buf_new_len && ssl->in_left < in_buf_new_len : ssl->in_buf_len < in_buf_new_len) { @@ -399,9 +395,6 @@ static void handle_buffer_resizing(mbedtls_ssl_context *ssl, int downsizing, ssl->in_msg = ssl->in_buf + written_in; ssl->in_len = ssl->in_buf + len_offset_in; ssl->in_iv = ssl->in_buf + iv_offset_in; - if (ssl->in_hshdr != NULL) { - ssl->in_hshdr = ssl->in_buf + hshdr_in; - } } } #endif /* MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH */ @@ -1495,10 +1488,9 @@ void mbedtls_ssl_session_reset_msg_layer(mbedtls_ssl_context *ssl, ssl->in_msgtype = 0; ssl->in_msglen = 0; ssl->in_hslen = 0; + ssl->in_hsfraglen = 0; ssl->keep_current_message = 0; ssl->transform_in = NULL; - ssl->in_hshdr = NULL; - ssl->in_hsfraglen = 0; #if defined(MBEDTLS_SSL_PROTO_DTLS) ssl->next_record_offset = 0; From 69f8f45e6f561651c1000af78a81ecb374af11bd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Feb 2025 16:08:59 +0100 Subject: [PATCH 08/17] Minor readability improvement No behavior change. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 8b1aa23415..fb91b948d7 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5032,10 +5032,12 @@ static int ssl_get_next_record(mbedtls_ssl_context *ssl) return ret; } - if (ssl->conf->badmac_limit != 0 && - ++ssl->badmac_seen >= ssl->conf->badmac_limit) { - MBEDTLS_SSL_DEBUG_MSG(1, ("too many records with bad MAC")); - return MBEDTLS_ERR_SSL_INVALID_MAC; + if (ssl->conf->badmac_limit != 0) { + ++ssl->badmac_seen; + if (ssl->badmac_seen >= ssl->conf->badmac_limit) { + MBEDTLS_SSL_DEBUG_MSG(1, ("too many records with bad MAC")); + return MBEDTLS_ERR_SSL_INVALID_MAC; + } } /* As above, invalid records cause From f6a676d93f47ba4f3cc78ec9b4adc02397ce4df4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Feb 2025 16:10:14 +0100 Subject: [PATCH 09/17] Rename badmac_seen to badmac_seen_or_in_hsfraglen Prepare to unify two fields of the `mbedtls_ssl_context` structure: `badmac_seen` (always present but only used in DTLS) and `in_hsfraglen` (always present but only used in non-DTLS TLS). Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 11 ++++++++++- library/ssl_msg.c | 4 ++-- library/ssl_tls.c | 6 +++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 8f7bb1feb9..0cad449011 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1724,7 +1724,16 @@ struct mbedtls_ssl_context { int MBEDTLS_PRIVATE(early_data_state); #endif - unsigned MBEDTLS_PRIVATE(badmac_seen); /*!< records with a bad MAC received */ + /** Multipurpose field. + * + * - DTLS: records with a bad MAC received. + * - TLS: accumulated length of handshake fragments (up to ::in_hslen). + * + * This field is multipurpose in order to preserve the ABI in the + * Mbed TLS 3.6 LTS branch. Until 3.6.2, it was only used in DTLS + * and called `badmac_seen`. + */ + unsigned MBEDTLS_PRIVATE(badmac_seen_or_in_hsfraglen); #if defined(MBEDTLS_X509_CRT_PARSE_C) /** Callback to customize X.509 certificate chain verification */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index fb91b948d7..1ad8f5ab91 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5033,8 +5033,8 @@ static int ssl_get_next_record(mbedtls_ssl_context *ssl) } if (ssl->conf->badmac_limit != 0) { - ++ssl->badmac_seen; - if (ssl->badmac_seen >= ssl->conf->badmac_limit) { + ++ssl->badmac_seen_or_in_hsfraglen; + if (ssl->badmac_seen_or_in_hsfraglen >= ssl->conf->badmac_limit) { MBEDTLS_SSL_DEBUG_MSG(1, ("too many records with bad MAC")); return MBEDTLS_ERR_SSL_INVALID_MAC; } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0c394946a5..f8cd74b91e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5021,7 +5021,7 @@ static const unsigned char ssl_serialized_context_header[] = { * uint8 in_cid<0..2^8-1> // Connection ID: expected incoming value * uint8 out_cid<0..2^8-1> // Connection ID: outgoing value to use * // fields from ssl_context - * uint32 badmac_seen; // DTLS: number of records with failing MAC + * uint32 badmac_seen_or_in_hsfraglen; // DTLS: number of records with failing MAC * uint64 in_window_top; // DTLS: last validated record seq_num * uint64 in_window; // DTLS: bitmask for replay protection * uint8 disable_datagram_packing; // DTLS: only one record per datagram @@ -5163,7 +5163,7 @@ int mbedtls_ssl_context_save(mbedtls_ssl_context *ssl, */ used += 4; if (used <= buf_len) { - MBEDTLS_PUT_UINT32_BE(ssl->badmac_seen, p, 0); + MBEDTLS_PUT_UINT32_BE(ssl->badmac_seen_or_in_hsfraglen, p, 0); p += 4; } @@ -5393,7 +5393,7 @@ static int ssl_context_load(mbedtls_ssl_context *ssl, return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; } - ssl->badmac_seen = MBEDTLS_GET_UINT32_BE(p, 0); + ssl->badmac_seen_or_in_hsfraglen = MBEDTLS_GET_UINT32_BE(p, 0); p += 4; #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) From ebdd405f6821b16a627fc723f2ed9a0e7d5c702e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Feb 2025 16:25:24 +0100 Subject: [PATCH 10/17] Change the type of in_hsfraglen to unsigned In the `mbedtls_ssl_context` structure, change the type of `in_hsfraglen` from `size_t` to `unsigned`. This is in preparation for merging `in_hsfraglen` into `badmac_seen_or_in_hsfraglen`, which has the type `unsigned` and cannot change since we do not want to change the ABI. Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 2 +- library/ssl_msg.c | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 0cad449011..161eb7e74e 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1826,7 +1826,7 @@ struct mbedtls_ssl_context { size_t MBEDTLS_PRIVATE(in_hslen); /*!< current handshake message length, including the handshake header */ - size_t MBEDTLS_PRIVATE(in_hsfraglen); /*!< accumulated length of hs fragments + unsigned MBEDTLS_PRIVATE(in_hsfraglen); /*!< accumulated length of hs fragments (up to in_hslen) */ int MBEDTLS_PRIVATE(nb_zero); /*!< # of 0-length encrypted messages */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 1ad8f5ab91..5940abafea 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -25,6 +25,7 @@ #include "constant_time_internal.h" #include "mbedtls/constant_time.h" +#include #include #if defined(MBEDTLS_USE_PSA_CRYPTO) @@ -3300,15 +3301,22 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) int ret; const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; MBEDTLS_SSL_DEBUG_MSG(3, - ("handshake fragment: %" MBEDTLS_PRINTF_SIZET " .. %" + ("handshake fragment: %u .. %" MBEDTLS_PRINTF_SIZET " of %" MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET, ssl->in_hsfraglen, - ssl->in_hsfraglen + + (size_t) ssl->in_hsfraglen + (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen), ssl->in_hslen, ssl->in_msglen)); if (ssl->in_msglen < hs_remain) { - ssl->in_hsfraglen += ssl->in_msglen; + /* ssl->in_msglen is a 25-bit value since it is the sum of the + * header length plus the payload length, the header length is 4 + * and the payload length was received on the wire encoded as + * 3 octets. We don't support 16-bit platforms; more specifically, + * we assume that both unsigned and size_t are at least 32 bits. + * Therefore there is no possible integer overflow here. + */ + ssl->in_hsfraglen += (unsigned) ssl->in_msglen; ssl->in_hdr = ssl->in_msg + ssl->in_msglen; ssl->in_msglen = 0; mbedtls_ssl_update_in_pointers(ssl); @@ -4697,7 +4705,7 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) if (ssl->in_hsfraglen != 0) { /* Not all handshake fragments have arrived, do not consume. */ MBEDTLS_SSL_DEBUG_MSG(3, - ("waiting for more fragments (%" MBEDTLS_PRINTF_SIZET " of %" + ("waiting for more fragments (%u of %" MBEDTLS_PRINTF_SIZET ", %" MBEDTLS_PRINTF_SIZET " left)", ssl->in_hsfraglen, ssl->in_hslen, ssl->in_hslen - ssl->in_hsfraglen)); From b710599e4ac1138a67fb87a8c08a2690c0ea2fed Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Feb 2025 16:28:51 +0100 Subject: [PATCH 11/17] Merge in_hsfraglen with badmac_seen_or_in_hsfraglen In the `mbedtls_ssl_context` structure, merge the field `in_hsfraglen` into `badmac_seen_or_in_hsfraglen`. This restores the ABI of `libmbedtls` as it was in Mbed TLS 3.6.0 through 3.6.2. The field `badmac_seen_or_in_hsfraglen` (formerly `badmac_seen`) was only used for DTLS (despite being present in non-DTLS builds), and the field `in_hsfraglen` was only used in non-DTLS TLS. Therefore the two values can be stored in the same field. Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 2 -- library/ssl_msg.c | 22 +++++++++++----------- library/ssl_tls.c | 2 +- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 161eb7e74e..09a58a25ae 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1826,8 +1826,6 @@ struct mbedtls_ssl_context { size_t MBEDTLS_PRIVATE(in_hslen); /*!< current handshake message length, including the handshake header */ - unsigned MBEDTLS_PRIVATE(in_hsfraglen); /*!< accumulated length of hs fragments - (up to in_hslen) */ int MBEDTLS_PRIVATE(nb_zero); /*!< # of 0-length encrypted messages */ int MBEDTLS_PRIVATE(keep_current_message); /*!< drop or reuse current message diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 5940abafea..7b11e4d06a 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3230,7 +3230,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) if (ssl->in_hslen == 0) { ssl->in_hslen = mbedtls_ssl_hs_hdr_len(ssl) + ssl_get_hs_total_len(ssl); - ssl->in_hsfraglen = 0; + ssl->badmac_seen_or_in_hsfraglen = 0; } MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen =" @@ -3297,15 +3297,15 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) } } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ - if (ssl->in_hsfraglen <= ssl->in_hslen) { + if (ssl->badmac_seen_or_in_hsfraglen <= ssl->in_hslen) { int ret; - const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; + const size_t hs_remain = ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen; MBEDTLS_SSL_DEBUG_MSG(3, ("handshake fragment: %u .. %" MBEDTLS_PRINTF_SIZET " of %" MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET, - ssl->in_hsfraglen, - (size_t) ssl->in_hsfraglen + + ssl->badmac_seen_or_in_hsfraglen, + (size_t) ssl->badmac_seen_or_in_hsfraglen + (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen), ssl->in_hslen, ssl->in_msglen)); if (ssl->in_msglen < hs_remain) { @@ -3316,13 +3316,13 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) * we assume that both unsigned and size_t are at least 32 bits. * Therefore there is no possible integer overflow here. */ - ssl->in_hsfraglen += (unsigned) ssl->in_msglen; + ssl->badmac_seen_or_in_hsfraglen += (unsigned) ssl->in_msglen; ssl->in_hdr = ssl->in_msg + ssl->in_msglen; ssl->in_msglen = 0; mbedtls_ssl_update_in_pointers(ssl); return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; } - if (ssl->in_hsfraglen > 0) { + if (ssl->badmac_seen_or_in_hsfraglen > 0) { /* * At in_first_hdr we have a sequence of records that cover the next handshake * record, each with its own record header that we need to remove. @@ -3352,7 +3352,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) ssl->in_msglen = merged_rec_len; /* Adjust message length. */ MBEDTLS_PUT_UINT16_BE(merged_rec_len, ssl->in_len, 0); - ssl->in_hsfraglen = 0; + ssl->badmac_seen_or_in_hsfraglen = 0; MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record", ssl->in_hdr, mbedtls_ssl_in_hdr_len(ssl) + merged_rec_len); } @@ -4702,13 +4702,13 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) return MBEDTLS_ERR_SSL_INTERNAL_ERROR; } - if (ssl->in_hsfraglen != 0) { + if (ssl->badmac_seen_or_in_hsfraglen != 0) { /* Not all handshake fragments have arrived, do not consume. */ MBEDTLS_SSL_DEBUG_MSG(3, ("waiting for more fragments (%u of %" MBEDTLS_PRINTF_SIZET ", %" MBEDTLS_PRINTF_SIZET " left)", - ssl->in_hsfraglen, ssl->in_hslen, - ssl->in_hslen - ssl->in_hsfraglen)); + ssl->badmac_seen_or_in_hsfraglen, ssl->in_hslen, + ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen)); return 0; } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f8cd74b91e..8428b38725 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1488,7 +1488,7 @@ void mbedtls_ssl_session_reset_msg_layer(mbedtls_ssl_context *ssl, ssl->in_msgtype = 0; ssl->in_msglen = 0; ssl->in_hslen = 0; - ssl->in_hsfraglen = 0; + ssl->badmac_seen_or_in_hsfraglen = 0; ssl->keep_current_message = 0; ssl->transform_in = NULL; From cb72cd2ec3e82458e0fcc5dbfe89dae1edd4ee99 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Feb 2025 16:36:36 +0100 Subject: [PATCH 12/17] Don't reset badmac_seen on a DTLS client reconnect Signed-off-by: Gilles Peskine --- library/ssl_tls.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8428b38725..7f74248252 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1488,10 +1488,15 @@ void mbedtls_ssl_session_reset_msg_layer(mbedtls_ssl_context *ssl, ssl->in_msgtype = 0; ssl->in_msglen = 0; ssl->in_hslen = 0; - ssl->badmac_seen_or_in_hsfraglen = 0; ssl->keep_current_message = 0; ssl->transform_in = NULL; + /* TLS: reset in_hsfraglen, which is part of message parsing. + * DTLS: on a client reconnect, don't reset badmac_seen. */ + if (!partial) { + ssl->badmac_seen_or_in_hsfraglen = 0; + } + #if defined(MBEDTLS_SSL_PROTO_DTLS) ssl->next_record_offset = 0; ssl->in_epoch = 0; From 55151d3da67936fda6f7d6b7aae392194abd0aac Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Feb 2025 23:09:00 +0100 Subject: [PATCH 13/17] Fix Doxygen misuse Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 09a58a25ae..597da2571f 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1727,7 +1727,7 @@ struct mbedtls_ssl_context { /** Multipurpose field. * * - DTLS: records with a bad MAC received. - * - TLS: accumulated length of handshake fragments (up to ::in_hslen). + * - TLS: accumulated length of handshake fragments (up to \c in_hslen). * * This field is multipurpose in order to preserve the ABI in the * Mbed TLS 3.6 LTS branch. Until 3.6.2, it was only used in DTLS From c52273d017fc185638b8e7da878695fa090b4105 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 18 Feb 2025 14:11:25 +0100 Subject: [PATCH 14/17] Add a note about badmac_seen's new name in ssl_context_info Signed-off-by: Gilles Peskine --- programs/ssl/ssl_context_info.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/programs/ssl/ssl_context_info.c b/programs/ssl/ssl_context_info.c index 51e87817ad..b9a0fe8751 100644 --- a/programs/ssl/ssl_context_info.c +++ b/programs/ssl/ssl_context_info.c @@ -743,6 +743,13 @@ static void print_deserialized_ssl_session(const uint8_t *ssl, uint32_t len, * uint8 alpn_chosen_len; * uint8 alpn_chosen<0..2^8-1> // ALPN: negotiated application protocol * + * Note: In the mbedtls_ssl_context structure, badmac_seen is called + * badmac_seen_or_in_hsfraglen since Mbed TLS 3.6.2. The field contains + * the badmac_seen value in DTLS, and a handshake parsing intermediate + * value in non-DTLS TLS. The value is only meaningful for DTLS and should + * not be saved in non-DTLS TLS, so in this program, the context info file + * filed remains badmac_seen. + * * /p ssl pointer to serialized session * /p len number of bytes in the buffer */ From 2878a0559eb302652a3b3c9ffc8439f79008e631 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Sat, 1 Feb 2025 15:33:37 +0200 Subject: [PATCH 15/17] Remove obselete checks due to the introduction of handhsake defragmen... tation. h/t @waleed-elmelegy-arm https://github.com/Mbed-TLS/mbedtls/pull/9928/commits/909e71672f6a11219e12347c2d7d2429b98e6500 Signed-off-by: Waleed Elmelegy Signed-off-by: Deomid rojer Ryabkov --- library/ssl_tls12_server.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 03722ac33c..3db1f44e34 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1061,23 +1061,6 @@ read_record_header: size_t handshake_len = MBEDTLS_GET_UINT24_BE(buf, 1); MBEDTLS_SSL_DEBUG_MSG(3, ("client hello v3, handshake len.: %u", (unsigned) handshake_len)); - - /* The record layer has a record size limit of 2^14 - 1 and - * fragmentation is not supported, so buf[1] should be zero. */ - if (buf[1] != 0) { - MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message: %u != 0", - (unsigned) buf[1])); - return MBEDTLS_ERR_SSL_DECODE_ERROR; - } - - /* We don't support fragmentation of ClientHello (yet?) */ - if (msg_len != mbedtls_ssl_hs_hdr_len(ssl) + handshake_len) { - MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message: %u != %u + %u", - (unsigned) msg_len, - (unsigned) mbedtls_ssl_hs_hdr_len(ssl), - (unsigned) handshake_len)); - return MBEDTLS_ERR_SSL_DECODE_ERROR; - } } #if defined(MBEDTLS_SSL_PROTO_DTLS) From 716aead3b95353510e4c7c38a337935e74e182c5 Mon Sep 17 00:00:00 2001 From: Deomid rojer Ryabkov Date: Tue, 4 Feb 2025 12:08:15 +0200 Subject: [PATCH 16/17] Update the changelog message Signed-off-by: Deomid rojer Ryabkov --- ChangeLog.d/tls-hs-defrag-in.txt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ChangeLog.d/tls-hs-defrag-in.txt b/ChangeLog.d/tls-hs-defrag-in.txt index 3555a789d8..55103c9a42 100644 --- a/ChangeLog.d/tls-hs-defrag-in.txt +++ b/ChangeLog.d/tls-hs-defrag-in.txt @@ -1,2 +1,5 @@ -Changes - * Defragment incoming TLS handshake messages. +Bugfix + * Support re-assembly of fragmented handshake messages in TLS, as mandated + by the spec. Lack of support was causing handshake failures with some + servers, especially with TLS 1.3 in practice (though both protocol + version could be affected in principle, and both are fixed now). From 4726d20320e640c77acb9431ab098e865ab10adb Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Fri, 31 Jan 2025 11:11:06 +0000 Subject: [PATCH 17/17] Remove unused variable in ssl_server.c Signed-off-by: Waleed Elmelegy Signed-off-by: Deomid rojer Ryabkov --- library/ssl_tls12_server.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 3db1f44e34..67df4284a4 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1057,11 +1057,6 @@ read_record_header: MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message")); return MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; } - { - size_t handshake_len = MBEDTLS_GET_UINT24_BE(buf, 1); - MBEDTLS_SSL_DEBUG_MSG(3, ("client hello v3, handshake len.: %u", - (unsigned) handshake_len)); - } #if defined(MBEDTLS_SSL_PROTO_DTLS) if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) {