Catch failures of md_hmac operations
Check the return values of mbedtls_md_xx functions 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_tls.c b/library/ssl_tls.c
index 155867b..04275a4 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -449,24 +449,45 @@
* First compute P_md5(secret,label+random)[0..dlen]
*/
if( ( md_info = mbedtls_md_info_from_type( MBEDTLS_MD_MD5 ) ) == NULL )
- return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
+ {
+ ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR;
+ goto exit;
+ }
if( ( ret = mbedtls_md_setup( &md_ctx, md_info, 1 ) ) != 0 )
- return( ret );
+ goto exit;
- mbedtls_md_hmac_starts( &md_ctx, S1, hs );
- mbedtls_md_hmac_update( &md_ctx, tmp + 20, nb );
- mbedtls_md_hmac_finish( &md_ctx, 4 + tmp );
+ ret = mbedtls_md_hmac_starts( &md_ctx, S1, hs );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_update( &md_ctx, tmp + 20, nb );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_finish( &md_ctx, 4 + tmp );
+ if( ret != 0 )
+ goto exit;
for( i = 0; i < dlen; i += 16 )
{
- mbedtls_md_hmac_reset ( &md_ctx );
- mbedtls_md_hmac_update( &md_ctx, 4 + tmp, 16 + 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, 4 + tmp, 16 + 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, 4 + tmp, 16 );
- mbedtls_md_hmac_finish( &md_ctx, 4 + tmp );
+ ret = mbedtls_md_hmac_reset ( &md_ctx );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_update( &md_ctx, 4 + tmp, 16 );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_finish( &md_ctx, 4 + tmp );
+ if( ret != 0 )
+ goto exit;
k = ( i + 16 > dlen ) ? dlen % 16 : 16;
@@ -480,24 +501,45 @@
* XOR out with P_sha1(secret,label+random)[0..dlen]
*/
if( ( md_info = mbedtls_md_info_from_type( MBEDTLS_MD_SHA1 ) ) == NULL )
- return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
+ {
+ ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR;
+ goto exit;
+ }
if( ( ret = mbedtls_md_setup( &md_ctx, md_info, 1 ) ) != 0 )
- return( ret );
+ goto exit;
- mbedtls_md_hmac_starts( &md_ctx, S2, hs );
- mbedtls_md_hmac_update( &md_ctx, tmp + 20, nb );
- mbedtls_md_hmac_finish( &md_ctx, tmp );
+ ret = mbedtls_md_hmac_starts( &md_ctx, S2, hs );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_update( &md_ctx, tmp + 20, 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 += 20 )
{
- mbedtls_md_hmac_reset ( &md_ctx );
- mbedtls_md_hmac_update( &md_ctx, tmp, 20 + 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, 20 + 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, 20 );
- 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, 20 );
+ if( ret != 0 )
+ goto exit;
+ ret = mbedtls_md_hmac_finish( &md_ctx, tmp );
+ if( ret != 0 )
+ goto exit;
k = ( i + 20 > dlen ) ? dlen % 20 : 20;
@@ -505,6 +547,7 @@
dstbuf[i + j] = (unsigned char)( dstbuf[i + j] ^ h_i[j] );
}
+exit:
mbedtls_md_free( &md_ctx );
mbedtls_platform_zeroize( tmp, sizeof( tmp ) );
@@ -548,21 +591,39 @@
* Compute P_<hash>(secret, label + random)[0..dlen]
*/
if ( ( ret = mbedtls_md_setup( &md_ctx, md_info, 1 ) ) != 0 )
- return( ret );
+ 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;
@@ -570,6 +631,7 @@
dstbuf[i + j] = h_i[j];
}
+exit:
mbedtls_md_free( &md_ctx );
mbedtls_platform_zeroize( tmp, sizeof( tmp ) );
@@ -1015,8 +1077,14 @@
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 )
+ return( ret );
+ ret = mbedtls_md_hmac_starts( &transform->md_ctx_dec,
+ mac_dec, mac_key_len );
+ if( ret != 0 )
+ return( ret );
}
}
else
@@ -1390,17 +1458,18 @@
* SSLv3.0 MAC functions
*/
#define SSL_MAC_MAX_BYTES 20 /* MD-5 or SHA-1 */
-static void ssl_mac( mbedtls_md_context_t *md_ctx,
- const unsigned char *secret,
- const unsigned char *buf, size_t len,
- const unsigned char *ctr, int type,
- unsigned char out[SSL_MAC_MAX_BYTES] )
+static int ssl_mac( mbedtls_md_context_t *md_ctx,
+ const unsigned char *secret,
+ const unsigned char *buf, size_t len,
+ const unsigned char *ctr, int type,
+ unsigned char out[SSL_MAC_MAX_BYTES] )
{
unsigned char header[11];
unsigned char padding[48];
int padlen;
int md_size = mbedtls_md_get_size( md_ctx->md_info );
int md_type = mbedtls_md_get_type( md_ctx->md_info );
+ int ret;
/* Only MD5 and SHA-1 supported */
if( md_type == MBEDTLS_MD_MD5 )
@@ -1414,19 +1483,43 @@
header[10] = (unsigned char)( len );
memset( padding, 0x36, padlen );
- mbedtls_md_starts( md_ctx );
- mbedtls_md_update( md_ctx, secret, md_size );
- mbedtls_md_update( md_ctx, padding, padlen );
- mbedtls_md_update( md_ctx, header, 11 );
- mbedtls_md_update( md_ctx, buf, len );
- mbedtls_md_finish( md_ctx, out );
+ ret = mbedtls_md_starts( md_ctx );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, secret, md_size );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, padding, padlen );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, header, 11 );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, buf, len );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_finish( md_ctx, out );
+ if( ret != 0 )
+ return( ret );
memset( padding, 0x5C, padlen );
- mbedtls_md_starts( md_ctx );
- mbedtls_md_update( md_ctx, secret, md_size );
- mbedtls_md_update( md_ctx, padding, padlen );
- mbedtls_md_update( md_ctx, out, md_size );
- mbedtls_md_finish( md_ctx, out );
+ ret = mbedtls_md_starts( md_ctx );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, secret, md_size );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, padding, padlen );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_update( md_ctx, out, md_size );
+ if( ret != 0 )
+ return( ret );
+ ret = mbedtls_md_finish( md_ctx, out );
+ if( ret != 0 )
+ return( ret );
+
+ return( 0 );
}
#endif /* MBEDTLS_SSL_PROTO_SSL3 */
@@ -1471,15 +1564,22 @@
if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 )
{
unsigned char mac[SSL_MAC_MAX_BYTES];
+ int ret;
- ssl_mac( &ssl->transform_out->md_ctx_enc,
- ssl->transform_out->mac_enc,
- ssl->out_msg, ssl->out_msglen,
- ssl->out_ctr, ssl->out_msgtype,
- mac );
+ ret = ssl_mac( &ssl->transform_out->md_ctx_enc,
+ ssl->transform_out->mac_enc,
+ ssl->out_msg, ssl->out_msglen,
+ ssl->out_ctr, ssl->out_msgtype,
+ mac );
- memcpy( ssl->out_msg + ssl->out_msglen, mac, ssl->transform_out->maclen );
+ if( ret == 0 )
+ memcpy( ssl->out_msg + ssl->out_msglen, mac, ssl->transform_out->maclen );
mbedtls_platform_zeroize( mac, ssl->transform_out->maclen );
+ if( ret != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_RET( 1, "ssl_mac", ret );
+ return( ret );
+ }
}
else
#endif
@@ -1488,17 +1588,35 @@
if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 )
{
unsigned char mac[MBEDTLS_SSL_MAC_ADD];
+ int ret;
- mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_ctr, 8 );
- mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_hdr, 3 );
- mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_len, 2 );
- mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc,
- ssl->out_msg, ssl->out_msglen );
- mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac );
- mbedtls_md_hmac_reset( &ssl->transform_out->md_ctx_enc );
+ ret = mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_ctr, 8 );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_hdr, 3 );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_len, 2 );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc,
+ ssl->out_msg, ssl->out_msglen );
+ ret = mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
+ ret = mbedtls_md_hmac_reset( &ssl->transform_out->md_ctx_enc );
+ if( ret != 0 )
+ goto hmac_failed_etm_disabled;
memcpy( ssl->out_msg + ssl->out_msglen, mac, ssl->transform_out->maclen );
+
+ hmac_failed_etm_disabled:
mbedtls_platform_zeroize( mac, ssl->transform_out->maclen );
+ if( ret != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_md_hmac_xxx", ret );
+ return( ret );
+ }
}
else
#endif
@@ -1751,18 +1869,33 @@
MBEDTLS_SSL_DEBUG_BUF( 4, "MAC'd meta-data", pseudo_hdr, 13 );
- mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, pseudo_hdr, 13 );
- mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc,
- ssl->out_iv, ssl->out_msglen );
- mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac );
- mbedtls_md_hmac_reset( &ssl->transform_out->md_ctx_enc );
+ ret = mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, pseudo_hdr, 13 );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc,
+ ssl->out_iv, ssl->out_msglen );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_reset( &ssl->transform_out->md_ctx_enc );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
memcpy( ssl->out_iv + ssl->out_msglen, mac,
ssl->transform_out->maclen );
ssl->out_msglen += ssl->transform_out->maclen;
auth_done++;
+
+ hmac_failed_etm_enabled:
mbedtls_platform_zeroize( mac, ssl->transform_out->maclen );
+ if( ret != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_RET( 1, "HMAC calculation failed", ret );
+ return( ret );
+ }
}
#endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */
}
@@ -2148,18 +2281,25 @@
MBEDTLS_SSL_DEBUG_BUF( 4, "MAC'd meta-data", pseudo_hdr, 13 );
- mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, pseudo_hdr, 13 );
- mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec,
- ssl->in_iv, ssl->in_msglen );
- mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect );
- mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec );
+ ret = mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, pseudo_hdr, 13 );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec,
+ ssl->in_iv, ssl->in_msglen );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
+ ret = mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec );
+ if( ret != 0 )
+ goto hmac_failed_etm_enabled;
MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_iv + ssl->in_msglen,
ssl->transform_in->maclen );
MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect,
ssl->transform_in->maclen );
- ret = 0;
if( mbedtls_ssl_safer_memcmp( ssl->in_iv + ssl->in_msglen, mac_expect,
ssl->transform_in->maclen ) != 0 )
{
@@ -2173,7 +2313,11 @@
hmac_failed_etm_enabled:
mbedtls_platform_zeroize( mac_expect, ssl->transform_in->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 */
@@ -2342,11 +2486,16 @@
#if defined(MBEDTLS_SSL_PROTO_SSL3)
if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 )
{
- ssl_mac( &ssl->transform_in->md_ctx_dec,
- ssl->transform_in->mac_dec,
- ssl->in_msg, ssl->in_msglen,
- ssl->in_ctr, ssl->in_msgtype,
- mac_expect );
+ ret = ssl_mac( &ssl->transform_in->md_ctx_dec,
+ ssl->transform_in->mac_dec,
+ ssl->in_msg, ssl->in_msglen,
+ ssl->in_ctr, ssl->in_msgtype,
+ mac_expect );
+ if( ret != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_RET( 1, "ssl_mac", ret );
+ return( ret );
+ }
memcpy( mac_peer, ssl->in_msg + ssl->in_msglen,
ssl->transform_in->maclen );
}