From b6102b6ccf0fe50577d563d0e87134b0ce4d6b15 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Feb 2025 23:11:09 +0100 Subject: [PATCH 01/15] Fix Doxygen markup Pacify `clang -Wdocumentation`. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_test_lib.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index 961433357..d7fe80f83 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -243,8 +243,8 @@ int key_opaque_set_alg_usage(const char *alg1, const char *alg2, * - free the provided PK context and re-initilize it as an opaque PK context * wrapping the PSA key imported in the above step. * - * \param[in/out] pk On input the non-opaque PK context which contains the - * key to be wrapped. On output the re-initialized PK + * \param[in,out] pk On input, the non-opaque PK context which contains the + * key to be wrapped. On output, the re-initialized PK * context which represents the opaque version of the one * provided as input. * \param[in] psa_alg The primary algorithm that will be associated to the From 3d490a91adadfad9a9d68889f5d9a4a972a2c7a6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 21:29:59 +0100 Subject: [PATCH 02/15] Tweak "waiting for more handshake fragments" log message In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak the "waiting for more handshake fragments" log message in ssl_consume_current_message(), and add a similar one in mbedtls_ssl_prepare_handshake_record(). Assert both. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 7b11e4d06..c2da065fa 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3320,6 +3320,9 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) ssl->in_hdr = ssl->in_msg + ssl->in_msglen; ssl->in_msglen = 0; mbedtls_ssl_update_in_pointers(ssl); + 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) { @@ -4704,11 +4707,9 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) if (ssl->badmac_seen_or_in_hsfraglen != 0) { /* Not all handshake fragments have arrived, do not consume. */ - MBEDTLS_SSL_DEBUG_MSG(3, - ("waiting for more fragments (%u of %" - MBEDTLS_PRINTF_SIZET ", %" MBEDTLS_PRINTF_SIZET " left)", - ssl->badmac_seen_or_in_hsfraglen, ssl->in_hslen, - ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen)); + MBEDTLS_SSL_DEBUG_MSG(3, ("Consume: waiting for more handshake fragments " + "%u/%" MBEDTLS_PRINTF_SIZET, + ssl->badmac_seen_or_in_hsfraglen, ssl->in_hslen)); return 0; } From 1e81d349b8e5ca24d8b9fad24a2da573f02ac058 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Mar 2025 16:46:10 +0100 Subject: [PATCH 03/15] Tweak handshake fragment log message In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak the "handshake fragment:" log message. This changes what information is displayed when a record contains data beyond the expected end of the handshake message. This case is currently untested and its handling will change in a subsequent commit. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index c2da065fa..1c642fe96 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3301,13 +3301,13 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) int ret; const size_t hs_remain = ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen; MBEDTLS_SSL_DEBUG_MSG(3, - ("handshake fragment: %u .. %" - MBEDTLS_PRINTF_SIZET " of %" - MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET, + ("handshake fragment: %" MBEDTLS_PRINTF_SIZET + ", %u..%u of %" MBEDTLS_PRINTF_SIZET, + ssl->in_msglen, ssl->badmac_seen_or_in_hsfraglen, - (size_t) ssl->badmac_seen_or_in_hsfraglen + - (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen), - ssl->in_hslen, ssl->in_msglen)); + 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 From af0c461f3969a8c51c29cd48b83197250a120c60 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 21:59:12 +0100 Subject: [PATCH 04/15] mbedtls_ssl_prepare_handshake_record(): refactor first fragment prep Minor refactoring of the initial checks and preparation when receiving the first fragment. Use `ssl->in_hsfraglen` to determine whether there is a pending handshake fragment, for consistency, and possibly for more robustness in case handshake fragments are mixed with non-handshake records (although this is not currently supported anyway). Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 1c642fe96..e68474d45 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3221,16 +3221,19 @@ static uint32_t ssl_get_hs_total_len(mbedtls_ssl_context const *ssl) int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *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; - } + if (ssl->badmac_seen_or_in_hsfraglen == 0) { + /* The handshake message must at least include the header. + * We may not have the full message yet in case of fragmentation. + * To simplify the code, we insist on having the header (and in + * particular the handshake message length) in the first + * fragment. */ + if (ssl->in_msglen < mbedtls_ssl_hs_hdr_len(ssl)) { + MBEDTLS_SSL_DEBUG_MSG(1, ("handshake message too short: %" MBEDTLS_PRINTF_SIZET, + ssl->in_msglen)); + return MBEDTLS_ERR_SSL_INVALID_RECORD; + } - if (ssl->in_hslen == 0) { ssl->in_hslen = mbedtls_ssl_hs_hdr_len(ssl) + ssl_get_hs_total_len(ssl); - ssl->badmac_seen_or_in_hsfraglen = 0; } MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen =" From 22c51b9a0bfbd85a81bb21cc58f51afd80157244 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 22:02:52 +0100 Subject: [PATCH 05/15] mbedtls_ssl_prepare_handshake_record(): log offsets after decryption Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index e68474d45..9f9cda9d2 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3241,6 +3241,14 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) MBEDTLS_PRINTF_SIZET, ssl->in_msglen, ssl->in_msg[0], ssl->in_hslen)); + if (ssl->transform_in != NULL) { + MBEDTLS_SSL_DEBUG_MSG(4, ("decrypted handshake message:" + " iv-buf=%d hdr-buf=%d hdr-buf=%d", + (int) (ssl->in_iv - ssl->in_buf), + (int) (ssl->in_hdr - ssl->in_buf), + (int) (ssl->in_msg - ssl->in_buf))); + } + #if defined(MBEDTLS_SSL_PROTO_DTLS) if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; From cc856a2c0ede76986155f01d0d69a041d39c128c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 22:24:56 +0100 Subject: [PATCH 06/15] 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 9f9cda9d2..f80eed7fa 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 7704170fe..301bfc403 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. From 302f37b05d4950494d8409b5dd0e275d1e76058c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Feb 2025 23:57:20 +0100 Subject: [PATCH 07/15] Fix dodgy printf calls Pacify `clang -Wformat-pedantic`. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index f80eed7fa..823b0ccf1 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3383,8 +3383,8 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) 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)); + (void *) ssl->in_msg, (void *) payload_end, + (void *) ssl->in_buf, in_buf_len)); return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; } memmove(payload_end, ssl->in_msg, ssl->in_msglen); From 58c3301f65e078c3080ec6fe9aaf025a560ab6d7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 4 Mar 2025 10:30:24 +0100 Subject: [PATCH 08/15] Make conversion explicit to silence MSVC warning Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 823b0ccf1..436870b51 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3389,7 +3389,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) } memmove(payload_end, ssl->in_msg, ssl->in_msglen); - ssl->badmac_seen_or_in_hsfraglen += ssl->in_msglen; + ssl->badmac_seen_or_in_hsfraglen += (unsigned) ssl->in_msglen; payload_end += ssl->in_msglen; if (ssl->badmac_seen_or_in_hsfraglen < ssl->in_hslen) { From 7719169ef4d73d18a8e3d4247881dec108c6a324 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Mar 2025 17:53:53 +0100 Subject: [PATCH 09/15] Update framework Changed log messages and added more tests in `tests/opt-testcases/handshake-generated.sh`. Signed-off-by: Gilles Peskine --- framework | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework b/framework index 4a009d4b3..8d85112a4 160000 --- a/framework +++ b/framework @@ -1 +1 @@ -Subproject commit 4a009d4b3cf6c55a558d90c92c1aa2d1ea2bb99b +Subproject commit 8d85112a44d052a5d89cb0a135e162384da42584 From 3175fc3be239c9029b32ec9ef87eff7763284890 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 15:15:20 +0100 Subject: [PATCH 10/15] Fix end check before memmove Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 436870b51..1dca728a0 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3377,7 +3377,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) #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) { + if (payload_end + ssl->in_msglen > 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=%" From b888cca5b658c2ed95ddd2dbf4fc7c0134ee0e90 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 19:03:00 +0100 Subject: [PATCH 11/15] Fix handshake defragmentation when the record has multiple messages A handshake record may contain multiple handshake messages, or multiple fragments (there can be the final fragment of a pending message, then zero or more whole messages, and an initial fragment of an incomplete message). This was previously untested, but supported, so don't break it. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 1dca728a0..6cd0f4137 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3314,6 +3314,15 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) 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; + /* How many more bytes we want to have a complete handshake message. */ + const size_t hs_remain = ssl->in_hslen - ssl->badmac_seen_or_in_hsfraglen; + /* How many bytes of the current record are part of the first + * handshake message. There may be more handshake messages (possibly + * incomplete) in the same record; if so, we leave them after the + * current record, and ssl_consume_current_message() will take + * care of consuming the next handshake message. */ + const size_t hs_this_fragment_len = + ssl->in_msglen > hs_remain ? hs_remain : ssl->in_msglen; if (ssl->badmac_seen_or_in_hsfraglen != 0) { /* We already had a handshake fragment. Prepare to append @@ -3324,21 +3333,9 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) ssl->in_msglen, ssl->badmac_seen_or_in_hsfraglen, ssl->badmac_seen_or_in_hsfraglen + - (unsigned) ssl->in_msglen, + (unsigned) hs_this_fragment_len, 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) { + } else if (hs_this_fragment_len == 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. */ @@ -3348,7 +3345,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) ssl->in_msglen, ssl->badmac_seen_or_in_hsfraglen, ssl->badmac_seen_or_in_hsfraglen + - (unsigned) ssl->in_msglen, + (unsigned) hs_this_fragment_len, ssl->in_hslen)); } else { /* This is the first fragment of many. */ @@ -3358,7 +3355,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) ssl->in_msglen, ssl->badmac_seen_or_in_hsfraglen, ssl->badmac_seen_or_in_hsfraglen + - (unsigned) ssl->in_msglen, + (unsigned) hs_this_fragment_len, ssl->in_hslen)); } @@ -3409,16 +3406,24 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *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=%" + ("Shouldn't happen: in_hslen=%" MBEDTLS_PRINTF_SIZET " > 0xffff", ssl->in_msglen)); return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; } MBEDTLS_PUT_UINT16_BE(ssl->in_msglen, ssl->in_len, 0); + size_t record_len = mbedtls_ssl_in_hdr_len(ssl) + ssl->in_msglen; MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record", - ssl->in_hdr, - mbedtls_ssl_in_hdr_len(ssl) + ssl->in_msglen); + ssl->in_hdr, record_len); + if (ssl->in_hslen < ssl->in_msglen) { + MBEDTLS_SSL_DEBUG_MSG(3, + ("More handshake messages in the record: " + "%" MBEDTLS_PRINTF_SIZET " + " + "%" MBEDTLS_PRINTF_SIZET, + ssl->in_hslen, + ssl->in_msglen - ssl->in_hslen)); + } } } From 0a467ccd2435bbf1e67a977e92a7992183a7a3d3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 19:22:52 +0100 Subject: [PATCH 12/15] Unify handshake fragment log messages There is no longer any different processing at this point, just near-identical log messages. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 47 +++++++++++++---------------------------------- 1 file changed, 13 insertions(+), 34 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 6cd0f4137..b63533d19 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3324,40 +3324,19 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) const size_t hs_this_fragment_len = ssl->in_msglen > hs_remain ? hs_remain : ssl->in_msglen; - 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) hs_this_fragment_len, - ssl->in_hslen)); - } else if (hs_this_fragment_len == 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) hs_this_fragment_len, - 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) hs_this_fragment_len, - ssl->in_hslen)); - } + MBEDTLS_SSL_DEBUG_MSG(3, + ("%s handshake fragment: %" MBEDTLS_PRINTF_SIZET + ", %u..%u of %" MBEDTLS_PRINTF_SIZET, + (ssl->badmac_seen_or_in_hsfraglen != 0 ? + "subsequent" : + hs_this_fragment_len == ssl->in_hslen ? + "sole" : + "initial"), + ssl->in_msglen, + ssl->badmac_seen_or_in_hsfraglen, + ssl->badmac_seen_or_in_hsfraglen + + (unsigned) hs_this_fragment_len, + 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 From dee926359ca8d351c97e0a9086aea244fda4f64c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 21:32:08 +0100 Subject: [PATCH 13/15] Pacify uncrustify Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index b63533d19..fd3cb2f1d 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3398,8 +3398,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) if (ssl->in_hslen < ssl->in_msglen) { MBEDTLS_SSL_DEBUG_MSG(3, ("More handshake messages in the record: " - "%" MBEDTLS_PRINTF_SIZET " + " - "%" MBEDTLS_PRINTF_SIZET, + "%" MBEDTLS_PRINTF_SIZET " + %" MBEDTLS_PRINTF_SIZET, ssl->in_hslen, ssl->in_msglen - ssl->in_hslen)); } From 229e200cb4cbbfdc449f711301201c5cf067f161 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 21:30:23 +0100 Subject: [PATCH 14/15] Note unused variables when debugging is disabled Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index fd3cb2f1d..ef842b06b 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3323,6 +3323,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) * care of consuming the next handshake message. */ const size_t hs_this_fragment_len = ssl->in_msglen > hs_remain ? hs_remain : ssl->in_msglen; + (void) hs_this_fragment_len; MBEDTLS_SSL_DEBUG_MSG(3, ("%s handshake fragment: %" MBEDTLS_PRINTF_SIZET @@ -3393,6 +3394,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) MBEDTLS_PUT_UINT16_BE(ssl->in_msglen, ssl->in_len, 0); size_t record_len = mbedtls_ssl_in_hdr_len(ssl) + ssl->in_msglen; + (void) record_len; MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record", ssl->in_hdr, record_len); if (ssl->in_hslen < ssl->in_msglen) { From c22e31508677fed2cd41af7bb899a1ef8953f233 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 7 Mar 2025 10:43:39 +0100 Subject: [PATCH 15/15] Fix a log message Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index ef842b06b..0a8f4a3c6 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3386,7 +3386,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) /* Update the record length in the fully reassembled record */ if (ssl->in_msglen > 0xffff) { MBEDTLS_SSL_DEBUG_MSG(1, - ("Shouldn't happen: in_hslen=%" + ("Shouldn't happen: in_msglen=%" MBEDTLS_PRINTF_SIZET " > 0xffff", ssl->in_msglen)); return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;