Skip to content

Commit 64645a6

Browse files
gilles-peskine-armgilles-peskine-arm
committed
Handshake defragmentation: reassemble incrementally
Reassemble handshake fragments incrementally instead of all at the end. That is, every time we receive a non-initial handshake fragment, append it to the initial fragment. Since we only have to deal with at most two handshake fragments at the same time, this simplifies the code (no re-parsing of a record) and is a little more memory-efficient (no need to store one record header per record). This commit also fixes a bug. The previous code did not calculate offsets correctly when records use an explicit IV, which is the case in TLS 1.2 with CBC (encrypt-then-MAC or not), GCM and CCM encryption (i.e. all but null and ChachaPoly). This led to the wrong data when an encrypted handshake message was fragmented (Finished or renegotiation). The new code handles this correctly. Signed-off-by: Gilles Peskine <[email protected]>
1 parent 3034ba4 commit 64645a6

File tree

2 files changed

+105
-59
lines changed

2 files changed

+105
-59
lines changed

‎library/ssl_msg.c‎

Lines changed: 105 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -3049,64 +3049,117 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl)
30493049
}
30503050
} else
30513051
#endif /* MBEDTLS_SSL_PROTO_DTLS */
3052-
if (ssl->in_hsfraglen <= ssl->in_hslen) {
3053-
int ret;
3054-
const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen;
3055-
MBEDTLS_SSL_DEBUG_MSG(3,
3056-
("handshake fragment: %" MBEDTLS_PRINTF_SIZET
3057-
", %" MBEDTLS_PRINTF_SIZET
3058-
"..%" MBEDTLS_PRINTF_SIZET
3059-
" of %" MBEDTLS_PRINTF_SIZET,
3060-
ssl->in_msglen,
3061-
ssl->in_hsfraglen,
3062-
ssl->in_hsfraglen + ssl->in_msglen,
3063-
ssl->in_hslen));
3064-
if (ssl->in_msglen < hs_remain) {
3065-
ssl->in_hsfraglen += ssl->in_msglen;
3066-
ssl->in_hdr = ssl->in_msg + ssl->in_msglen;
3067-
ssl->in_msglen = 0;
3068-
mbedtls_ssl_update_in_pointers(ssl);
3052+
{
3053+
unsigned char *const reassembled_record_start =
3054+
ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN;
3055+
unsigned char *const payload_start =
3056+
reassembled_record_start + mbedtls_ssl_in_hdr_len(ssl);
3057+
unsigned char *payload_end = payload_start + ssl->in_hsfraglen;
3058+
3059+
if (ssl->in_hsfraglen != 0) {
3060+
/* We already had a handshake fragment. Prepare to append
3061+
* to the initial segment. */
3062+
MBEDTLS_SSL_DEBUG_MSG(3,
3063+
("subsequent handshake fragment: %" MBEDTLS_PRINTF_SIZET
3064+
", %" MBEDTLS_PRINTF_SIZET
3065+
"..%" MBEDTLS_PRINTF_SIZET
3066+
" of %" MBEDTLS_PRINTF_SIZET,
3067+
ssl->in_msglen,
3068+
ssl->in_hsfraglen,
3069+
ssl->in_hsfraglen + ssl->in_msglen,
3070+
ssl->in_hslen));
3071+
3072+
const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen;
3073+
if (ssl->in_msglen > hs_remain) {
3074+
MBEDTLS_SSL_DEBUG_MSG(1, ("Handshake fragment too long: %"
3075+
MBEDTLS_PRINTF_SIZET " but only %"
3076+
MBEDTLS_PRINTF_SIZET " of %"
3077+
MBEDTLS_PRINTF_SIZET " remain",
3078+
ssl->in_msglen,
3079+
hs_remain,
3080+
ssl->in_hslen));
3081+
return MBEDTLS_ERR_SSL_INVALID_RECORD;
3082+
}
3083+
} else if (ssl->in_msglen == ssl->in_hslen) {
3084+
/* This is the sole fragment. */
3085+
/* Emit a log message in the same format as when there are
3086+
* multiple fragments, for ease of matching. */
3087+
MBEDTLS_SSL_DEBUG_MSG(3,
3088+
("sole handshake fragment: %" MBEDTLS_PRINTF_SIZET
3089+
", %" MBEDTLS_PRINTF_SIZET
3090+
"..%" MBEDTLS_PRINTF_SIZET
3091+
" of %" MBEDTLS_PRINTF_SIZET,
3092+
ssl->in_msglen,
3093+
ssl->in_hsfraglen,
3094+
ssl->in_hsfraglen + ssl->in_msglen,
3095+
ssl->in_hslen));
3096+
} else {
3097+
/* This is the first fragment of many. */
3098+
MBEDTLS_SSL_DEBUG_MSG(3,
3099+
("initial handshake fragment: %" MBEDTLS_PRINTF_SIZET
3100+
", %" MBEDTLS_PRINTF_SIZET
3101+
"..%" MBEDTLS_PRINTF_SIZET
3102+
" of %" MBEDTLS_PRINTF_SIZET,
3103+
ssl->in_msglen,
3104+
ssl->in_hsfraglen,
3105+
ssl->in_hsfraglen + ssl->in_msglen,
3106+
ssl->in_hslen));
3107+
}
3108+
3109+
/* Move the received handshake fragment to have the whole message
3110+
* (at least the part received so far) in a single segment at a
3111+
* known offset in the input buffer.
3112+
* - When receiving a non-initial handshake fragment, append it to
3113+
* the initial segment.
3114+
* - Even the initial handshake fragment is moved, if it was
3115+
* encrypted with an explicit IV: decryption leaves the payload
3116+
* after the explicit IV, but here we move it to start where the
3117+
* IV was.
3118+
*/
3119+
if (payload_end + ssl->in_hsfraglen > ssl->in_buf + ssl->in_buf_len) {
3120+
MBEDTLS_SSL_DEBUG_MSG(1,
3121+
("Shouldn't happen: no room to move handshake fragment %"
3122+
MBEDTLS_PRINTF_SIZET " from %p to %p (buf=%p len=%"
3123+
MBEDTLS_PRINTF_SIZET ")",
3124+
ssl->in_msglen,
3125+
ssl->in_msg, payload_end,
3126+
ssl->in_buf, ssl->in_buf_len));
3127+
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
3128+
}
3129+
memmove(payload_end, ssl->in_msg, ssl->in_msglen);
3130+
3131+
ssl->in_hsfraglen += ssl->in_msglen;
3132+
payload_end += ssl->in_msglen;
3133+
3134+
if (ssl->in_hsfraglen < ssl->in_hslen) {
30693135
MBEDTLS_SSL_DEBUG_MSG(3, ("Prepare: waiting for more handshake fragments %"
3070-
MBEDTLS_PRINTF_SIZET "/%" MBEDTLS_PRINTF_SIZET,
3136+
MBEDTLS_PRINTF_SIZET "/%"
3137+
MBEDTLS_PRINTF_SIZET,
30713138
ssl->in_hsfraglen, ssl->in_hslen));
3072-
return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING;
3073-
}
3074-
if (ssl->in_hsfraglen > 0) {
3075-
/*
3076-
* At in_first_hdr we have a sequence of records that cover the next handshake
3077-
* record, each with its own record header that we need to remove.
3078-
* Note that the reassembled record size may not equal the size of the message,
3079-
* there may be more messages after it, complete or partial.
3080-
*/
3081-
unsigned char *in_first_hdr = ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN;
3082-
unsigned char *p = in_first_hdr, *q = NULL;
3083-
size_t merged_rec_len = 0;
3084-
do {
3085-
mbedtls_record rec;
3086-
ret = ssl_parse_record_header(ssl, p, mbedtls_ssl_in_hdr_len(ssl), &rec);
3087-
if (ret != 0) {
3088-
return ret;
3089-
}
3090-
merged_rec_len += rec.data_len;
3091-
p = rec.buf + rec.buf_len;
3092-
if (q != NULL) {
3093-
memmove(q, rec.buf + rec.data_offset, rec.data_len);
3094-
q += rec.data_len;
3095-
} else {
3096-
q = p;
3097-
}
3098-
} while (merged_rec_len < ssl->in_hslen);
3099-
ssl->in_hdr = in_first_hdr;
3139+
ssl->in_hdr = payload_end;
3140+
ssl->in_msglen = 0;
31003141
mbedtls_ssl_update_in_pointers(ssl);
3101-
ssl->in_msglen = merged_rec_len;
3102-
/* Adjust message length. */
3103-
MBEDTLS_PUT_UINT16_BE(merged_rec_len, ssl->in_len, 0);
3142+
return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING;
3143+
} else {
3144+
ssl->in_msglen = ssl->in_hsfraglen;
31043145
ssl->in_hsfraglen = 0;
3146+
ssl->in_hdr = reassembled_record_start;
3147+
mbedtls_ssl_update_in_pointers(ssl);
3148+
3149+
/* Update the record length in the fully reassembled record */
3150+
if (ssl->in_msglen > 0xffff) {
3151+
MBEDTLS_SSL_DEBUG_MSG(1,
3152+
("Shouldn't happen: in_msglen=%"
3153+
MBEDTLS_PRINTF_SIZET " > 0xffff",
3154+
ssl->in_msglen));
3155+
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
3156+
}
3157+
MBEDTLS_PUT_UINT16_BE(ssl->in_msglen, ssl->in_len, 0);
3158+
31053159
MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record",
3106-
ssl->in_hdr, mbedtls_ssl_in_hdr_len(ssl) + merged_rec_len);
3160+
ssl->in_hdr,
3161+
mbedtls_ssl_in_hdr_len(ssl) + ssl->in_msglen);
31073162
}
3108-
} else {
3109-
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
31103163
}
31113164

31123165
return 0;

‎tests/scripts/analyze_outcomes.py‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,6 @@ def _has_word_re(words: typing.Iterable[str],
3434
re.DOTALL)
3535

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

0 commit comments

Comments
 (0)