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
+110 −41
Merged

Defragment incoming TLS handshake messages #9872

Changes from 2 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
2 changes: 2 additions & 0 deletions ChangeLog.d/tls-hs-defrag-in.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Changes
* Defragment incoming TLS handshake messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can eve make classify that as a bugfix: (1) there is nothing in the spec that says support for receiving fragemented messages is optional (quite the opposite: it's the first point in the implementation pitfalls section), and (2) it was causing interop failures in practice.

Also, I think we should expand a bit so that users who are not aware of TLS internals may make more sense of the entry, perhaps:

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

2 changes: 2 additions & 0 deletions include/mbedtls/ssl.h
View file
Original file line number Diff line number Diff line change
@@ -1808,6 +1808,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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

One more comment that was posted on the tests's pr 9928

@gilles-peskine-arm commented:

Introducing a new buffer pointer creates two risks: buffer overflow, and use-after-free.

I would prefer to have more clarity as to the size of the (sub-)buffer accessible from in_hshdr (it isn't in_hsfraglen), and also the impact on the in_hdr field (which has become less straightforward). I don't have any bad feeling about the current code here, but I'd prefer to have better documentation to help future maintainers (I do have a bad feeling about someone later fixing a bug that's unrelated to fragmentation, and missing some interaction between their bug and fragmentation).

I am a little worried about a use-after-free. handle_buffer_resizing takes care of this pointer, and it's the only risky place I can think of, but I am not very confident. Please reset in_hshdr in mbedtls_ssl_update_in_pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in_hshdr points to the first handshake fragment and may legitimately stay behind the current/last message and thus should not be reset in mbedtls_ssl_update_in_pointers.

the overview of the approach to defragmentation is as follows:

  1. we start with an empty buffer, in_hslen == 0 and in_hshdr == NULL
  2. when the first record of the handshake message is received (which has to be at least 4 bytes) we parse the expected length into in_hslen, set in_hshdr = in_hdr and in_hsfraglen = in_msglen.
  3. as more fragments arrive, we accumulate them in the buffer and keep track of the total length so far in_hsfraglen until it covers the entire expected handshake message. during this time in_hdr advances forward while in_hshdr stays behind.
  4. once we have enough fragments buffered, we merge them, starting from in_hshdr, removing the record headers and obtaining a complete handshake message, which we then process and start over.

i hope this makes it clear. any suggestions wrt comments or documentation are welcome.

as a more general note, i think that an extensive comment or perhaps even a readme explaining the ways in/out buffers and pointers are managed would be helpful. it took me a while to wrap my head around it when i started.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, that's very useful!

Would you mind adding comments? Both on the context fields, and also a comment in mbedtls_ssl_update_in_pointers to explain why this in-pointer should not be updated.

i think that an extensive comment or perhaps even a readme explaining the ways in/out buffers and pointers are managed would be helpful. it took me a while to wrap my head around it when i started.

Absolutely, yes! The more we add features like defragmentation, the more complicated it is, and it doesn't help that there's so little documentation. Whenever we identify new wisdom about how things work, we should write them down inside the code, even if it's incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, upon reflection i realized that we don't need to store this pointer: it will always be the first message in the buffer. so, instead in dd14c0a i'm removing it.

i tested manually with facebook as well as #9928 with this change - defrag tests are passing.

i also expanded in_hsfraglen comment a bit to clarify that it goes from 0 up to in_hslen.

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
8 changes: 7 additions & 1 deletion library/ssl_misc.h
View file
Original file line number Diff line number Diff line change
@@ -1829,7 +1829,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);
99 changes: 87 additions & 12 deletions library/ssl_msg.c
View file
Original file line number Diff line number Diff line change
@@ -3225,7 +3225,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 = %"
@@ -3291,10 +3295,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);
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_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 {
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 = 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;
@@ -4639,6 +4692,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
*/
@@ -4664,6 +4727,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);
@@ -5338,7 +5402,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;
@@ -5354,24 +5418,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);
}

/*
17 changes: 15 additions & 2 deletions library/ssl_tls.c
View file
Original file line number Diff line number Diff line change
@@ -343,12 +343,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) {
@@ -380,7 +385,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;
@@ -390,6 +398,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 */
@@ -1483,6 +1494,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;