From c68b70c9dc33303edf8ac4498998f912fd1eaa8f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2019 11:47:35 +0200 Subject: [PATCH 01/10] HMAC_DRBG: support set_entropy_len() before seed() mbedtls_hmac_drbg_seed() always set the entropy length to the default, so a call to mbedtls_hmac_drbg_set_entropy_len() before seed() had no effect. Change this to the more intuitive behavior that set_entropy_len() sets the entropy length and seed() respects that and only uses the default entropy length if there was no call to set_entropy_len(). --- include/mbedtls/hmac_drbg.h | 19 ++++++------------- library/hmac_drbg.c | 25 ++++++++++++++----------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 20b29d981..7931c2281 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -139,13 +139,11 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * Note that SHA-256 is just as efficient as SHA-224. * The security strength can be reduced if a smaller * entropy length is set with - * mbedtls_hmac_drbg_set_entropy_len() afterwards. + * mbedtls_hmac_drbg_set_entropy_len(). * - * \note The entropy length for the initial seeding is - * the security strength (converted from bits to bytes). - * You can set a different entropy length for subsequent - * seeding by calling mbedtls_hmac_drbg_set_entropy_len() - * after this function. + * \note The default entropy length is the security strength + * (converted from bits to bytes). You can override + * it by calling mbedtls_hmac_drbg_set_entropy_len(). * * \note During the initial seeding, this function calls * the entropy source to obtain a nonce @@ -224,14 +222,9 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx /** * \brief This function sets the amount of entropy grabbed on each - * reseed. + * seed or reseed. * - * The default value is set by mbedtls_hmac_drbg_seed(). - * - * \note mbedtls_hmac_drbg_seed() always sets the entropy length - * to the default value based on the chosen MD algorithm, - * so this function only has an effect if it is called - * after mbedtls_hmac_drbg_seed(). + * See the documentation of mbedtls_hmac_drbg_seed() for the default value. * * \param ctx The HMAC_DRBG context. * \param len The amount of entropy to grab, in bytes. diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 50d88bd54..284c9b4e9 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -273,16 +273,19 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, ctx->reseed_interval = MBEDTLS_HMAC_DRBG_RESEED_INTERVAL; - /* - * See SP800-57 5.6.1 (p. 65-66) for the security strength provided by - * each hash function, then according to SP800-90A rev1 10.1 table 2, - * min_entropy_len (in bits) is security_strength. - * - * (This also matches the sizes used in the NIST test vectors.) - */ - ctx->entropy_len = md_size <= 20 ? 16 : /* 160-bits hash -> 128 bits */ - md_size <= 28 ? 24 : /* 224-bits hash -> 192 bits */ - 32; /* better (256+) -> 256 bits */ + if( ctx->entropy_len == 0 ) + { + /* + * See SP800-57 5.6.1 (p. 65-66) for the security strength provided by + * each hash function, then according to SP800-90A rev1 10.1 table 2, + * min_entropy_len (in bits) is security_strength. + * + * (This also matches the sizes used in the NIST test vectors.) + */ + ctx->entropy_len = md_size <= 20 ? 16 : /* 160-bits hash -> 128 bits */ + 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 ) @@ -303,7 +306,7 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx } /* - * Set entropy length grabbed for reseeds + * Set entropy length grabbed for seeding */ void mbedtls_hmac_drbg_set_entropy_len( mbedtls_hmac_drbg_context *ctx, size_t len ) { From c4280acfa0e9814f1bc062a3226082357799a6d8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Oct 2019 20:31:54 +0200 Subject: [PATCH 02/10] CTR_DRBG: Don't use functions before they're defined Move the definitions of mbedtls_ctr_drbg_seed_entropy_len() and mbedtls_ctr_drbg_seed() to after they are used. This makes the code easier to read and to maintain. --- library/ctr_drbg.c | 124 ++++++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index fb121575b..80544dfb8 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -62,68 +62,6 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ) #endif } -/* - * Non-public function wrapped by mbedtls_ctr_drbg_seed(). Necessary to allow - * NIST tests to succeed (which require known length fixed entropy) - */ -/* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) - * mbedtls_ctr_drbg_seed_entropy_len(ctx, f_entropy, p_entropy, - * custom, len, entropy_len) - * implements - * CTR_DRBG_Instantiate(entropy_input, nonce, personalization_string, - * security_strength) -> initial_working_state - * with inputs - * custom[:len] = nonce || personalization_string - * where entropy_input comes from f_entropy for entropy_len bytes - * and with outputs - * ctx = initial_working_state - */ -int mbedtls_ctr_drbg_seed_entropy_len( - mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len, - size_t entropy_len ) -{ - int ret; - unsigned char key[MBEDTLS_CTR_DRBG_KEYSIZE]; - - memset( key, 0, MBEDTLS_CTR_DRBG_KEYSIZE ); - - mbedtls_aes_init( &ctx->aes_ctx ); - - ctx->f_entropy = f_entropy; - ctx->p_entropy = p_entropy; - - ctx->entropy_len = entropy_len; - ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; - - /* - * Initialize with an empty key - */ - if( ( ret = mbedtls_aes_setkey_enc( &ctx->aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS ) ) != 0 ) - { - return( ret ); - } - - if( ( ret = mbedtls_ctr_drbg_reseed( ctx, custom, len ) ) != 0 ) - { - return( ret ); - } - return( 0 ); -} - -int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len ) -{ - return( mbedtls_ctr_drbg_seed_entropy_len( ctx, f_entropy, p_entropy, custom, len, - MBEDTLS_CTR_DRBG_ENTROPY_LEN ) ); -} - void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ) { if( ctx == NULL ) @@ -427,6 +365,68 @@ exit: return( ret ); } +/* + * Non-public function wrapped by mbedtls_ctr_drbg_seed(). Necessary to allow + * NIST tests to succeed (which require known length fixed entropy) + */ +/* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) + * mbedtls_ctr_drbg_seed_entropy_len(ctx, f_entropy, p_entropy, + * custom, len, entropy_len) + * implements + * CTR_DRBG_Instantiate(entropy_input, nonce, personalization_string, + * security_strength) -> initial_working_state + * with inputs + * custom[:len] = nonce || personalization_string + * where entropy_input comes from f_entropy for entropy_len bytes + * and with outputs + * ctx = initial_working_state + */ +int mbedtls_ctr_drbg_seed_entropy_len( + mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), + void *p_entropy, + const unsigned char *custom, + size_t len, + size_t entropy_len ) +{ + int ret; + unsigned char key[MBEDTLS_CTR_DRBG_KEYSIZE]; + + memset( key, 0, MBEDTLS_CTR_DRBG_KEYSIZE ); + + mbedtls_aes_init( &ctx->aes_ctx ); + + ctx->f_entropy = f_entropy; + ctx->p_entropy = p_entropy; + + ctx->entropy_len = entropy_len; + ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; + + /* + * Initialize with an empty key + */ + if( ( ret = mbedtls_aes_setkey_enc( &ctx->aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS ) ) != 0 ) + { + return( ret ); + } + + if( ( ret = mbedtls_ctr_drbg_reseed( ctx, custom, len ) ) != 0 ) + { + return( ret ); + } + return( 0 ); +} + +int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), + void *p_entropy, + const unsigned char *custom, + size_t len ) +{ + return( mbedtls_ctr_drbg_seed_entropy_len( ctx, f_entropy, p_entropy, custom, len, + MBEDTLS_CTR_DRBG_ENTROPY_LEN ) ); +} + /* CTR_DRBG_Generate with derivation function (SP 800-90A §10.2.1.5.2) * mbedtls_ctr_drbg_random_with_add(ctx, output, output_len, additional, add_len) * implements From 912ffe414e62a13f1b1cb25d3e051b776e459fa5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2019 12:15:55 +0200 Subject: [PATCH 03/10] CTR_DRBG: support set_entropy_len() before seed() mbedtls_ctr_drbg_seed() always set the entropy length to the default, so a call to mbedtls_ctr_drbg_set_entropy_len() before seed() had no effect. Change this to the more intuitive behavior that set_entropy_len() sets the entropy length and seed() respects that and only uses the default entropy length if there was no call to set_entropy_len(). The former test-only function mbedtls_ctr_drbg_seed_entropy_len() is no longer used, but keep it for strict ABI compatibility. --- include/mbedtls/ctr_drbg.h | 16 +++---- library/ctr_drbg.c | 53 ++++++++++++----------- tests/suites/test_suite_ctr_drbg.function | 6 +-- 3 files changed, 35 insertions(+), 40 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 766e64701..c14507ff6 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -214,11 +214,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * with mbedtls_entropy_init() (which registers the platform's default * entropy sources). * - * \p f_entropy is always called with a buffer size equal to the entropy - * length. The entropy length is initially #MBEDTLS_CTR_DRBG_ENTROPY_LEN - * and this value is always used for the initial seeding. You can change - * the entropy length for subsequent seeding by calling - * mbedtls_ctr_drbg_set_entropy_len() after this function. + * The entropy length is #MBEDTLS_CTR_DRBG_ENTROPY_LEN by default. + * You can override it by calling mbedtls_ctr_drbg_set_entropy_len(). * * You can provide a personalization string in addition to the * entropy source, to make this instantiation as unique as possible. @@ -255,6 +252,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. + * \p f_entropy is always called with a buffer size + * equal to the entropy length. * \param p_entropy The entropy context to pass to \p f_entropy. * \param custom The personalization string. * This can be \c NULL, in which case the personalization @@ -298,15 +297,10 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, /** * \brief This function sets the amount of entropy grabbed on each - * subsequent reseed. + * seed or reseed. * * The default value is #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * - * \note mbedtls_ctr_drbg_seed() always sets the entropy length - * to #MBEDTLS_CTR_DRBG_ENTROPY_LEN, so this function - * only has an effect when it is called after - * mbedtls_ctr_drbg_seed(). - * * \note The security strength of CTR_DRBG is bounded by the * entropy length. Thus: * - When using AES-256 diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 80544dfb8..32b34e462 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -365,29 +365,22 @@ exit: return( ret ); } -/* - * Non-public function wrapped by mbedtls_ctr_drbg_seed(). Necessary to allow - * NIST tests to succeed (which require known length fixed entropy) - */ /* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) - * mbedtls_ctr_drbg_seed_entropy_len(ctx, f_entropy, p_entropy, - * custom, len, entropy_len) + * mbedtls_ctr_drbg_seed(ctx, f_entropy, p_entropy, custom, len) * implements * CTR_DRBG_Instantiate(entropy_input, nonce, personalization_string, * security_strength) -> initial_working_state * with inputs * custom[:len] = nonce || personalization_string - * where entropy_input comes from f_entropy for entropy_len bytes + * where entropy_input comes from f_entropy for ctx->entropy_len bytes * and with outputs * ctx = initial_working_state */ -int mbedtls_ctr_drbg_seed_entropy_len( - mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len, - size_t entropy_len ) +int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), + void *p_entropy, + const unsigned char *custom, + size_t len ) { int ret; unsigned char key[MBEDTLS_CTR_DRBG_KEYSIZE]; @@ -399,7 +392,8 @@ int mbedtls_ctr_drbg_seed_entropy_len( ctx->f_entropy = f_entropy; ctx->p_entropy = p_entropy; - ctx->entropy_len = entropy_len; + if( ctx->entropy_len == 0 ) + ctx->entropy_len = MBEDTLS_CTR_DRBG_ENTROPY_LEN; ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; /* @@ -417,14 +411,15 @@ int mbedtls_ctr_drbg_seed_entropy_len( return( 0 ); } -int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len ) +/* Backward compatibility wrapper */ +int mbedtls_ctr_drbg_seed_entropy_len( + mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), void *p_entropy, + const unsigned char *custom, size_t len, + size_t entropy_len ) { - return( mbedtls_ctr_drbg_seed_entropy_len( ctx, f_entropy, p_entropy, custom, len, - MBEDTLS_CTR_DRBG_ENTROPY_LEN ) ); + mbedtls_ctr_drbg_set_entropy_len( ctx, entropy_len ); + return( mbedtls_ctr_drbg_seed( ctx, f_entropy, p_entropy, custom, len ) ); } /* CTR_DRBG_Generate with derivation function (SP 800-90A §10.2.1.5.2) @@ -678,8 +673,11 @@ int mbedtls_ctr_drbg_self_test( int verbose ) mbedtls_printf( " CTR_DRBG (PR = TRUE) : " ); test_offset = 0; - CHK( mbedtls_ctr_drbg_seed_entropy_len( &ctx, ctr_drbg_self_test_entropy, - (void *) entropy_source_pr, nonce_pers_pr, 16, 32 ) ); + mbedtls_ctr_drbg_set_entropy_len( &ctx, 32 ); + CHK( mbedtls_ctr_drbg_seed( &ctx, + ctr_drbg_self_test_entropy, + (void *) entropy_source_pr, + nonce_pers_pr, 16 ) ); mbedtls_ctr_drbg_set_prediction_resistance( &ctx, MBEDTLS_CTR_DRBG_PR_ON ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, MBEDTLS_CTR_DRBG_BLOCKSIZE ) ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, MBEDTLS_CTR_DRBG_BLOCKSIZE ) ); @@ -699,8 +697,11 @@ int mbedtls_ctr_drbg_self_test( int verbose ) mbedtls_ctr_drbg_init( &ctx ); test_offset = 0; - CHK( mbedtls_ctr_drbg_seed_entropy_len( &ctx, ctr_drbg_self_test_entropy, - (void *) entropy_source_nopr, nonce_pers_nopr, 16, 32 ) ); + mbedtls_ctr_drbg_set_entropy_len( &ctx, 32 ); + CHK( mbedtls_ctr_drbg_seed( &ctx, + ctr_drbg_self_test_entropy, + (void *) entropy_source_nopr, + nonce_pers_nopr, 16 ) ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, 16 ) ); CHK( mbedtls_ctr_drbg_reseed( &ctx, NULL, 0 ) ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, 16 ) ); diff --git a/tests/suites/test_suite_ctr_drbg.function b/tests/suites/test_suite_ctr_drbg.function index 4a97826f6..01050d92d 100644 --- a/tests/suites/test_suite_ctr_drbg.function +++ b/tests/suites/test_suite_ctr_drbg.function @@ -44,11 +44,11 @@ static void ctr_drbg_validate_internal( int reseed_mode, data_t * nonce, /* CTR_DRBG_Instantiate(entropy[:entropy->len], nonce, perso, ) * where nonce||perso = nonce[nonce->len] */ - TEST_ASSERT( mbedtls_ctr_drbg_seed_entropy_len( + mbedtls_ctr_drbg_set_entropy_len( &ctx, entropy_chunk_len ); + TEST_ASSERT( mbedtls_ctr_drbg_seed( &ctx, mbedtls_test_entropy_func, entropy->x, - nonce->x, nonce->len, - entropy_chunk_len ) == 0 ); + nonce->x, nonce->len ) == 0 ); if( reseed_mode == RESEED_ALWAYS ) mbedtls_ctr_drbg_set_prediction_resistance( &ctx, From 0e59c473cdc8d60510340e8799e9862dc11202c4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 18 Oct 2019 16:40:10 +0200 Subject: [PATCH 04/10] Changelog entry for xxx_drbg_set_entropy_len before xxx_drbg_seed --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index c92e42953..2f2afb7b7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,6 +6,10 @@ Bugfix * Remove redundant line for getting the bitlen of a bignum, since the variable holding the returned value is overwritten a line after. Found by irwir in #2377. + * Support mbedtls_hmac_drbg_set_entropy_len() and + mbedtls_ctr_drbg_set_entropy_len() before the DRBG is seeded. Before, + the initial seeding always reset the entropy length to the compile-time + default. Changes * Add unit tests for AES-GCM when called through mbedtls_cipher_auth_xxx() From f4c0dbc6282dc8f65ef7c23052e24f595b2a9018 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 28 Oct 2019 17:28:46 +0100 Subject: [PATCH 05/10] Fix CTR_DRBG benchmark You can't reuse a CTR_DRBG context without free()ing it and re-init()ing. This generally happened to work, but was never guaranteed. It could have failed with alternative implementations of the AES module because mbedtls_ctr_drbg_seed() calls mbedtls_aes_init() on a context which is already initialized if mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a memory leak. Calling free() and seed() with no intervening init fails when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex representation. So add the missing free() and init(). --- programs/test/benchmark.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 2b8656692..9c06d07b0 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -674,12 +674,13 @@ int main( int argc, char *argv[] ) mbedtls_ctr_drbg_context ctr_drbg; mbedtls_ctr_drbg_init( &ctr_drbg ); - if( mbedtls_ctr_drbg_seed( &ctr_drbg, myrand, NULL, NULL, 0 ) != 0 ) mbedtls_exit(1); TIME_AND_TSC( "CTR_DRBG (NOPR)", mbedtls_ctr_drbg_random( &ctr_drbg, buf, BUFSIZE ) ); + mbedtls_ctr_drbg_free( &ctr_drbg ); + mbedtls_ctr_drbg_init( &ctr_drbg ); if( mbedtls_ctr_drbg_seed( &ctr_drbg, myrand, NULL, NULL, 0 ) != 0 ) mbedtls_exit(1); mbedtls_ctr_drbg_set_prediction_resistance( &ctr_drbg, MBEDTLS_CTR_DRBG_PR_ON ); From b02a2332189749bad75806ab414bd40701c65f44 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 28 Oct 2019 17:33:07 +0100 Subject: [PATCH 06/10] Note that mbedtls_ctr_drbg_seed() must not be called twice You can't reuse a CTR_DRBG context without free()ing it and re-init()ing it. This generally happened to work, but was never guaranteed. It could have failed with alternative implementations of the AES module because mbedtls_ctr_drbg_seed() calls mbedtls_aes_init() on a context which is already initialized if mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a memory leak. Calling free() and seed() with no intervening init fails when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex representation. --- include/mbedtls/ctr_drbg.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index c14507ff6..e0b5ed9c9 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -249,6 +249,13 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); #endif /** * \param ctx The CTR_DRBG context to seed. + * It must have been initialized with + * mbedtls_ctr_drbg_init(). + * After a successful call to mbedtls_ctr_drbg_seed(), + * you may not call mbedtls_ctr_drbg_seed() again on + * the same context unless you call + * mbedtls_ctr_drbg_free() and mbedtls_ctr_drbg_init() + * again first. * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. From 19baefa04f23e9cf338c973208ec3d21498b6970 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 31 Oct 2019 15:07:08 +0100 Subject: [PATCH 07/10] Clarify that the "FATAL" message is expected The test case "Memory buffer small buffer" emits a message "FATAL: verification of first header failed". In this test case, it's actually expected, but it looks weird to see this message from a passing test. Add a comment that states this explicitly, and modify the test description to indicate that the failure is expected, and change the test function name to be more accurate. Fix #309 --- tests/suites/test_suite_memory_buffer_alloc.data | 4 ++-- tests/suites/test_suite_memory_buffer_alloc.function | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_memory_buffer_alloc.data b/tests/suites/test_suite_memory_buffer_alloc.data index d59f1135a..660ca0a37 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.data +++ b/tests/suites/test_suite_memory_buffer_alloc.data @@ -16,8 +16,8 @@ memory_buffer_alloc_free_alloc:100:64:100:100:0:0:0:1:200:0 Memory buffer alloc - Out of Memory test memory_buffer_alloc_oom_test: -Memory buffer small buffer -memory_buffer_small_buffer: +Memory buffer: heap too small (header verification should fail) +memory_buffer_heap_too_small: Memory buffer underalloc memory_buffer_underalloc: diff --git a/tests/suites/test_suite_memory_buffer_alloc.function b/tests/suites/test_suite_memory_buffer_alloc.function index bc034367a..886d22b07 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.function +++ b/tests/suites/test_suite_memory_buffer_alloc.function @@ -232,11 +232,14 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ -void memory_buffer_small_buffer( ) +void memory_buffer_heap_too_small( ) { unsigned char buf[1]; mbedtls_memory_buffer_alloc_init( buf, sizeof( buf ) ); + /* With MBEDTLS_MEMORY_DEBUG enabled, this prints a message + * "FATAL: verification of first header failed". + */ TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() != 0 ); } /* END_CASE */ From 4a55e7c73646f5e7752b2af94fa3b1843b563dea Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 31 Oct 2019 15:07:35 +0100 Subject: [PATCH 08/10] More accurate test case description --- tests/suites/test_suite_memory_buffer_alloc.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_memory_buffer_alloc.data b/tests/suites/test_suite_memory_buffer_alloc.data index 660ca0a37..d780fd41b 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.data +++ b/tests/suites/test_suite_memory_buffer_alloc.data @@ -19,5 +19,5 @@ memory_buffer_alloc_oom_test: Memory buffer: heap too small (header verification should fail) memory_buffer_heap_too_small: -Memory buffer underalloc +Memory buffer: attempt to allocate SIZE_MAX memory_buffer_underalloc: From 554d5d598f045b83b6d7f9fefd389eaa2a8453d4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 31 Oct 2019 15:07:45 +0100 Subject: [PATCH 09/10] Enable more test cases without MBEDTLS_MEMORY_DEBUG None of the test cases in tests_suite_memory_buffer_alloc actually need MBEDTLS_MEMORY_DEBUG. Some have additional checks when MBEDTLS_MEMORY_DEBUG but all are useful even without it. So enable them all and #ifdef out the parts that require DEBUG. --- .../test_suite_memory_buffer_alloc.function | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_memory_buffer_alloc.function b/tests/suites/test_suite_memory_buffer_alloc.function index 886d22b07..cc884c28e 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.function +++ b/tests/suites/test_suite_memory_buffer_alloc.function @@ -29,7 +29,7 @@ void mbedtls_memory_buffer_alloc_self_test( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, int d_bytes, int free_a, int free_b, int free_c, int free_d, int e_bytes, @@ -39,8 +39,11 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, unsigned char *ptr_a = NULL, *ptr_b = NULL, *ptr_c = NULL, *ptr_d = NULL, *ptr_e = NULL, *ptr_f = NULL; +#if defined(MBEDTLS_MEMORY_DEBUG) size_t reported_blocks; - size_t allocated_bytes = 0, reported_bytes; + size_t reported_bytes; +#endif + size_t allocated_bytes = 0; mbedtls_memory_buffer_alloc_init( buf, sizeof( buf ) ); @@ -78,8 +81,10 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, allocated_bytes += d_bytes * sizeof(char); } +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == allocated_bytes ); +#endif if( free_a ) { @@ -117,8 +122,10 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, allocated_bytes -= d_bytes * sizeof(char); } +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == allocated_bytes ); +#endif if( e_bytes > 0 ) { @@ -178,8 +185,10 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, ptr_f = NULL; } +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == 0 ); +#endif TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); @@ -188,12 +197,14 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_alloc_oom_test( ) { unsigned char buf[1024]; unsigned char *ptr_a = NULL, *ptr_b = NULL, *ptr_c = NULL; +#if defined(MBEDTLS_MEMORY_DEBUG) size_t reported_blocks, reported_bytes; +#endif (void)ptr_c; @@ -210,8 +221,10 @@ void memory_buffer_alloc_oom_test( ) ptr_c = mbedtls_calloc( 431, sizeof(char) ); TEST_ASSERT( ptr_c == NULL ); +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes >= 864 && reported_bytes <= sizeof(buf) ); +#endif mbedtls_free( ptr_a ); ptr_a = NULL; @@ -221,8 +234,10 @@ void memory_buffer_alloc_oom_test( ) ptr_b = NULL; TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == 0 ); +#endif TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); @@ -231,7 +246,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_heap_too_small( ) { unsigned char buf[1]; @@ -244,7 +259,7 @@ void memory_buffer_heap_too_small( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_underalloc( ) { unsigned char buf[100]; From 349a079f2dc1ecbd62a1684dc3601442809689ef Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 17 Dec 2019 10:17:20 +0000 Subject: [PATCH 10/10] Fix some pylint warnings Fix a too-long line to meet PEP8 standards --- tests/scripts/mbedtls_test.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/scripts/mbedtls_test.py b/tests/scripts/mbedtls_test.py index 6ac68a4fb..8f24435bf 100755 --- a/tests/scripts/mbedtls_test.py +++ b/tests/scripts/mbedtls_test.py @@ -310,7 +310,10 @@ class MbedTlsTest(BaseHostTest): param_bytes, length = self.test_vector_to_bytes(function_id, dependencies, args) - self.send_kv(''.join('{:02x}'.format(x) for x in length), ''.join('{:02x}'.format(x) for x in param_bytes)) + self.send_kv( + ''.join('{:02x}'.format(x) for x in length), + ''.join('{:02x}'.format(x) for x in param_bytes) + ) @staticmethod def get_result(value):