From 69c0ea26c74cbdb2adb6f48b2afb3d60c455f73c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 7 Jun 2019 15:38:59 +0200 Subject: [PATCH 1/5] Test suites: cope with psa_crypto_init failure psa_crypto_init() can fail. Do check its return code. Don't call it before initializing local objects that are going to be cleaned up. --- tests/suites/test_suite_pk.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index d85d9ed3d..3282214fb 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -124,11 +124,11 @@ void pk_psa_utils( ) size_t len; mbedtls_pk_debug_item dbg; - TEST_ASSERT( psa_crypto_init() == 0 ); - mbedtls_pk_init( &pk ); mbedtls_pk_init( &pk2 ); + TEST_ASSERT( psa_crypto_init() == 0 ); + TEST_ASSERT( mbedtls_pk_setup_opaque( &pk, 0 ) == MBEDTLS_ERR_PK_BAD_INPUT_DATA ); From 614faa26ac8e1fa978a34480013b3ae57c7f72fe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 7 Jun 2019 15:39:07 +0200 Subject: [PATCH 2/5] Test PSA functions against PSA_SUCCESS, not 0 Writing 0 instead of PSA_SUCCESS is correct, but bad form. --- tests/suites/test_suite_cipher.function | 4 ++-- tests/suites/test_suite_pk.function | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 1ea14088b..209d8e46f 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -982,7 +982,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, #else if( use_psa == 1 ) { - TEST_ASSERT( psa_crypto_init() == 0 ); + TEST_ASSERT( psa_crypto_init( ) == PSA_SUCCESS ); /* PSA requires that the tag immediately follows the ciphertext. */ tmp_cipher = mbedtls_calloc( 1, cipher->len + tag->len ); @@ -1143,7 +1143,7 @@ void test_vec_crypt( int cipher_id, int operation, char *hex_key, #else if( use_psa == 1 ) { - TEST_ASSERT( psa_crypto_init() == 0 ); + TEST_ASSERT( psa_crypto_init( ) == PSA_SUCCESS ); TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( &ctx, mbedtls_cipher_info_from_type( cipher_id ), 0 ) ); } diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 3282214fb..98c5a8321 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -127,7 +127,7 @@ void pk_psa_utils( ) mbedtls_pk_init( &pk ); mbedtls_pk_init( &pk2 ); - TEST_ASSERT( psa_crypto_init() == 0 ); + TEST_ASSERT( psa_crypto_init( ) == PSA_SUCCESS ); TEST_ASSERT( mbedtls_pk_setup_opaque( &pk, 0 ) == MBEDTLS_ERR_PK_BAD_INPUT_DATA ); From 9bb1f64706d98efc7a584ba19403ad7ffa398e8d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 7 Jun 2019 17:10:39 +0200 Subject: [PATCH 3/5] Don't call memset after calloc memset has undefined behavior when either pointer can be NULL, which is the case when it's the result of malloc/calloc with a size of 0. The memset calls here are useless anyway since they come immediately after calloc. --- tests/suites/test_suite_nist_kw.function | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/suites/test_suite_nist_kw.function b/tests/suites/test_suite_nist_kw.function index f1acde91a..9c34ea619 100644 --- a/tests/suites/test_suite_nist_kw.function +++ b/tests/suites/test_suite_nist_kw.function @@ -170,10 +170,6 @@ void nist_kw_plaintext_lengths( int in_len, int out_len, int mode, int res ) TEST_ASSERT( ciphertext != NULL ); } - memset( plaintext, 0, in_len ); - memset( ciphertext, 0, output_len ); - - TEST_ASSERT( mbedtls_nist_kw_setkey( &ctx, MBEDTLS_CIPHER_ID_AES, key, 8 * sizeof( key ), 1 ) == 0 ); @@ -225,10 +221,6 @@ void nist_kw_ciphertext_lengths( int in_len, int out_len, int mode, int res ) TEST_ASSERT( ciphertext != NULL ); } - memset( plaintext, 0, output_len ); - memset( ciphertext, 0, in_len ); - - TEST_ASSERT( mbedtls_nist_kw_setkey( &ctx, MBEDTLS_CIPHER_ID_AES, key, 8 * sizeof( key ), 0 ) == 0 ); unwrap_ret = mbedtls_nist_kw_unwrap( &ctx, mode, ciphertext, in_len, From e39b903de54c6c92cd89c81116bb80a3cc7f019c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Jun 2019 19:31:29 +0200 Subject: [PATCH 4/5] entropy_nv_seed: clean up properly Call mbedtls_entropy_free on test failure. Restore the previous NV seed functions which the call to mbedtls_platform_set_nv_seed() changed. This didn't break anything, but only because the NV seed functions used for these tests happened to work for the tests that got executed later in the .data file. --- tests/suites/test_suite_entropy.function | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/suites/test_suite_entropy.function b/tests/suites/test_suite_entropy.function index 0b1cfe80d..35efc7800 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -306,6 +306,10 @@ void entropy_nv_seed( data_t * read_seed ) { mbedtls_sha512_context accumulator; mbedtls_entropy_context ctx; + int (*original_mbedtls_nv_seed_read)( unsigned char *buf, size_t buf_len ) = + mbedtls_nv_seed_read; + int (*original_mbedtls_nv_seed_write)( unsigned char *buf, size_t buf_len ) = + mbedtls_nv_seed_write; unsigned char header[2]; unsigned char entropy[MBEDTLS_ENTROPY_BLOCK_SIZE]; @@ -372,7 +376,10 @@ void entropy_nv_seed( data_t * read_seed ) TEST_ASSERT( memcmp( check_seed, buffer_seed, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); TEST_ASSERT( memcmp( check_entropy, entropy, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); +exit: mbedtls_entropy_free( &ctx ); + mbedtls_nv_seed_read = original_mbedtls_nv_seed_read; + mbedtls_nv_seed_write = original_mbedtls_nv_seed_write; } /* END_CASE */ From 66afcca5a9f94abe6d9b8b9b9e2349efdc649e29 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Jun 2019 19:33:42 +0200 Subject: [PATCH 5/5] entropy_nv_seed: cope with SHA-256 This test case was only executed if the SHA-512 module was enabled and MBEDTLS_ENTROPY_FORCE_SHA256 was not enabled, so "config.pl full" didn't have a chance to reach it even if that enabled MBEDTLS_PLATFORM_NV_SEED_ALT. Now all it takes to enable this test is MBEDTLS_PLATFORM_NV_SEED_ALT and its requirements, and the near-ubiquitous MD module. --- tests/suites/test_suite_entropy.function | 59 +++++++++++++++--------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/tests/suites/test_suite_entropy.function b/tests/suites/test_suite_entropy.function index 35efc7800..31722a2f3 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -301,10 +301,19 @@ void entropy_nv_seed_std_io( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_ENTROPY_NV_SEED:MBEDTLS_PLATFORM_NV_SEED_ALT:MBEDTLS_ENTROPY_SHA512_ACCUMULATOR */ +/* BEGIN_CASE depends_on:MBEDTLS_MD_C:MBEDTLS_ENTROPY_NV_SEED:MBEDTLS_PLATFORM_NV_SEED_ALT */ void entropy_nv_seed( data_t * read_seed ) { - mbedtls_sha512_context accumulator; +#if defined(MBEDTLS_ENTROPY_SHA512_ACCUMULATOR) + const mbedtls_md_info_t *md_info = + mbedtls_md_info_from_type( MBEDTLS_MD_SHA512 ); +#elif defined(MBEDTLS_ENTROPY_SHA256_ACCUMULATOR) + const mbedtls_md_info_t *md_info = + mbedtls_md_info_from_type( MBEDTLS_MD_SHA256 ); +#else +#error "Unsupported entropy accumulator" +#endif + mbedtls_md_context_t accumulator; mbedtls_entropy_context ctx; int (*original_mbedtls_nv_seed_read)( unsigned char *buf, size_t buf_len ) = mbedtls_nv_seed_read; @@ -320,17 +329,14 @@ void entropy_nv_seed( data_t * read_seed ) memset( entropy, 0, MBEDTLS_ENTROPY_BLOCK_SIZE ); memset( buf, 0, MBEDTLS_ENTROPY_BLOCK_SIZE ); - memset( buffer_seed, 0, MBEDTLS_ENTROPY_BLOCK_SIZE ); memset( empty, 0, MBEDTLS_ENTROPY_BLOCK_SIZE ); memset( check_seed, 2, MBEDTLS_ENTROPY_BLOCK_SIZE ); memset( check_entropy, 3, MBEDTLS_ENTROPY_BLOCK_SIZE ); - // Set the initial NV seed to read - memcpy( buffer_seed, read_seed->x, read_seed->len ); - // Make sure we read/write NV seed from our buffers mbedtls_platform_set_nv_seed( buffer_nv_seed_read, buffer_nv_seed_write ); + mbedtls_md_init( &accumulator ); mbedtls_entropy_init( &ctx ); entropy_clear_sources( &ctx ); @@ -338,45 +344,54 @@ void entropy_nv_seed( data_t * read_seed ) MBEDTLS_ENTROPY_BLOCK_SIZE, MBEDTLS_ENTROPY_SOURCE_STRONG ) == 0 ); + // Set the initial NV seed to read + TEST_ASSERT( read_seed->len >= MBEDTLS_ENTROPY_BLOCK_SIZE ); + memcpy( buffer_seed, read_seed->x, MBEDTLS_ENTROPY_BLOCK_SIZE ); + // Do an entropy run TEST_ASSERT( mbedtls_entropy_func( &ctx, entropy, sizeof( entropy ) ) == 0 ); - // Determine what should have happened with manual entropy internal logic - // Only use the SHA-512 version to check // Init accumulator header[1] = MBEDTLS_ENTROPY_BLOCK_SIZE; - mbedtls_sha512_starts( &accumulator, 0 ); + TEST_ASSERT( mbedtls_md_setup( &accumulator, md_info, 0 ) == 0 ); // First run for updating write_seed header[0] = 0; - mbedtls_sha512_update( &accumulator, header, 2 ); - mbedtls_sha512_update( &accumulator, read_seed->x, read_seed->len ); - mbedtls_sha512_finish( &accumulator, buf ); + TEST_ASSERT( mbedtls_md_starts( &accumulator ) == 0 ); + TEST_ASSERT( mbedtls_md_update( &accumulator, header, 2 ) == 0 ); + TEST_ASSERT( mbedtls_md_update( &accumulator, + read_seed->x, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); + TEST_ASSERT( mbedtls_md_finish( &accumulator, buf ) == 0 ); - memset( &accumulator, 0, sizeof( mbedtls_sha512_context ) ); - mbedtls_sha512_starts( &accumulator, 0 ); - mbedtls_sha512_update( &accumulator, buf, MBEDTLS_ENTROPY_BLOCK_SIZE ); + TEST_ASSERT( mbedtls_md_starts( &accumulator ) == 0 ); + TEST_ASSERT( mbedtls_md_update( &accumulator, + buf, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); - mbedtls_sha512( buf, MBEDTLS_ENTROPY_BLOCK_SIZE, check_seed, 0 ); + TEST_ASSERT( mbedtls_md( md_info, buf, MBEDTLS_ENTROPY_BLOCK_SIZE, + check_seed ) == 0 ); // Second run for actual entropy (triggers mbedtls_entropy_update_nv_seed) header[0] = MBEDTLS_ENTROPY_SOURCE_MANUAL; - mbedtls_sha512_update( &accumulator, header, 2 ); - mbedtls_sha512_update( &accumulator, empty, MBEDTLS_ENTROPY_BLOCK_SIZE ); + TEST_ASSERT( mbedtls_md_update( &accumulator, header, 2 ) == 0 ); + TEST_ASSERT( mbedtls_md_update( &accumulator, + empty, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); header[0] = 0; - mbedtls_sha512_update( &accumulator, header, 2 ); - mbedtls_sha512_update( &accumulator, check_seed, MBEDTLS_ENTROPY_BLOCK_SIZE ); - mbedtls_sha512_finish( &accumulator, buf ); + TEST_ASSERT( mbedtls_md_update( &accumulator, header, 2 ) == 0 ); + TEST_ASSERT( mbedtls_md_update( &accumulator, + check_seed, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); + TEST_ASSERT( mbedtls_md_finish( &accumulator, buf ) == 0 ); - mbedtls_sha512( buf, MBEDTLS_ENTROPY_BLOCK_SIZE, check_entropy, 0 ); + TEST_ASSERT( mbedtls_md( md_info, buf, MBEDTLS_ENTROPY_BLOCK_SIZE, + check_entropy ) == 0 ); // Check result of both NV file and entropy received with the manual calculations TEST_ASSERT( memcmp( check_seed, buffer_seed, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); TEST_ASSERT( memcmp( check_entropy, entropy, MBEDTLS_ENTROPY_BLOCK_SIZE ) == 0 ); exit: + mbedtls_md_free( &accumulator ); mbedtls_entropy_free( &ctx ); mbedtls_nv_seed_read = original_mbedtls_nv_seed_read; mbedtls_nv_seed_write = original_mbedtls_nv_seed_write;