Skip to content

Incremental TLS handshake defragmentation #10011

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

Merged
merged 14 commits into from
Mar 7, 2025
Merged

Incremental TLS handshake defragmentation #10011

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2e5a7ea
Fix Doxygen markup
gilles-peskine-arm Feb 12, 2025
9bdc8aa
Tweak "waiting for more handshake fragments" log message
gilles-peskine-arm Feb 28, 2025
0702772
Tweak handshake fragment log message
gilles-peskine-arm Mar 3, 2025
7a17696
mbedtls_ssl_prepare_handshake_record(): refactor first fragment prep
gilles-peskine-arm Feb 28, 2025
235eae9
mbedtls_ssl_prepare_handshake_record(): log offsets after decryption
gilles-peskine-arm Feb 28, 2025
e85ece6
Handshake defragmentation: reassemble incrementally
gilles-peskine-arm Feb 28, 2025
90a9593
Fix dodgy printf calls
gilles-peskine-arm Feb 25, 2025
e4a3fc2
Update framework
gilles-peskine-arm Mar 3, 2025
0851ec9
Fix end check before memmove
gilles-peskine-arm Mar 6, 2025
15c072f
Fix handshake defragmentation when the record has multiple messages
gilles-peskine-arm Mar 6, 2025
afb254c
Unify handshake fragment log messages
gilles-peskine-arm Mar 6, 2025
b8f1e4b
Pacify uncrustify
gilles-peskine-arm Mar 6, 2025
dab1cb5
Note unused variables when debugging is disabled
gilles-peskine-arm Mar 6, 2025
e34ec86
Fix a log message
gilles-peskine-arm Mar 7, 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
162 changes: 110 additions & 52 deletions library/ssl_msg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3049,64 +3049,122 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl)
}
} else
#endif /* MBEDTLS_SSL_PROTO_DTLS */
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
"..%" MBEDTLS_PRINTF_SIZET
" of %" MBEDTLS_PRINTF_SIZET,
ssl->in_msglen,
ssl->in_hsfraglen,
ssl->in_hsfraglen + ssl->in_msglen,
ssl->in_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);
{
unsigned char *const reassembled_record_start =
ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN;
unsigned char *const payload_start =
reassembled_record_start + mbedtls_ssl_in_hdr_len(ssl);
unsigned char *payload_end = payload_start + ssl->in_hsfraglen;

if (ssl->in_hsfraglen != 0) {
/* We already had a handshake fragment. Prepare to append
* to the initial segment. */
MBEDTLS_SSL_DEBUG_MSG(3,
("subsequent handshake fragment: %" MBEDTLS_PRINTF_SIZET
", %" MBEDTLS_PRINTF_SIZET
"..%" MBEDTLS_PRINTF_SIZET
" of %" MBEDTLS_PRINTF_SIZET,
ssl->in_msglen,
ssl->in_hsfraglen,
ssl->in_hsfraglen + ssl->in_msglen,
ssl->in_hslen));

const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen;
if (ssl->in_msglen > hs_remain) {
Copy link
Contributor

@mpg mpg Mar 6, 2025

Choose a reason for hiding this comment

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

IIUC we're being stricter than the protocol here: this condition means that there's more data in the record after the rest of this handshake message, presumably the (beginning of) the next handshake message, which is allowed by the standard unless I'm mistaken.

I don't have an issue with rejecting it anyway, but I'd like a comment explaining that we are rejecting something the standard allows and why we're doing that. Also, that's a limitation we should document. And I find the current debug message a bit confusing.

(Unless I'm mistaken about what's happening here.)

Copy link
Contributor Author

@gilles-peskine-arm gilles-peskine-arm Mar 6, 2025

Choose a reason for hiding this comment

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

Right.

I added this check because I believed that the old code silently ignored subsequent handshake messages if there is more than one in the same record. However, revisiting this, I'm not sure that was the case: here we do nothing, but ssl_consume_current_message handles it. So now I'm worried that I've broken some existing untested behavior. But if I remove the check here, I'm worried that the defragmentation code may be creating an unexpected state. I'll keep digging today.

Copy link
Contributor Author

@gilles-peskine-arm gilles-peskine-arm Mar 6, 2025

Choose a reason for hiding this comment

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

Leveraging #9976, I've crafted a test case with ServerHello and ServerCertificate in the same message, and that test case passes before incremental defragmentation. So this check removes functionality, and I'm going to remove it.

MBEDTLS_SSL_DEBUG_MSG(1, ("Handshake fragment too long: %"
MBEDTLS_PRINTF_SIZET " but only %"
MBEDTLS_PRINTF_SIZET " of %"
MBEDTLS_PRINTF_SIZET " remain",
ssl->in_msglen,
hs_remain,
ssl->in_hslen));
return MBEDTLS_ERR_SSL_INVALID_RECORD;
}
} else if (ssl->in_msglen == ssl->in_hslen) {
/* This is the sole fragment. */
/* Emit a log message in the same format as when there are
* multiple fragments, for ease of matching. */
MBEDTLS_SSL_DEBUG_MSG(3,
("sole handshake fragment: %" MBEDTLS_PRINTF_SIZET
", %" MBEDTLS_PRINTF_SIZET
"..%" MBEDTLS_PRINTF_SIZET
" of %" MBEDTLS_PRINTF_SIZET,
ssl->in_msglen,
ssl->in_hsfraglen,
ssl->in_hsfraglen + ssl->in_msglen,
ssl->in_hslen));
} else {
/* This is the first fragment of many. */
MBEDTLS_SSL_DEBUG_MSG(3,
("initial handshake fragment: %" MBEDTLS_PRINTF_SIZET
", %" MBEDTLS_PRINTF_SIZET
"..%" MBEDTLS_PRINTF_SIZET
" of %" MBEDTLS_PRINTF_SIZET,
ssl->in_msglen,
ssl->in_hsfraglen,
ssl->in_hsfraglen + ssl->in_msglen,
ssl->in_hslen));
}

/* Move the received handshake fragment to have the whole message
* (at least the part received so far) in a single segment at a
* known offset in the input buffer.
* - When receiving a non-initial handshake fragment, append it to
* the initial segment.
* - Even the initial handshake fragment is moved, if it was
* encrypted with an explicit IV: decryption leaves the payload
* after the explicit IV, but here we move it to start where the
* IV was.
*/
#if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH)
size_t const in_buf_len = ssl->in_buf_len;
#else
size_t const in_buf_len = MBEDTLS_SSL_IN_BUFFER_LEN;
#endif
if (payload_end + ssl->in_hsfraglen > ssl->in_buf + in_buf_len) {
Copy link
Contributor

@minosgalanakis minosgalanakis Mar 4, 2025

Choose a reason for hiding this comment

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

I assume that when MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH is enabled, a buffer can only expand when resized?

Copy link
Contributor Author

@gilles-peskine-arm gilles-peskine-arm Mar 4, 2025

Choose a reason for hiding this comment

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

Good question. A buffer can shrink when resized. But resizing only happens at the beginning and at the end of the handshake.

In any case, I don't think that matters here? We're checking whether the data fits now. If there's code that wants to resize the buffer, it would have to determine whether the data that's already in the buffer would still fit. Which, currently, is easy: resizing is only done at times when there's no data in the buffer.

Copy link
Contributor

@mpg mpg Mar 6, 2025

Choose a reason for hiding this comment

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

I think you mean payload_end + ssl->in_msglen here, to protect the call to memmove() below.

MBEDTLS_SSL_DEBUG_MSG(1,
("Shouldn't happen: no room to move handshake fragment %"
MBEDTLS_PRINTF_SIZET " from %p to %p (buf=%p len=%"
MBEDTLS_PRINTF_SIZET ")",
ssl->in_msglen,
ssl->in_msg, payload_end,
ssl->in_buf, in_buf_len));
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
}
memmove(payload_end, ssl->in_msg, ssl->in_msglen);

ssl->in_hsfraglen += ssl->in_msglen;
payload_end += ssl->in_msglen;

if (ssl->in_hsfraglen < ssl->in_hslen) {
MBEDTLS_SSL_DEBUG_MSG(3, ("Prepare: waiting for more handshake fragments %"
MBEDTLS_PRINTF_SIZET "/%" MBEDTLS_PRINTF_SIZET,
MBEDTLS_PRINTF_SIZET "/%"
MBEDTLS_PRINTF_SIZET,
ssl->in_hsfraglen, ssl->in_hslen));
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 {
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;
ssl->in_hdr = payload_end;
ssl->in_msglen = 0;
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);
return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING;
} else {
ssl->in_msglen = ssl->in_hsfraglen;
ssl->in_hsfraglen = 0;
ssl->in_hdr = reassembled_record_start;
mbedtls_ssl_update_in_pointers(ssl);

/* Update the record length in the fully reassembled record */
if (ssl->in_msglen > 0xffff) {
MBEDTLS_SSL_DEBUG_MSG(1,
("Shouldn't happen: in_msglen=%"
MBEDTLS_PRINTF_SIZET " > 0xffff",
ssl->in_msglen));
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
}
MBEDTLS_PUT_UINT16_BE(ssl->in_msglen, ssl->in_len, 0);

MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record",
ssl->in_hdr, mbedtls_ssl_in_hdr_len(ssl) + merged_rec_len);
ssl->in_hdr,
mbedtls_ssl_in_hdr_len(ssl) + ssl->in_msglen);
}
} else {
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
}

return 0;
Expand Down
7 changes: 0 additions & 7 deletions tests/scripts/analyze_outcomes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,6 @@ def _has_word_re(words: typing.Iterable[str],
re.DOTALL)

IGNORED_TESTS = {
'handshake-generated': [
# Temporary disable Handshake defragmentation tests until mbedtls
# pr #10011 has been merged.
'Handshake defragmentation on client: len=4, TLS 1.2',
'Handshake defragmentation on client: len=5, TLS 1.2',
'Handshake defragmentation on client: len=13, TLS 1.2'
],
'ssl-opt': [
# We don't run ssl-opt.sh with Valgrind on the CI because
# it's extremely slow. We don't intend to change this.
Expand Down