Skip to content

Commit

Permalink
Merge pull request #5313 from gilles-peskine-arm/missing-ret-check-mb…
Browse files Browse the repository at this point in the history 
…edtls_md_hmac

Check HMAC return values
  • Loading branch information
@daverodgman
daverodgman authored Dec 13, 2021
2 parents c11192f + ecf6beb commit 050ad4b
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 31 deletions.
6 changes: 4 additions & 2 deletions ChangeLog.d/check-return.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ Bugfix
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
Expand All @@ -13,5 +15,5 @@ Features
(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.
5 changes: 5 additions & 0 deletions ChangeLog.d/ssl-mac-zeroize.txt
View file
Original file line number Diff line number Diff line change
@@ -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.
14 changes: 14 additions & 0 deletions include/mbedtls/md.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -181,6 +182,7 @@ void mbedtls_md_free( mbedtls_md_context_t *ctx );
* 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 );

/**
Expand All @@ -202,6 +204,7 @@ int mbedtls_md_setup( mbedtls_md_context_t *ctx, const mbedtls_md_info_t *md_inf
* \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 );

Expand Down Expand Up @@ -251,6 +254,7 @@ const char *mbedtls_md_get_name( const mbedtls_md_info_t *md_info );
* \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA on parameter-verification
* failure.
*/
MBEDTLS_CHECK_RETURN_TYPICAL
int mbedtls_md_starts( mbedtls_md_context_t *ctx );

/**
Expand All @@ -269,6 +273,7 @@ int mbedtls_md_starts( mbedtls_md_context_t *ctx );
* \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 );

/**
Expand All @@ -289,6 +294,7 @@ int mbedtls_md_update( mbedtls_md_context_t *ctx, const unsigned char *input, si
* \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 );

/**
Expand All @@ -309,6 +315,7 @@ int mbedtls_md_finish( mbedtls_md_context_t *ctx, unsigned char *output );
* \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 );

Expand All @@ -330,6 +337,7 @@ int mbedtls_md( const mbedtls_md_info_t *md_info, const unsigned char *input, si
* 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 */
Expand All @@ -352,6 +360,7 @@ int mbedtls_md_file( const mbedtls_md_info_t *md_info, const char *path,
* \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 );

Expand All @@ -374,6 +383,7 @@ int mbedtls_md_hmac_starts( mbedtls_md_context_t *ctx, const unsigned char *key,
* \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 );

Expand All @@ -395,6 +405,7 @@ int mbedtls_md_hmac_update( mbedtls_md_context_t *ctx, const unsigned char *inpu
* \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);

/**
Expand All @@ -412,6 +423,7 @@ int mbedtls_md_hmac_finish( mbedtls_md_context_t *ctx, unsigned char *output);
* \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 );

/**
Expand All @@ -436,11 +448,13 @@ int mbedtls_md_hmac_reset( mbedtls_md_context_t *ctx );
* \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
Expand Down
93 changes: 75 additions & 18 deletions library/ssl_msg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,16 +663,25 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl,
}
#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
Expand All @@ -683,6 +692,14 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl,
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 */

Expand Down Expand Up @@ -925,18 +942,34 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl,
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 */
}
Expand Down Expand Up @@ -1207,12 +1240,20 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl,
/* 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 );
Expand All @@ -1224,9 +1265,19 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl,
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 */

Expand Down Expand Up @@ -1418,7 +1469,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl,
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,
Expand All @@ -1441,6 +1492,12 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl,
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 );
}

/*
Expand Down
44 changes: 33 additions & 11 deletions library/ssl_tls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,19 +500,37 @@ static int tls_prf_generic( mbedtls_md_type_t md_type,
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;

Expand Down Expand Up @@ -958,8 +976,12 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform,
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 */
Expand Down

0 comments on commit 050ad4b

Please sign in to comment.