From 52344c2972bc174ab817ea22c7c8bf7b5fc53ac5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 3 Jan 2018 15:24:20 +0000 Subject: [PATCH] Correct space needed for MAC in case of NULL cipher The macro constant `MBEDTLS_SSL_MAC_ADD` defined in `ssl_internal.h` defines an upper bound for the amount of space needed for the record authentication tag. Its definition distinguishes between the presence of an ARC4 or CBC ciphersuite suite, in which case the maximum size of an enabled SHA digest is used; otherwise, `MBEDTLS_SSL_MAC_ADD` is set to 16 to accomodate AEAD authentication tags. This assignment has a flaw in the situation where confidentiality is not needed and the NULL cipher is in use. In this case, the authentication tag also uses a SHA digest, but the definition of `MBEDTLS_SSL_MAC_ADD` doesn't guarantee enough space. The present commit fixes this by distinguishing between the presence of *some* ciphersuite using a MAC, including those using a NULL cipher. For that, the previously internal macro `SSL_SOME_MODES_USE_MAC` from `ssl_tls.c` is renamed and moved to the public macro `MBEDTLS_SOME_MODES_USE_MAC` defined in `ssl_internal.h`. --- include/mbedtls/ssl_internal.h | 12 ++++++++++-- library/ssl_tls.c | 18 ++++++------------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index deaa93fbd..40ff53f12 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -146,7 +146,15 @@ #define MBEDTLS_SSL_COMPRESSION_ADD 0 #endif -#if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_MODE_CBC) +#if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ + ( defined(MBEDTLS_CIPHER_MODE_CBC) && \ + ( defined(MBEDTLS_AES_C) || \ + defined(MBEDTLS_CAMELLIA_C) || \ + defined(MBEDTLS_ARIA_C) ) ) +#define MBEDTLS_SSL_SOME_MODES_USE_MAC +#endif + +#if defined(MBEDTLS_SSL_SOME_MODES_USE_MAC) /* Ciphersuites using HMAC */ #if defined(MBEDTLS_SHA512_C) #define MBEDTLS_SSL_MAC_ADD 48 /* SHA-384 used for HMAC */ @@ -155,7 +163,7 @@ #else #define MBEDTLS_SSL_MAC_ADD 20 /* SHA-1 used for HMAC */ #endif -#else +#else /* MBEDTLS_SSL_SOME_MODES_USE_MAC */ /* AEAD ciphersuites: GCM and CCM use a 128 bits tag */ #define MBEDTLS_SSL_MAC_ADD 16 #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f0f38c0b2..780a682cc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1710,15 +1710,9 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, } #endif /* MBEDTLS_SSL_PROTO_SSL3 */ -#if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ - ( defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C)) ) -#define SSL_SOME_MODES_USE_MAC -#endif - /* The function below is only used in the Lucky 13 counter-measure in * ssl_decrypt_buf(). These are the defines that guard the call site. */ -#if defined(SSL_SOME_MODES_USE_MAC) && \ +#if defined(MBEDTLS_SSL_SOME_MODES_USE_MAC) && \ ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_2) ) @@ -1761,7 +1755,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) /* * Add MAC before if needed */ -#if defined(SSL_SOME_MODES_USE_MAC) +#if defined(MBEDTLS_SSL_SOME_MODES_USE_MAC) if( mode == MBEDTLS_MODE_STREAM || ( mode == MBEDTLS_MODE_CBC #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) @@ -1814,7 +1808,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) ssl->out_msglen += ssl->transform_out->maclen; auth_done++; } -#endif /* AEAD not the only option */ +#endif /* MBEDTLS_SSL_SOME_MODES_USE_MAC */ /* * Encrypt @@ -2090,7 +2084,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { mbedtls_cipher_mode_t mode; int auth_done = 0; -#if defined(SSL_SOME_MODES_USE_MAC) +#if defined(MBEDTLS_SSL_SOME_MODES_USE_MAC) size_t padlen = 0, correct = 1; #endif @@ -2470,7 +2464,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) * Authenticate if not done yet. * Compute the MAC regardless of the padding result (RFC4346, CBCTIME). */ -#if defined(SSL_SOME_MODES_USE_MAC) +#if defined(MBEDTLS_SSL_SOME_MODES_USE_MAC) if( auth_done == 0 ) { unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; @@ -2618,7 +2612,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) */ if( correct == 0 ) return( MBEDTLS_ERR_SSL_INVALID_MAC ); -#endif /* SSL_SOME_MODES_USE_MAC */ +#endif /* MBEDTLS_SSL_SOME_MODES_USE_MAC */ /* Make extra sure authentication was performed, exactly once */ if( auth_done != 1 )