coffee/mbedtls
1
0
Fork 0
mirror of https://github.com/Mbed-TLS/mbedtls.git synced 2026-11-09 14:50:17 +08:00
Code Issues Packages Projects Releases Wiki Activity

Fix confusion between ssl->badmac_seen and ssl->in_hsfraglen

Browse Source 
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 <Gilles.Peskine@arm.com>
This commit is contained in:
Gilles Peskine 2026-05-10 14:55:51 +02:00
parent 397db25dc3
commit 24c2875cb0
2 changed files with 86 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
library/ssl_misc.h
View File

@ -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

44
library/ssl_msg.c
View File

@ -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"));