From 5260ce27edaaa43e4df193b146e0881d238ff7ae Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Mon, 9 May 2022 18:15:54 +0100 Subject: [PATCH] Fix uninitialised memory access in constant time functions Fix an issue reported by Coverity whereby some constant time functions called from the ssl decrypt code could potentially access uninitialised memory. Signed-off-by: Paul Elliott --- library/constant_time.c | 12 ++++++++++++ library/constant_time_internal.h | 7 +++++++ library/ssl_msg.c | 4 ++-- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index a6451bb67..0b409b3e4 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -514,6 +514,12 @@ int mbedtls_ct_hmac( mbedtls_svc_key_id_t key, PSA_CHK( psa_hash_update( &operation, add_data, add_data_len ) ); PSA_CHK( psa_hash_update( &operation, data, min_data_len ) ); + /* Fill the hash buffer in advance with something that is + * not a valid hash (barring an attack on the hash and + * deliberately-crafted input), in case the caller doesn't + * check the return status properly. */ + memset( output, '!', hash_size ); + /* For each possible length, compute the hash up to that point */ for( offset = min_data_len; offset <= max_data_len; offset++ ) { @@ -609,6 +615,12 @@ int mbedtls_ct_hmac( mbedtls_md_context_t *ctx, MD_CHK( mbedtls_md_update( ctx, add_data, add_data_len ) ); MD_CHK( mbedtls_md_update( ctx, data, min_data_len ) ); + /* Fill the hash buffer in advance with something that is + * not a valid hash (barring an attack on the hash and + * deliberately-crafted input), in case the caller doesn't + * check the return status properly. */ + memset( output, '!', hash_size ); + /* For each possible length, compute the hash up to that point */ for( offset = min_data_len; offset <= max_data_len; offset++ ) { diff --git a/library/constant_time_internal.h b/library/constant_time_internal.h index 4838d05e9..9466bc378 100644 --- a/library/constant_time_internal.h +++ b/library/constant_time_internal.h @@ -221,6 +221,13 @@ void mbedtls_ct_memcpy_if_eq( unsigned char *dest, * offset_secret, but only on \p offset_min, \p offset_max and \p len. * Functionally equivalent to `memcpy(dst, src + offset_secret, len)`. * + * \note This function reads from \p dest, but the value that + * is read does not influence the result and this + * function's behavior is well-defined regardless of the + * contents of the buffers. This may result in false + * positives from static or dynamic analyzers, especially + * if \p dest is not initialized. + * * \param dest The destination buffer. This must point to a writable * buffer of at least \p len bytes. * \param src The base of the source buffer. This must point to a diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 083c8d2e6..ba412afb0 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1634,8 +1634,8 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, #if defined(MBEDTLS_SSL_SOME_SUITES_USE_MAC) if( auth_done == 0 ) { - unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; - unsigned char mac_peer[MBEDTLS_SSL_MAC_ADD]; + unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD] = { 0 }; + unsigned char mac_peer[MBEDTLS_SSL_MAC_ADD] = { 0 }; /* If the initial value of padlen was such that * data_len < maclen + padlen + 1, then padlen