Catch failures of md_hmac operations
Declare mbedtls_md functions as MBEDTLS_CHECK_RETURN_TYPICAL, meaning that
their return values should be checked.
Do check the return values in our code. We were already doing that
everywhere for hash calculations, but not for HMAC calculations.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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/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 2d06fdd..e41455c 100644
--- a/library/ssl_msg.c
+++ b/library/ssl_msg.c
@@ -665,16 +665,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
@@ -685,7 +694,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 */
@@ -928,19 +944,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 */
}
@@ -1211,12 +1242,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,7 +1263,6 @@
transform->maclen );
/* Compare expected MAC with MAC at the end of the record. */
- ret = 0;
if( mbedtls_ct_memcmp( data + rec->data_len, mac_expect,
transform->maclen ) != 0 )
{
@@ -1237,7 +1275,11 @@
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 */
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 046caec..a566f81 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 */
