From 7c28ccf8abd06f8b468090f5b30f9b0409a2c023 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jun 2026 14:56:59 +0200 Subject: [PATCH] Use functions instead of macros for badmac_seen_or_in_hsfraglen setters Let the caller handle returning on error. This is less surprising, and it's more flexible in case the caller needs to perform some cleanup (which is currently not the case). Signed-off-by: Gilles Peskine --- library/ssl_misc.h | 57 +++++++++++++++++++--------------------------- library/ssl_msg.c | 12 +++++++--- 2 files changed, 32 insertions(+), 37 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index e4a01993f3..db31548cdf 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -496,27 +496,22 @@ static inline unsigned mbedtls_ssl_get_badmac_seen(const mbedtls_ssl_context *ss } #if defined(MBEDTLS_SSL_PROTO_DTLS) -static inline void mbedtls_ssl_set_badmac_seen(mbedtls_ssl_context *ssl, - unsigned badmac_seen) -{ - ssl->badmac_seen_or_in_hsfraglen = badmac_seen; -} - -#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 /* We shouldn't be trying to set badmac_seen if DTLS support is disabled * at compile time. If this is called from a code block that checks for the * DTLS protocol at run time, it should be guarded by * defined(MBEDTLS_SSL_PROTO_DTLS). */ -#undef MBEDTLS_SSL_SET_BADMAC_SEEN +MBEDTLS_CHECK_RETURN_CRITICAL +static inline int mbedtls_ssl_set_badmac_seen(mbedtls_ssl_context *ssl, + unsigned badmac_seen) +{ + 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; + } + ssl->badmac_seen_or_in_hsfraglen = badmac_seen; + return 0; +} #endif static inline unsigned mbedtls_ssl_get_in_hsfraglen(const mbedtls_ssl_context *ssl) @@ -529,26 +524,20 @@ static inline unsigned mbedtls_ssl_get_in_hsfraglen(const mbedtls_ssl_context *s return ssl->badmac_seen_or_in_hsfraglen; } -static inline void mbedtls_ssl_set_in_hsfraglen(mbedtls_ssl_context *ssl, - unsigned in_hsfraglen) +MBEDTLS_CHECK_RETURN_CRITICAL +static inline int 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) + 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; + } #endif + ssl->badmac_seen_or_in_hsfraglen = in_hsfraglen; + return 0; +} /* * TLS extension flags (for extensions with outgoing ServerHello content diff --git a/library/ssl_msg.c b/library/ssl_msg.c index be4406b1a2..b4fa74ca9e 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3427,12 +3427,16 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) in_hsfraglen, ssl->in_hslen)); ssl->in_hdr = payload_end; ssl->in_msglen = 0; - MBEDTLS_SSL_SET_IN_HSFRAGLEN(ssl, in_hsfraglen); + if (mbedtls_ssl_set_in_hsfraglen(ssl, in_hsfraglen) != 0) { + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + } mbedtls_ssl_update_in_pointers(ssl); return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; } else { ssl->in_msglen = in_hsfraglen; - MBEDTLS_SSL_SET_IN_HSFRAGLEN(ssl, 0); + if (mbedtls_ssl_set_in_hsfraglen(ssl, 0) != 0) { + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + } ssl->in_hdr = reassembled_record_start; mbedtls_ssl_update_in_pointers(ssl); @@ -5150,7 +5154,9 @@ static int ssl_get_next_record(mbedtls_ssl_context *ssl) if (ssl->conf->badmac_limit != 0) { unsigned badmac_seen = mbedtls_ssl_get_badmac_seen(ssl) + 1; - MBEDTLS_SSL_SET_BADMAC_SEEN(ssl, badmac_seen); + if (mbedtls_ssl_set_badmac_seen(ssl, badmac_seen) != 0) { + return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + } if (badmac_seen >= ssl->conf->badmac_limit) { MBEDTLS_SSL_DEBUG_MSG(1, ("too many records with bad MAC")); return MBEDTLS_ERR_SSL_INVALID_MAC;