From 2e5a7ea9bc4301745c0234225b18218f4af3edc3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Feb 2025 23:11:09 +0100 Subject: [PATCH 01/14] 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 6fc3d730724b..bc5cce51a050 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 9bdc8aa80b3d8df7286273cf2710e1d658d147c5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 21:29:59 +0100 Subject: [PATCH 02/14] 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 a87785cfea22..a9310aa976f3 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3054,6 +3054,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 %" + MBEDTLS_PRINTF_SIZET "/%" MBEDTLS_PRINTF_SIZET, + ssl->in_hsfraglen, ssl->in_hslen)); return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; } if (ssl->in_hsfraglen > 0) { @@ -4438,11 +4441,9 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) 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)); + MBEDTLS_SSL_DEBUG_MSG(3, ("Consume: waiting for more handshake fragments %" + MBEDTLS_PRINTF_SIZET "/%" MBEDTLS_PRINTF_SIZET, + ssl->in_hsfraglen, ssl->in_hslen)); return 0; } From 07027722cbe091f2fe8f446e4805dcc93f604bda Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Mar 2025 16:46:10 +0100 Subject: [PATCH 03/14] 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 | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index a9310aa976f3..12d46c305ace 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3042,13 +3042,14 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) 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 " of %" - MBEDTLS_PRINTF_SIZET " msglen %" MBEDTLS_PRINTF_SIZET, + ("handshake fragment: %" MBEDTLS_PRINTF_SIZET + ", %" MBEDTLS_PRINTF_SIZET + "..%" MBEDTLS_PRINTF_SIZET + " of %" MBEDTLS_PRINTF_SIZET, + ssl->in_msglen, ssl->in_hsfraglen, - ssl->in_hsfraglen + - (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen), - ssl->in_hslen, ssl->in_msglen)); + 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; From 7a17696c3414d10970fedc135dac7f8bcdf893a6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 21:59:12 +0100 Subject: [PATCH 04/14] 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 12d46c305ace..a8c79172fcfe 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -2962,16 +2962,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->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->in_hsfraglen = 0; } MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen =" From 235eae9e0381c889b0d011971fe2c8123652a073 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 22:02:52 +0100 Subject: [PATCH 05/14] 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 a8c79172fcfe..cba6096eb453 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -2982,6 +2982,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 e85ece6584d9fae3a3f3661619d0223e6482acff Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 28 Feb 2025 22:24:56 +0100 Subject: [PATCH 06/14] 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, 110 insertions(+), 59 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index cba6096eb453..454b1ebbdd03 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -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) { + 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) { + 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; diff --git a/tests/scripts/analyze_outcomes.py b/tests/scripts/analyze_outcomes.py index 39460176253a..e68c2cbf09f3 100755 --- a/tests/scripts/analyze_outcomes.py +++ b/tests/scripts/analyze_outcomes.py @@ -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. From 90a9593bbd3ec343e16d73cbaf998f8e5768a9b6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Feb 2025 23:57:20 +0100 Subject: [PATCH 07/14] 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 454b1ebbdd03..9d8857dfa6c2 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3127,8 +3127,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 e4a3fc2f5818729fc13739448c9518f41094d627 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Mar 2025 17:53:53 +0100 Subject: [PATCH 08/14] 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 4a009d4b3cf6..8d85112a44d0 160000 --- a/framework +++ b/framework @@ -1 +1 @@ -Subproject commit 4a009d4b3cf6c55a558d90c92c1aa2d1ea2bb99b +Subproject commit 8d85112a44d052a5d89cb0a135e162384da42584 From 0851ec93444b55b1dbb0db7cec3c4845f9cefcd3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 15:15:20 +0100 Subject: [PATCH 09/14] 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 9d8857dfa6c2..ad3bf575925a 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3121,7 +3121,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->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 15c072f0de4555c4810acec14e074c01ddf871de Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 19:03:00 +0100 Subject: [PATCH 10/14] 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 ad3bf575925a..acd05b0382fe 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3055,6 +3055,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->in_hsfraglen; + /* How many more bytes we want to have a complete handshake message. */ + const size_t hs_remain = ssl->in_hslen - ssl->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->in_hsfraglen != 0) { /* We already had a handshake fragment. Prepare to append @@ -3066,21 +3075,9 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) " of %" MBEDTLS_PRINTF_SIZET, ssl->in_msglen, ssl->in_hsfraglen, - ssl->in_hsfraglen + ssl->in_msglen, + ssl->in_hsfraglen + hs_this_fragment_len, ssl->in_hslen)); - - const size_t hs_remain = ssl->in_hslen - ssl->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. */ @@ -3091,7 +3088,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) " of %" MBEDTLS_PRINTF_SIZET, ssl->in_msglen, ssl->in_hsfraglen, - ssl->in_hsfraglen + ssl->in_msglen, + ssl->in_hsfraglen + hs_this_fragment_len, ssl->in_hslen)); } else { /* This is the first fragment of many. */ @@ -3102,7 +3099,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) " of %" MBEDTLS_PRINTF_SIZET, ssl->in_msglen, ssl->in_hsfraglen, - ssl->in_hsfraglen + ssl->in_msglen, + ssl->in_hsfraglen + hs_this_fragment_len, ssl->in_hslen)); } @@ -3154,16 +3151,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 afb254c5fed409cf3811e468aae82a72198f38ae Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 19:23:22 +0100 Subject: [PATCH 11/14] 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 | 51 +++++++++++++---------------------------------- 1 file changed, 14 insertions(+), 37 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index acd05b0382fe..851c0df3944c 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3065,43 +3065,20 @@ 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->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 + 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 - ", %" MBEDTLS_PRINTF_SIZET - "..%" MBEDTLS_PRINTF_SIZET - " of %" MBEDTLS_PRINTF_SIZET, - ssl->in_msglen, - ssl->in_hsfraglen, - ssl->in_hsfraglen + 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 - ", %" MBEDTLS_PRINTF_SIZET - "..%" MBEDTLS_PRINTF_SIZET - " of %" MBEDTLS_PRINTF_SIZET, - ssl->in_msglen, - ssl->in_hsfraglen, - ssl->in_hsfraglen + hs_this_fragment_len, - ssl->in_hslen)); - } + MBEDTLS_SSL_DEBUG_MSG(3, + ("%s handshake fragment: %" MBEDTLS_PRINTF_SIZET + ", %" MBEDTLS_PRINTF_SIZET + "..%" MBEDTLS_PRINTF_SIZET + " of %" MBEDTLS_PRINTF_SIZET, + (ssl->in_hsfraglen != 0 ? + "subsequent" : + hs_this_fragment_len == ssl->in_hslen ? + "sole" : + "initial"), + ssl->in_msglen, + ssl->in_hsfraglen, + ssl->in_hsfraglen + 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 b8f1e4bae3fa26743ee3dfe43f22c2425dbb2db9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 21:32:08 +0100 Subject: [PATCH 12/14] 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 851c0df3944c..3c7ff8279fb2 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3141,8 +3141,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 dab1cb5b4515c683e0016b381afc0b1bd30b797b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Mar 2025 21:30:23 +0100 Subject: [PATCH 13/14] 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 3c7ff8279fb2..cc133be273cb 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3064,6 +3064,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 @@ -3136,6 +3137,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 e34ec86370b340bc845a91ea0b08e016c84c9d92 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 7 Mar 2025 10:43:39 +0100 Subject: [PATCH 14/14] 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 cc133be273cb..d91e8300a67a 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3129,7 +3129,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;