Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defragment incoming TLS handshake messages #9872

Merged
merged 10 commits into from
Feb 24, 2025
Merged

Defragment incoming TLS handshake messages #9872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ac2cf1f
Defragment incoming TLS handshake messages
rojer Mar 10, 2024
5f7c2c2
Update ChangeLog.d/tls-hs-defrag-in.txt
rojer Jan 15, 2025
cad11ad
Review comments
rojer Jan 18, 2025
3dfe75e
Remove mbedtls_ssl_reset_in_out_pointers
rojer Jan 26, 2025
aaa152e
Allow fragments less HS msg header size (4 bytes)
rojer Jan 26, 2025
b70e76a
Add a safety check for in_hsfraglen
rojer Jan 27, 2025
afa11db
Remove obselete checks due to the introduction of handhsake defragmen...
rojer Feb 1, 2025
eb77e5b
Update the changelog message
rojer Feb 4, 2025
cf4e6a1
Remove unused variable in ssl_server.c
waleed-elmelegy-arm Jan 31, 2025
dd14c0a
Remove in_hshdr
rojer Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog.d/tls-hs-defrag-in.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
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).
2 changes: 2 additions & 0 deletions include/mbedtls/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1808,6 +1808,8 @@ 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
(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
Expand Down
5 changes: 3 additions & 2 deletions library/ssl_misc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1829,10 +1829,11 @@ 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_update_in_pointers(mbedtls_ssl_context *ssl);
void mbedtls_ssl_reset_out_pointers(mbedtls_ssl_context *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);
Expand Down
102 changes: 89 additions & 13 deletions library/ssl_msg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3219,13 +3219,17 @@ 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;
}

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;
}

MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen ="
" %" MBEDTLS_PRINTF_SIZET ", type = %u, hslen = %"
Expand Down Expand Up @@ -3291,10 +3295,60 @@ 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;
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,
("handshake fragment: %" MBEDTLS_PRINTF_SIZET " .. %"
MBEDTLS_PRINTF_SIZET " of %"
MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET,
ssl->in_hsfraglen,
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_hdr = ssl->in_msg + ssl->in_msglen;
ssl->in_msglen = 0;
mbedtls_ssl_update_in_pointers(ssl);
Comment on lines +3311 to +3313
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing in #9989 reveals a bug: defragmentation of encrypted records does not work correctly in TLS 1.2 when the symmetric encryption is CBC (EtM or not), CCM or GCM. It works with any TLS 1.3 cipher suite, and with ChachaPoly and null encryption in TLS 1.2. The problematic cases are exactly the ones where the record includes an explicit IV (always 8 bytes).

In the initial handshake, only the Finished message is likely to be affected, and it's only 16 bytes, so this happens when the fragment size is less than 16 bytes. This can happen with larger fragment sizes during renegotiation.

In problematic scenarios, the fragment reassembly loop looks for the second fragment 8 bytes too early in the buffer. There is an 8-byte gap between the fragments, but the reassembly expects them to be consecutive.

It's not 100% clear to me yet, but I think the offset update here isn't correct. As far as I can tell from looking at an example in a debugger, at the end of the first incomplete fragment, ssl->in_msg and ssl->in_hdr end up pointing 8 bytes past the end of the end of the fragment.

return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING;
}
if (ssl->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.
* Note that the reassembled record size may not equal the size of the message,
* 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;
do {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I was trying to analyze and fix https://github.com/Mbed-TLS/mbedtls/pull/9872/files#r1967669302, I tried to make sense of what the various pointers and offsets should be at any given point (ssl->in_msg, ssl->in_hdr, ssl->in_hsfraglen, etc., as well as the various record fields while processing records), and I have a hard time figuring out whether the value I'm seeing is the value that should be there at any given point during parsing. I'm not sure if the reassembly loop should be changed, or if it's getting unexpected data.

One of the difficulties in understanding the code is that fragment accumulation is completely disconnected from fragment reassembly, so reassembly has to re-parse data. I would find it easier to understand if the structure of the code was: when we have finished parsing a fragment, if it wasn't the initial fragment, then merge it with the initial fragment. With this structure, there'd be fewer moving parts. Fragment reassembly would have access to the offsets and record data from the latest fragment, and wouldn't need to do any parsing, so it would be easier to figure out offsets. There would also be fewer opportunities for parsing errors or integer/buffer overflows. In addition, the input buffer would fill out less quickly — the current structure adds a 5- or 13-byte overhead per fragment. @rojer Did you try doing fragment reassembly incrementally? Are there any difficulties in doing it that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i don't see any reason why it couldn't be done that way. this just happened to be the way i went at it at the time.

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 = 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;
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;
Expand Down Expand Up @@ -4639,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
*/
Expand All @@ -4664,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);
Expand Down Expand Up @@ -5338,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;
Expand All @@ -5354,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 /* MBEDTLS_SSL_PROTO_DTLS */
{
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);
}

/*
Expand Down
15 changes: 11 additions & 4 deletions library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,12 +343,13 @@ 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;
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 (downsizing ?
ssl->in_buf_len > in_buf_new_len && ssl->in_left < in_buf_new_len :
ssl->in_buf_len < in_buf_new_len) {
Expand Down Expand Up @@ -380,7 +381,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;
Expand Down Expand Up @@ -1408,7 +1412,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));
Expand Down Expand Up @@ -1473,14 +1478,16 @@ 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;
ssl->nb_zero = 0;
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;

Expand Down
22 changes: 0 additions & 22 deletions library/ssl_tls12_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,28 +1056,6 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl)
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));

/* 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)
if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) {
Expand Down