From cc856a2c0ede76986155f01d0d69a041d39c128c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 22:24:56 +0100 Subject: [PATCH] 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 --- library/ssl_msg.c | 162 +++++++++++++++++++----------- tests/scripts/analyze_outcomes.py | 7 -- 2 files changed, 105 insertions(+), 64 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 9f9cda9d29..f80eed7fa4 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3308,70 +3308,118 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) } } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ - if (ssl->badmac_seen_or_in_hsfraglen <= ssl->in_hslen) { - int ret; - const size_t hs_remain = ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen; - MBEDTLS_SSL_DEBUG_MSG(3, - ("handshake fragment: %" MBEDTLS_PRINTF_SIZET - ", %u..%u of %" MBEDTLS_PRINTF_SIZET, - ssl->in_msglen, - ssl->badmac_seen_or_in_hsfraglen, - ssl->badmac_seen_or_in_hsfraglen + - (unsigned) ssl->in_msglen, - ssl->in_hslen)); - if (ssl->in_msglen < hs_remain) { - /* 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->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); + { + 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->badmac_seen_or_in_hsfraglen; + + if (ssl->badmac_seen_or_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 + ", %u..%u of %" MBEDTLS_PRINTF_SIZET, + ssl->in_msglen, + ssl->badmac_seen_or_in_hsfraglen, + ssl->badmac_seen_or_in_hsfraglen + + (unsigned) ssl->in_msglen, + ssl->in_hslen)); + + const size_t hs_remain = ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen; + if (ssl->in_msglen > hs_remain) { + 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 + ", %u..%u of %" MBEDTLS_PRINTF_SIZET, + ssl->in_msglen, + ssl->badmac_seen_or_in_hsfraglen, + ssl->badmac_seen_or_in_hsfraglen + + (unsigned) 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 + ", %u..%u of %" MBEDTLS_PRINTF_SIZET, + ssl->in_msglen, + ssl->badmac_seen_or_in_hsfraglen, + ssl->badmac_seen_or_in_hsfraglen + + (unsigned) 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->badmac_seen_or_in_hsfraglen > ssl->in_buf + in_buf_len) { + 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->badmac_seen_or_in_hsfraglen += ssl->in_msglen; + payload_end += ssl->in_msglen; + + if (ssl->badmac_seen_or_in_hsfraglen < ssl->in_hslen) { MBEDTLS_SSL_DEBUG_MSG(3, ("Prepare: waiting for more handshake fragments " "%u/%" MBEDTLS_PRINTF_SIZET, ssl->badmac_seen_or_in_hsfraglen, ssl->in_hslen)); - return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; - } - 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. - * 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->badmac_seen_or_in_hsfraglen; ssl->badmac_seen_or_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; diff --git a/tests/scripts/analyze_outcomes.py b/tests/scripts/analyze_outcomes.py index 7704170fe2..301bfc403d 100755 --- a/tests/scripts/analyze_outcomes.py +++ b/tests/scripts/analyze_outcomes.py @@ -34,13 +34,6 @@ class CoverageTask(outcome_analysis.CoverageTask): 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.