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/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 */