Merge pull request #5313 from gilles-peskine-arm/missing-ret-check-mbedtls_md_hmac
Check HMAC return values
diff --git a/ChangeLog.d/check-return.txt b/ChangeLog.d/check-return.txt
index 045b180..7d905da 100644
--- a/ChangeLog.d/check-return.txt
+++ b/ChangeLog.d/check-return.txt
@@ -5,6 +5,8 @@
This does not concern the implementation provided with Mbed TLS,
where this function cannot fail, or full-module replacements with
MBEDTLS_AES_ALT or MBEDTLS_DES_ALT. Reported by Armelle Duboc in #1092.
+ * Some failures of HMAC operations were ignored. These failures could only
+ happen with an alternative implementation of the underlying hash module.
Features
* Warn if errors from certain functions are ignored. This is currently
@@ -13,5 +15,5 @@
(where supported) for critical functions where ignoring the return
value is almost always a bug. Enable the new configuration option
MBEDTLS_CHECK_RETURN_WARNING to get warnings for other functions. This
- is currently implemented in the AES and DES modules, and will be extended
- to other modules in the future.
+ is currently implemented in the AES, DES and md modules, and will be
+ extended to other modules in the future.
diff --git a/ChangeLog.d/ssl-mac-zeroize.txt b/ChangeLog.d/ssl-mac-zeroize.txt
new file mode 100644
index 0000000..b49c7ac
--- /dev/null
+++ b/ChangeLog.d/ssl-mac-zeroize.txt
@@ -0,0 +1,5 @@
+Security
+ * Zeroize intermediate variables used to calculate the MAC in CBC cipher
+ suites. This hardens the library in case stack memory leaks through a
+ memory disclosure vulnerabilty, which could formerly have allowed a
+ man-in-the-middle to inject fake ciphertext into a DTLS connection.
diff --git a/include/mbedtls/md.h b/include/mbedtls/md.h
index fa2b152..2b668f5 100644
--- a/include/mbedtls/md.h
+++ b/include/mbedtls/md.h
@@ -29,6 +29,7 @@
#include <stddef.h>
#include "mbedtls/build_info.h"
+#include "mbedtls/platform_util.h"
/** The selected feature is not available. */
#define MBEDTLS_ERR_MD_FEATURE_UNAVAILABLE -0x5080
@@ -181,6 +182,7 @@
* failure.
* \return #MBEDTLS_ERR_MD_ALLOC_FAILED on memory-allocation failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_setup( mbedtls_md_context_t *ctx, const mbedtls_md_info_t *md_info, int hmac );
/**
@@ -202,6 +204,7 @@
* \return \c 0 on success.
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_clone( mbedtls_md_context_t *dst,
const mbedtls_md_context_t *src );
@@ -251,6 +254,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_starts( mbedtls_md_context_t *ctx );
/**
@@ -269,6 +273,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_update( mbedtls_md_context_t *ctx, const unsigned char *input, size_t ilen );
/**
@@ -289,6 +294,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_finish( mbedtls_md_context_t *ctx, unsigned char *output );
/**
@@ -309,6 +315,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md( const mbedtls_md_info_t *md_info, const unsigned char *input, size_t ilen,
unsigned char *output );
@@ -330,6 +337,7 @@
* the file pointed by \p path.
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA if \p md_info was NULL.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_file( const mbedtls_md_info_t *md_info, const char *path,
unsigned char *output );
#endif /* MBEDTLS_FS_IO */
@@ -352,6 +360,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_hmac_starts( mbedtls_md_context_t *ctx, const unsigned char *key,
size_t keylen );
@@ -374,6 +383,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_hmac_update( mbedtls_md_context_t *ctx, const unsigned char *input,
size_t ilen );
@@ -395,6 +405,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_hmac_finish( mbedtls_md_context_t *ctx, unsigned char *output);
/**
@@ -412,6 +423,7 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_hmac_reset( mbedtls_md_context_t *ctx );
/**
@@ -436,11 +448,13 @@
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_hmac( const mbedtls_md_info_t *md_info, const unsigned char *key, size_t keylen,
const unsigned char *input, size_t ilen,
unsigned char *output );
/* Internal use */
+MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_process( mbedtls_md_context_t *ctx, const unsigned char *data );
#ifdef __cplusplus
diff --git a/library/ssl_msg.c b/library/ssl_msg.c
index 7c523ee..51eb461 100644
--- a/library/ssl_msg.c
+++ b/library/ssl_msg.c
@@ -663,16 +663,25 @@
}
#if defined(MBEDTLS_SSL_PROTO_TLS1_2)
unsigned char mac[MBEDTLS_SSL_MAC_ADD];
+ int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
ssl_extract_add_data_from_record( add_data, &add_data_len, rec,
transform->minor_ver,
transform->taglen );
- mbedtls_md_hmac_update( &transform->md_ctx_enc, add_data,
- add_data_len );
- mbedtls_md_hmac_update( &transform->md_ctx_enc, data, rec->data_len );
- mbedtls_md_hmac_finish( &transform->md_ctx_enc, mac );
- mbedtls_md_hmac_reset( &transform->md_ctx_enc );
+ ret = mbedtls_md_hmac_update( &transform->md_ctx_enc, add_data,
+ add_data_len );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_update( &transform->md_ctx_enc, data, rec->data_len );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_finish( &transform->md_ctx_enc, mac );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_reset( &transform->md_ctx_enc );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
memcpy( data + rec->data_len, mac, transform->maclen );
#endif
@@ -683,6 +692,14 @@
rec->data_len += transform->maclen;
post_avail -= transform->maclen;
auth_done++;
+
+ hmac_failed_etm_disabled:
+ mbedtls_platform_zeroize( mac, transform->maclen );
+ if( ret != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_md_hmac_xxx", ret );
+ return( ret );
+ }
}
#endif /* MBEDTLS_SSL_SOME_SUITES_USE_MAC */
@@ -925,18 +942,34 @@
MBEDTLS_SSL_DEBUG_BUF( 4, "MAC'd meta-data", add_data,
add_data_len );
- mbedtls_md_hmac_update( &transform->md_ctx_enc, add_data,
- add_data_len );
- mbedtls_md_hmac_update( &transform->md_ctx_enc,
- data, rec->data_len );
- mbedtls_md_hmac_finish( &transform->md_ctx_enc, mac );
- mbedtls_md_hmac_reset( &transform->md_ctx_enc );
+ ret = mbedtls_md_hmac_update( &transform->md_ctx_enc, add_data,
+ add_data_len );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_update( &transform->md_ctx_enc,
+ data, rec->data_len );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_finish( &transform->md_ctx_enc, mac );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_reset( &transform->md_ctx_enc );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
memcpy( data + rec->data_len, mac, transform->maclen );
rec->data_len += transform->maclen;
post_avail -= transform->maclen;
auth_done++;
+
+ hmac_failed_etm_enabled:
+ mbedtls_platform_zeroize( mac, transform->maclen );
+ if( ret != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_RET( 1, "HMAC calculation failed", ret );
+ return( ret );
+ }
}
#endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */
}
@@ -1207,12 +1240,20 @@
/* Calculate expected MAC. */
MBEDTLS_SSL_DEBUG_BUF( 4, "MAC'd meta-data", add_data,
add_data_len );
- mbedtls_md_hmac_update( &transform->md_ctx_dec, add_data,
- add_data_len );
- mbedtls_md_hmac_update( &transform->md_ctx_dec,
+ ret = mbedtls_md_hmac_update( &transform->md_ctx_dec, add_data,
+ add_data_len );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_update( &transform->md_ctx_dec,
data, rec->data_len );
- mbedtls_md_hmac_finish( &transform->md_ctx_dec, mac_expect );
- mbedtls_md_hmac_reset( &transform->md_ctx_dec );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_finish( &transform->md_ctx_dec, mac_expect );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_reset( &transform->md_ctx_dec );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", data + rec->data_len,
transform->maclen );
@@ -1224,9 +1265,19 @@
transform->maclen ) != 0 )
{
MBEDTLS_SSL_DEBUG_MSG( 1, ( "message mac does not match" ) );
- return( MBEDTLS_ERR_SSL_INVALID_MAC );
+ ret = MBEDTLS_ERR_SSL_INVALID_MAC;
+ goto hmac_failed_etm_enabled;
}
auth_done++;
+
+ hmac_failed_etm_enabled:
+ mbedtls_platform_zeroize( mac_expect, transform->maclen );
+ if( ret != 0 )
+ {
+ if( ret != MBEDTLS_ERR_SSL_INVALID_MAC )
+ MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_hmac_xxx", ret );
+ return( ret );
+ }
}
#endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */
@@ -1418,7 +1469,7 @@
if( ret != 0 )
{
MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ct_hmac", ret );
- return( ret );
+ goto hmac_failed_etm_disabled;
}
mbedtls_ct_memcpy_offset( mac_peer, data,
@@ -1441,6 +1492,12 @@
correct = 0;
}
auth_done++;
+
+ hmac_failed_etm_disabled:
+ mbedtls_platform_zeroize( mac_peer, transform->maclen );
+ mbedtls_platform_zeroize( mac_expect, transform->maclen );
+ if( ret != 0 )
+ return( ret );
}
/*
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 12ebc06..a974dbb 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -500,19 +500,37 @@
if ( ( ret = mbedtls_md_setup( &md_ctx, md_info, 1 ) ) != 0 )
goto exit;
- mbedtls_md_hmac_starts( &md_ctx, secret, slen );
- mbedtls_md_hmac_update( &md_ctx, tmp + md_len, nb );
- mbedtls_md_hmac_finish( &md_ctx, tmp );
+ ret = mbedtls_md_hmac_starts( &md_ctx, secret, slen );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_update( &md_ctx, tmp + md_len, nb );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_finish( &md_ctx, tmp );
+ if( ret != 0 )
+ goto exit;
for( i = 0; i < dlen; i += md_len )
{
- mbedtls_md_hmac_reset ( &md_ctx );
- mbedtls_md_hmac_update( &md_ctx, tmp, md_len + nb );
- mbedtls_md_hmac_finish( &md_ctx, h_i );
+ ret = mbedtls_md_hmac_reset ( &md_ctx );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_update( &md_ctx, tmp, md_len + nb );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_finish( &md_ctx, h_i );
+ if( ret != 0 )
+ goto exit;
- mbedtls_md_hmac_reset ( &md_ctx );
- mbedtls_md_hmac_update( &md_ctx, tmp, md_len );
- mbedtls_md_hmac_finish( &md_ctx, tmp );
+ ret = mbedtls_md_hmac_reset ( &md_ctx );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_update( &md_ctx, tmp, md_len );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_finish( &md_ctx, tmp );
+ if( ret != 0 )
+ goto exit;
k = ( i + md_len > dlen ) ? dlen % md_len : md_len;
@@ -958,8 +976,12 @@
For AEAD-based ciphersuites, there is nothing to do here. */
if( mac_key_len != 0 )
{
- mbedtls_md_hmac_starts( &transform->md_ctx_enc, mac_enc, mac_key_len );
- mbedtls_md_hmac_starts( &transform->md_ctx_dec, mac_dec, mac_key_len );
+ ret = mbedtls_md_hmac_starts( &transform->md_ctx_enc, mac_enc, mac_key_len );
+ if( ret != 0 )
+ goto end;
+ ret = mbedtls_md_hmac_starts( &transform->md_ctx_dec, mac_dec, mac_key_len );
+ if( ret != 0 )
+ goto end;
}
#endif
#endif /* MBEDTLS_SSL_SOME_SUITES_USE_MAC */