From 5b3669377404b2e35696e2d192a06cecd083d663 Mon Sep 17 00:00:00 2001 From: Arto Kinnunen Date: Wed, 20 Nov 2019 16:13:13 +0200 Subject: [PATCH 1/4] Harden hmac_drbg and md against fault injection -Add flow monitor, loop integrity check and variable doubling to harden mbedtls_hmac_drbg_update_ret. -Use longer hamming distance for nonce usage in hmac_drbg_reseed_core -Return actual value instead of success in mbedtls_hmac_drbg_seed and mbedtls_hmac_drbg_seed_buf -Check illegal condition in hmac_drbg_reseed_core. -Double buf/buf_len variables in mbedtls_hmac_drbg_random_with_add -Add more hamming distance to MBEDTLS_HMAC_DRBG_PR_ON/OFF --- include/mbedtls/hmac_drbg.h | 14 +++-- library/hmac_drbg.c | 102 +++++++++++++++++++++++++++--------- library/md.c | 15 +++++- 3 files changed, 101 insertions(+), 30 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index ed03854558..f38337aa78 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -70,8 +70,8 @@ /* \} name SECTION: Module settings */ -#define MBEDTLS_HMAC_DRBG_PR_OFF 0 /**< No prediction resistance */ -#define MBEDTLS_HMAC_DRBG_PR_ON 1 /**< Prediction resistance enabled */ +#define MBEDTLS_HMAC_DRBG_PR_OFF 0x55555555 /**< No prediction resistance */ +#define MBEDTLS_HMAC_DRBG_PR_ON 0x2AAAAAAA /**< Prediction resistance enabled */ #ifdef __cplusplus extern "C" { @@ -202,7 +202,8 @@ void mbedtls_hmac_drbg_set_reseed_interval( mbedtls_hmac_drbg_context *ctx, * \param add_len Length of additional data, or 0 * * \return \c 0 on success, or an error from the underlying - * hash calculation. + * hash calculation or + * MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. * * \note Additional data is optional, pass NULL and 0 as second * third argument if no additional data is being used. @@ -237,7 +238,8 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, * \return 0 if successful, or * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED, or * MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG, or - * MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG. + * MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG, or + * MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. */ int mbedtls_hmac_drbg_random_with_add( void *p_rng, unsigned char *output, size_t output_len, @@ -255,7 +257,9 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, * * \return 0 if successful, or * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED, or - * MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG + * MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG, + * MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG, or + * MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. */ int mbedtls_hmac_drbg_random( void *p_rng, unsigned char *output, size_t out_len ); diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index d0de5de004..824b009e16 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -51,6 +51,9 @@ #endif /* MBEDTLS_SELF_TEST */ #endif /* MBEDTLS_PLATFORM_C */ +#define HMAC_NONCE_YES 0x4AAAAAAA +#define HMAC_NONCE_NO 0x75555555 + /* * HMAC_DRBG context initialization */ @@ -74,40 +77,74 @@ int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, mbedtls_md_get_handle( &ctx->md_ctx ) ); unsigned char rounds = ( additional != NULL && add_len != 0 ) ? 2 : 1; unsigned char sep[1]; + volatile unsigned char flow_counter = 0; unsigned char K[MBEDTLS_MD_MAX_SIZE]; - int ret; + int ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; for( sep[0] = 0; sep[0] < rounds; sep[0]++ ) { /* Step 1 or 4 */ + flow_counter++; if( ( ret = mbedtls_md_hmac_reset( &ctx->md_ctx ) ) != 0 ) goto exit; + + flow_counter++; if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, ctx->V, md_len ) ) != 0 ) goto exit; + + flow_counter++; if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, sep, 1 ) ) != 0 ) goto exit; + if( rounds == 2 ) { + flow_counter++; if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, additional, add_len ) ) != 0 ) goto exit; } + + flow_counter++; if( ( ret = mbedtls_md_hmac_finish( &ctx->md_ctx, K ) ) != 0 ) goto exit; /* Step 2 or 5 */ + flow_counter++; if( ( ret = mbedtls_md_hmac_starts( &ctx->md_ctx, K, md_len ) ) != 0 ) goto exit; + + flow_counter++; if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, ctx->V, md_len ) ) != 0 ) goto exit; + + flow_counter++; if( ( ret = mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ) ) != 0 ) goto exit; } exit: + + if( ret == 0 ) + { + ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; + /* Check for possible attack. + * Counters needs to have correct values when returning success + */ + if ( ( ( flow_counter == 7 ) && ( sep[0] == 1 ) ) || + ( ( flow_counter == 16 ) && ( sep[0] == 2 ) ) ) + { + flow_counter = flow_counter - sep[0]; + // Double check flow_counter + if ( ( flow_counter == 6 ) || ( flow_counter == 14 ) ) + { + ret = 0; + } + } + } + mbedtls_platform_zeroize( K, sizeof( K ) ); return( ret ); } @@ -125,10 +162,10 @@ void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, * Simplified HMAC_DRBG initialisation (for use with deterministic ECDSA) */ int mbedtls_hmac_drbg_seed_buf( mbedtls_hmac_drbg_context *ctx, - mbedtls_md_handle_t md_info, - const unsigned char *data, size_t data_len ) + mbedtls_md_handle_t md_info, + const unsigned char *data, size_t data_len ) { - int ret; + int ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; if( ( ret = mbedtls_md_setup( &ctx->md_ctx, md_info, 1 ) ) != 0 ) return( ret ); @@ -146,7 +183,7 @@ int mbedtls_hmac_drbg_seed_buf( mbedtls_hmac_drbg_context *ctx, if( ( ret = mbedtls_hmac_drbg_update_ret( ctx, data, data_len ) ) != 0 ) return( ret ); - return( 0 ); + return( ret ); } /* @@ -160,22 +197,19 @@ static int hmac_drbg_reseed_core( mbedtls_hmac_drbg_context *ctx, { unsigned char seed[MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT]; size_t seedlen = 0; + size_t total_entropy_len; int ret; + if( use_nonce == HMAC_NONCE_NO ) + total_entropy_len = ctx->entropy_len; + else + total_entropy_len = ctx->entropy_len * 3 / 2; + + /* III. Check input length */ + if( len > MBEDTLS_HMAC_DRBG_MAX_INPUT || + total_entropy_len + len > MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT ) { - size_t total_entropy_len; - - if( use_nonce == 0 ) - total_entropy_len = ctx->entropy_len; - else - total_entropy_len = ctx->entropy_len * 3 / 2; - - /* III. Check input length */ - if( len > MBEDTLS_HMAC_DRBG_MAX_INPUT || - total_entropy_len + len > MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT ) - { - return( MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG ); - } + return( MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG ); } memset( seed, 0, MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT ); @@ -190,7 +224,7 @@ static int hmac_drbg_reseed_core( mbedtls_hmac_drbg_context *ctx, /* For initial seeding, allow adding of nonce generated * from the entropy source. See Sect 8.6.7 in SP800-90A. */ - if( use_nonce ) + if( use_nonce == HMAC_NONCE_YES ) { /* Note: We don't merge the two calls to f_entropy() in order * to avoid requesting too much entropy from f_entropy() @@ -225,6 +259,11 @@ static int hmac_drbg_reseed_core( mbedtls_hmac_drbg_context *ctx, ctx->reseed_counter = 1; exit: + if (ret == 0 && ctx->reseed_counter != 1) + { + /* Illegal condition, possible attack detected */ + ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; + } /* 4. Done */ mbedtls_platform_zeroize( seed, seedlen ); return( ret ); @@ -236,8 +275,7 @@ exit: int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, const unsigned char *additional, size_t len ) { - return( hmac_drbg_reseed_core( ctx, additional, len, - 0 /* no nonce */ ) ); + return( hmac_drbg_reseed_core( ctx, additional, len, HMAC_NONCE_NO ) ); } /* @@ -286,13 +324,12 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, md_size <= 28 ? 24 : /* 224-bits hash -> 192 bits */ 32; /* better (256+) -> 256 bits */ - if( ( ret = hmac_drbg_reseed_core( ctx, custom, len, - 1 /* add nonce */ ) ) != 0 ) + if( ( ret = hmac_drbg_reseed_core( ctx, custom, len, HMAC_NONCE_YES ) ) != 0 ) { return( ret ); } - return( 0 ); + return( ret ); } /* @@ -329,6 +366,8 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, const unsigned char *additional, size_t add_len ) { int ret; + volatile unsigned char *output_fi = output; + volatile size_t out_len_fi = out_len; mbedtls_hmac_drbg_context *ctx = (mbedtls_hmac_drbg_context *) p_rng; size_t md_len = mbedtls_md_get_size( mbedtls_md_get_handle( &ctx->md_ctx ) ); @@ -390,6 +429,21 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, exit: /* 8. Done */ + + if (ret == 0) + { + /* + * Check doubled variables and illegal conditions in case of possible + * attack. + */ + ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; + if ( ( out_len_fi == out_len ) && ( output_fi == output) && + ( left == 0 ) ) + { + ret = 0; + } + } + return( ret ); } diff --git a/library/md.c b/library/md.c index d9d6509735..7644ac6560 100644 --- a/library/md.c +++ b/library/md.c @@ -575,15 +575,28 @@ int mbedtls_md_hmac_starts( mbedtls_md_context_t *ctx, const unsigned char *key, if( ( ret = mbedtls_md_info_starts( md_info, ctx->md_ctx ) ) != 0 ) goto cleanup; + i++; // Use i as flow control + if( ( ret = mbedtls_md_info_update( md_info, ctx->md_ctx, ipad, mbedtls_md_info_block_size( md_info ) ) ) != 0 ) { goto cleanup; } + i++; // Use i as flow control now + cleanup: mbedtls_platform_zeroize( sum, sizeof( sum ) ); + if ( ret == 0 ) + { + ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; + /* Check possible fault injection */ + if ( ( i - 2 ) == keylen ) { + ret = 0; + } + } + return( ret ); } @@ -653,7 +666,7 @@ int mbedtls_md_hmac_finish( mbedtls_md_context_t *ctx, unsigned char *output ) if( ( ret = mbedtls_md_info_finish( md_info, ctx->md_ctx, output ) ) != 0 ) return( ret ); - return( 0 ); + return( ret ); } int mbedtls_md_hmac_reset( mbedtls_md_context_t *ctx ) From 5299a42f257ad0a43f2bedd9925f8423f3f02245 Mon Sep 17 00:00:00 2001 From: Arto Kinnunen Date: Mon, 25 Nov 2019 16:19:25 +0200 Subject: [PATCH 2/4] Update HMAC and MD FI countermeasures -Return error value by default. -Success is returned only after checking internal states. -Append flow_control to cover also last function call. --- library/hmac_drbg.c | 71 ++++++++++++++++++++++++--------------------- library/md.c | 18 +++++------- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 824b009e16..2ebdac0883 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -123,30 +123,30 @@ int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, flow_counter++; if( ( ret = mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ) ) != 0 ) goto exit; + flow_counter++; } exit: - if( ret == 0 ) + mbedtls_platform_zeroize( K, sizeof( K ) ); + /* Check for possible attack. + * Counters needs to have correct values when returning success + */ + if ( ret != 0 ) + return( ret ); // error case, return immediately + + if ( ( ( flow_counter == 8 ) && ( sep[0] == 1 ) ) || + ( ( flow_counter == 18 ) && ( sep[0] == 2 ) ) ) { - ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; - /* Check for possible attack. - * Counters needs to have correct values when returning success - */ - if ( ( ( flow_counter == 7 ) && ( sep[0] == 1 ) ) || - ( ( flow_counter == 16 ) && ( sep[0] == 2 ) ) ) + flow_counter = flow_counter - sep[0]; + // Double check flow_counter + if ( ( flow_counter == 7 ) || ( flow_counter == 16 ) ) { - flow_counter = flow_counter - sep[0]; - // Double check flow_counter - if ( ( flow_counter == 6 ) || ( flow_counter == 14 ) ) - { - ret = 0; - } + return ret; // success, return 0 from ret } } - mbedtls_platform_zeroize( K, sizeof( K ) ); - return( ret ); + return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED ); } #if !defined(MBEDTLS_DEPRECATED_REMOVED) @@ -259,14 +259,20 @@ static int hmac_drbg_reseed_core( mbedtls_hmac_drbg_context *ctx, ctx->reseed_counter = 1; exit: - if (ret == 0 && ctx->reseed_counter != 1) - { - /* Illegal condition, possible attack detected */ - ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; - } + /* 4. Done */ mbedtls_platform_zeroize( seed, seedlen ); - return( ret ); + + if ( ret != 0 ) + return ret; + + if ( ret == 0 && ctx->reseed_counter == 1 ) + { + /* All ok, return 0 from ret */ + return ret; + } + + return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED ); } /* @@ -430,21 +436,20 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, exit: /* 8. Done */ - if (ret == 0) + if ( ret != 0 ) + return ret; + + /* + * Check doubled variables and illegal conditions in case of possible + * attack. + */ + if ( ( out_len_fi == out_len ) && ( output_fi == output) && + ( left == 0 ) ) { - /* - * Check doubled variables and illegal conditions in case of possible - * attack. - */ - ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; - if ( ( out_len_fi == out_len ) && ( output_fi == output) && - ( left == 0 ) ) - { - ret = 0; - } + return ret; // Success, return 0 } - return( ret ); + return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED ); } /* diff --git a/library/md.c b/library/md.c index 7644ac6560..777b43caa0 100644 --- a/library/md.c +++ b/library/md.c @@ -525,7 +525,7 @@ int mbedtls_md_hmac_starts( mbedtls_md_context_t *ctx, const unsigned char *key, int ret; unsigned char sum[MBEDTLS_MD_MAX_SIZE]; unsigned char *ipad, *opad; - size_t i; + size_t i = 0; mbedtls_md_handle_t md_info; @@ -588,16 +588,14 @@ int mbedtls_md_hmac_starts( mbedtls_md_context_t *ctx, const unsigned char *key, cleanup: mbedtls_platform_zeroize( sum, sizeof( sum ) ); - if ( ret == 0 ) - { - ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; - /* Check possible fault injection */ - if ( ( i - 2 ) == keylen ) { - ret = 0; - } - } + if ( ret != 0 ) + return ret; - return( ret ); + /* Check possible fault injection */ + if ( ( i - 2 ) == keylen ) + return ret; // success, return 0 from ret + + return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED ); } int mbedtls_md_hmac_update( mbedtls_md_context_t *ctx, From c5bbdd411c620ed6739c62e7b96e222c9d43225a Mon Sep 17 00:00:00 2001 From: Arto Kinnunen Date: Mon, 25 Nov 2019 17:32:28 +0200 Subject: [PATCH 3/4] Include platform.h in hmac_drbg.c and md.c Include platform.h to find MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. --- library/hmac_drbg.c | 1 + library/md.c | 1 + 2 files changed, 2 insertions(+) diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 2ebdac0883..2b79ee5411 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -34,6 +34,7 @@ #if defined(MBEDTLS_HMAC_DRBG_C) #include "mbedtls/hmac_drbg.h" +#include "mbedtls/platform.h" #include "mbedtls/platform_util.h" #include diff --git a/library/md.c b/library/md.c index 777b43caa0..4f03acc38b 100644 --- a/library/md.c +++ b/library/md.c @@ -32,6 +32,7 @@ #if defined(MBEDTLS_MD_C) #include "mbedtls/md.h" +#include "mbedtls/platform.h" #include "mbedtls/platform_util.h" #if defined(MBEDTLS_PLATFORM_C) From ec37a89bf3287d6833b58ba1b365d3364990effd Mon Sep 17 00:00:00 2001 From: Arto Kinnunen Date: Thu, 28 Nov 2019 13:40:32 +0200 Subject: [PATCH 4/4] Save ROM by using int instead of char Save 20 bytes of ROM by changing variable type from char to int. --- library/hmac_drbg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 2b79ee5411..82703b0a32 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -78,7 +78,7 @@ int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, mbedtls_md_get_handle( &ctx->md_ctx ) ); unsigned char rounds = ( additional != NULL && add_len != 0 ) ? 2 : 1; unsigned char sep[1]; - volatile unsigned char flow_counter = 0; + volatile unsigned int flow_counter = 0; unsigned char K[MBEDTLS_MD_MAX_SIZE]; int ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED;