From 24c2875cb0eb655fffe7b6a7a956f34bacb7b0a4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 10 May 2026 14:55:51 +0200 Subject: [PATCH] Fix confusion between ssl->badmac_seen and ssl->in_hsfraglen Since Mbed TLS 3.6.3, the field `badmac_seen_or_in_hsfraglen` in the `mbedtls_ssl_context` structure has a dual purpose: it is the new name of `badmac_seen` for DTLS, and it has a new meaning `in_hsfraglen` for TLS. This was done to add support for handshake message fragmentation in TLS while preserving the library ABI. There was a bug in some places in the code, which reads the dual-purpose field in the wrong protocol. Specifically: * `mbedtls_ssl_prepare_handshake_record()` (beginning): mistakenly reads `badmac_seen_or_in_hsfraglen` in DTLS to mean `in_hsfraglen`. (DTLS handshake defragmentation does not use `in_hsfraglen`.) As a consequence, in DTLS, after receiving a record with a bad MAC, the field `ssl->in_hslen` was not correctly updated, which causes problems further in handshake message processing. * `mbedtls_ssl_prepare_handshake_record()` (end): correctly updates `badmac_seen_or_in_hsfraglen` in TLS with the `in_hsfraglen` meaning. * `ssl_consume_current_message()`: incorrectly read `badmac_seen_or_in_hsfraglen` in DTLS to mean `in_hsfraglen`, and thus stops processing messages after receiving a record with a bad MAC. * `ssl_get_next_record()`: accesses `badmac_seen_or_in_hsfraglen` to mean `badmac_seen` in DTLS. No bug here. * `mbedtls_ssl_handle_message_type()`: reads `badmac_seen_or_in_hsfraglen` to mean `in_hsfraglen` in TLS. No bug here. * `mbedtls_ssl_session_reset_msg_layer()`: both fields need to be reset when not partial, and a partial reset only applies to DTLS where we do want to preserve `badmac_seen` in a partial reset. No bug here. * `mbedtls_ssl_context_save()`, `mbedtls_ssl_context_load()`: saving `badmac_seen` in DTLS is desired and done. Saving `in_hsfraglen` in TLS is harmless. To fix this, in `ssl_msg.c`, always access the dual-purpose field `badmac_seen_or_in_hsfraglen` through getters and setters that are specific to one purpose and that check that the purpose matches the protocol. Signed-off-by: Gilles Peskine --- library/ssl_misc.h | 63 ++++++++++++++++++++++++++++++++++++++++++++++ library/ssl_msg.c | 44 ++++++++++++++++---------------- 2 files changed, 86 insertions(+), 21 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 218a30d408..2f509f32e9 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -484,6 +484,69 @@ static inline size_t mbedtls_ssl_get_input_buflen(const mbedtls_ssl_context *ctx } #endif +static inline unsigned mbedtls_ssl_get_badmac_seen(const mbedtls_ssl_context *ssl) +{ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { + return ssl->badmac_seen_or_in_hsfraglen; + } +#endif + (void) ssl; + return 0; +} + +static inline void mbedtls_ssl_set_badmac_seen(mbedtls_ssl_context *ssl, + unsigned badmac_seen) +{ + ssl->badmac_seen_or_in_hsfraglen = badmac_seen; +} + +#if defined(MBEDTLS_SSL_PROTO_DTLS) +#define MBEDTLS_SSL_SET_BADMAC_SEEN(ssl, badmac_seen) \ + do { \ + if ((ssl)->conf->transport != MBEDTLS_SSL_TRANSPORT_DATAGRAM) { \ + MBEDTLS_SSL_DEBUG_RET(1, ("Internal error: trying to set badmac_seen in TLS"), \ + MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED); \ + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; \ + } \ + mbedtls_ssl_set_badmac_seen(ssl, badmac_seen); \ + } while (0) +#else +#define MBEDTLS_SSL_SET_BADMAC_SEEN(ssl, badmac_seen) \ + mbedtls_ssl_set_badmac_seen(ssl, badmac_seen) +#endif + +static inline unsigned mbedtls_ssl_get_in_hsfraglen(const mbedtls_ssl_context *ssl) +{ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { + return 0; + } +#endif + return ssl->badmac_seen_or_in_hsfraglen; +} + +static inline void mbedtls_ssl_set_in_hsfraglen(mbedtls_ssl_context *ssl, + unsigned in_hsfraglen) +{ + ssl->badmac_seen_or_in_hsfraglen = in_hsfraglen; +} + +#if defined(MBEDTLS_SSL_PROTO_DTLS) +#define MBEDTLS_SSL_SET_IN_HSFRAGLEN(ssl, in_hsfraglen) \ + do { \ + if ((ssl)->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { \ + MBEDTLS_SSL_DEBUG_RET(1, ("Internal error: trying to set in_hsfraglen in DTLS"), \ + MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED); \ + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; \ + } \ + mbedtls_ssl_set_in_hsfraglen(ssl, in_hsfraglen); \ + } while (0) +#else +#define MBEDTLS_SSL_SET_IN_HSFRAGLEN(ssl, in_hsfraglen) \ + mbedtls_ssl_set_in_hsfraglen(ssl, in_hsfraglen) +#endif + /* * TLS extension flags (for extensions with outgoing ServerHello content * that need it (e.g. for RENEGOTIATION_INFO the server already knows because diff --git a/library/ssl_msg.c b/library/ssl_msg.c index b0ab608350..be4406b1a2 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3224,7 +3224,10 @@ static uint32_t ssl_get_hs_total_len(mbedtls_ssl_context const *ssl) int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) { - if (ssl->badmac_seen_or_in_hsfraglen == 0) { + /* Set handshake record length if this is the first fragment. + * Do it unconditionally for DTLS, for which handshake fragmentation + * is handled completely differently. */ + if (mbedtls_ssl_get_in_hsfraglen(ssl) == 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 @@ -3364,9 +3367,10 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) 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; + unsigned in_hsfraglen = mbedtls_ssl_get_in_hsfraglen(ssl); + unsigned char *payload_end = payload_start + 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; + const size_t hs_remain = ssl->in_hslen - 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 @@ -3379,15 +3383,12 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) 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" : + (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, + in_hsfraglen, + in_hsfraglen + (unsigned) hs_this_fragment_len, ssl->in_hslen)); /* Move the received handshake fragment to have the whole message @@ -3417,20 +3418,21 @@ 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 += (unsigned) ssl->in_msglen; + in_hsfraglen += (unsigned) ssl->in_msglen; payload_end += ssl->in_msglen; - if (ssl->badmac_seen_or_in_hsfraglen < ssl->in_hslen) { + if (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)); + in_hsfraglen, ssl->in_hslen)); ssl->in_hdr = payload_end; ssl->in_msglen = 0; + MBEDTLS_SSL_SET_IN_HSFRAGLEN(ssl, in_hsfraglen); mbedtls_ssl_update_in_pointers(ssl); 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_msglen = in_hsfraglen; + MBEDTLS_SSL_SET_IN_HSFRAGLEN(ssl, 0); ssl->in_hdr = reassembled_record_start; mbedtls_ssl_update_in_pointers(ssl); @@ -4785,11 +4787,11 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) return MBEDTLS_ERR_SSL_INTERNAL_ERROR; } - if (ssl->badmac_seen_or_in_hsfraglen != 0) { + if (mbedtls_ssl_get_in_hsfraglen(ssl) != 0) { /* Not all handshake fragments have arrived, do not consume. */ MBEDTLS_SSL_DEBUG_MSG(3, ("Consume: waiting for more handshake fragments " "%u/%" MBEDTLS_PRINTF_SIZET, - ssl->badmac_seen_or_in_hsfraglen, ssl->in_hslen)); + mbedtls_ssl_get_in_hsfraglen(ssl), ssl->in_hslen)); return 0; } @@ -5147,8 +5149,9 @@ static int ssl_get_next_record(mbedtls_ssl_context *ssl) } if (ssl->conf->badmac_limit != 0) { - ++ssl->badmac_seen_or_in_hsfraglen; - if (ssl->badmac_seen_or_in_hsfraglen >= ssl->conf->badmac_limit) { + unsigned badmac_seen = mbedtls_ssl_get_badmac_seen(ssl) + 1; + MBEDTLS_SSL_SET_BADMAC_SEEN(ssl, badmac_seen); + if (badmac_seen >= ssl->conf->badmac_limit) { MBEDTLS_SSL_DEBUG_MSG(1, ("too many records with bad MAC")); return MBEDTLS_ERR_SSL_INVALID_MAC; } @@ -5208,8 +5211,7 @@ int mbedtls_ssl_handle_message_type(mbedtls_ssl_context *ssl) * we don't accept any other message type. For TLS 1.3, the spec forbids * interleaving other message types between handshake fragments. For TLS * 1.2, the spec does not forbid it but we do. */ - if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_STREAM && - ssl->badmac_seen_or_in_hsfraglen != 0 && + if (mbedtls_ssl_get_in_hsfraglen(ssl) != 0 && ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE) { MBEDTLS_SSL_DEBUG_MSG(1, ("non-handshake message in the middle" " of a fragmented handshake message"));