From 14613bcd75614408448079f27fb9aa79f6be847e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 22:30:31 +0200 Subject: [PATCH 01/11] Fix parity tests to actually fail the test on error Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_metadata.function | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_metadata.function b/tests/suites/test_suite_psa_crypto_metadata.function index 1ba846695..96d3afb3d 100644 --- a/tests/suites/test_suite_psa_crypto_metadata.function +++ b/tests/suites/test_suite_psa_crypto_metadata.function @@ -58,7 +58,7 @@ /* Check the parity of value. * Return 0 if value has even parity and a nonzero value otherwise. */ -int test_parity( uint32_t value ) +int check_parity( uint32_t value ) { value ^= value >> 16; value ^= value >> 8; @@ -66,7 +66,7 @@ int test_parity( uint32_t value ) return( 0x9669 & 1 << ( value & 0xf ) ); } #define TEST_PARITY( value ) \ - TEST_ASSERT( test_parity( value ) ) + TEST_ASSERT( check_parity( value ) ) void algorithm_classification( psa_algorithm_t alg, unsigned flags ) { @@ -497,7 +497,7 @@ void ecc_key_family( int curve_arg ) psa_key_type_t public_type = PSA_KEY_TYPE_ECC_PUBLIC_KEY( curve ); psa_key_type_t pair_type = PSA_KEY_TYPE_ECC_KEY_PAIR( curve ); - test_parity( curve ); + TEST_PARITY( curve ); test_key_type( public_type, KEY_TYPE_IS_ECC | KEY_TYPE_IS_PUBLIC_KEY ); test_key_type( pair_type, KEY_TYPE_IS_ECC | KEY_TYPE_IS_KEY_PAIR ); @@ -514,7 +514,7 @@ void dh_key_family( int group_arg ) psa_key_type_t public_type = PSA_KEY_TYPE_DH_PUBLIC_KEY( group ); psa_key_type_t pair_type = PSA_KEY_TYPE_DH_KEY_PAIR( group ); - test_parity( group ); + TEST_PARITY( group ); test_key_type( public_type, KEY_TYPE_IS_DH | KEY_TYPE_IS_PUBLIC_KEY ); test_key_type( pair_type, KEY_TYPE_IS_DH | KEY_TYPE_IS_KEY_PAIR ); From bab1b5204889ffcc22ac57807ea85817e3a55d39 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 22:49:19 +0200 Subject: [PATCH 02/11] psa_its: Annotate file removal after a failed creation Let static analyzers know that it's ok if remove() fails here. Signed-off-by: Gilles Peskine --- library/psa_its_file.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/library/psa_its_file.c b/library/psa_its_file.c index 34a75dc69..2fbff20ef 100644 --- a/library/psa_its_file.c +++ b/library/psa_its_file.c @@ -233,7 +233,12 @@ exit: if( rename_replace_existing( PSA_ITS_STORAGE_TEMP, filename ) != 0 ) status = PSA_ERROR_STORAGE_FAILURE; } - remove( PSA_ITS_STORAGE_TEMP ); + /* The temporary file may still exist, but only in failure cases where + * we're already reporting an error. So there's nothing we can do on + * failure. If the function succeeded, and in some error cases, the + * temporary file doesn't exist and so remove() is expected to fail. + * Thus we just ignore the return status of remove(). */ + (void) remove( PSA_ITS_STORAGE_TEMP ); return( status ); } From 169ca7f06d99d3e8e4db06c56474f90a8c357be1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 22:50:06 +0200 Subject: [PATCH 03/11] psa_crypto_storage: Annotate file removal after a failed creation Let static analyzers know that it's ok if psa_its_remove() fails here. Signed-off-by: Gilles Peskine --- library/psa_crypto_storage.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto_storage.c b/library/psa_crypto_storage.c index 37820533f..103c9bbb8 100644 --- a/library/psa_crypto_storage.c +++ b/library/psa_crypto_storage.c @@ -174,7 +174,13 @@ static psa_status_t psa_crypto_storage_store( const psa_key_file_id_t key, exit: if( status != PSA_SUCCESS ) - psa_its_remove( data_identifier ); + { + /* Remove the file in case we managed to create it but something + * went wrong. It's ok if the file doesn't exist. If the file exists + * but the removal fails, we're already reporting an error so there's + * nothing else we can do. */ + (void) psa_its_remove( data_identifier ); + } return( status ); } From a09713c7956b300382e5ce4b7cea3b08cf5553e4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 22:50:18 +0200 Subject: [PATCH 04/11] test cleanup: Annotate file removal after a failed creation Let static analyzers know that it's ok if remove() fails here. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_its.function | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_psa_its.function b/tests/suites/test_suite_psa_its.function index b6cc488a6..a7ce7b1d4 100644 --- a/tests/suites/test_suite_psa_its.function +++ b/tests/suites/test_suite_psa_its.function @@ -40,16 +40,23 @@ static psa_storage_uid_t uid_max = 0; static void cleanup( void ) { + /* Call remove() on all the files that a test might have created. + * We ignore the error if the file exists but remove() fails because + * it can't be checked portably (except by attempting to open the file + * first, which is needlessly slow and complicated here). A failure of + * remove() on an existing file is very unlikely anyway and would not + * have significant consequences other than perhaps failing the next + * test case. */ char filename[PSA_ITS_STORAGE_FILENAME_LENGTH]; psa_storage_uid_t uid; for( uid = 0; uid < uid_max; uid++ ) { psa_its_fill_filename( uid, filename ); - remove( filename ); + (void) remove( filename ); } psa_its_fill_filename( (psa_storage_uid_t)( -1 ), filename ); - remove( filename ); - remove( PSA_ITS_STORAGE_TEMP ); + (void) remove( filename ); + (void) remove( PSA_ITS_STORAGE_TEMP ); uid_max = 0; } From 64f13ef6ab5f642f597ff45252cfc008e2d51673 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 23:15:20 +0200 Subject: [PATCH 05/11] Add missing cleanup to some multipart operation tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index f4b9a8f67..635114137 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -3066,6 +3066,7 @@ void mac_sign( int key_type_arg, sizeof( actual_mac ) - mac_length ) ); exit: + psa_mac_abort( &operation ); psa_destroy_key( handle ); PSA_DONE( ); } @@ -3104,6 +3105,7 @@ void mac_verify( int key_type_arg, expected_mac->len ) ); exit: + psa_mac_abort( &operation ); psa_destroy_key( handle ); PSA_DONE( ); } @@ -3183,6 +3185,7 @@ void cipher_setup( int key_type_arg, #endif exit: + psa_cipher_abort( &operation ); PSA_DONE( ); } /* END_CASE */ @@ -3335,6 +3338,7 @@ void cipher_bad_order( ) PSA_ASSERT( psa_destroy_key( handle ) ); exit: + psa_cipher_abort( &operation ); PSA_DONE( ); } /* END_CASE */ @@ -3393,6 +3397,7 @@ void cipher_encrypt( int alg_arg, int key_type_arg, } exit: + psa_cipher_abort( &operation ); mbedtls_free( output ); psa_destroy_key( handle ); PSA_DONE( ); @@ -3461,6 +3466,7 @@ void cipher_encrypt_multipart( int alg_arg, int key_type_arg, output, total_output_length ); exit: + psa_cipher_abort( &operation ); mbedtls_free( output ); psa_destroy_key( handle ); PSA_DONE( ); @@ -3532,6 +3538,7 @@ void cipher_decrypt_multipart( int alg_arg, int key_type_arg, output, total_output_length ); exit: + psa_cipher_abort( &operation ); mbedtls_free( output ); psa_destroy_key( handle ); PSA_DONE( ); @@ -3593,6 +3600,7 @@ void cipher_decrypt( int alg_arg, int key_type_arg, } exit: + psa_cipher_abort( &operation ); mbedtls_free( output ); psa_destroy_key( handle ); PSA_DONE( ); @@ -3674,6 +3682,8 @@ void cipher_verify_output( int alg_arg, int key_type_arg, ASSERT_COMPARE( input->x, input->len, output2, output2_length ); exit: + psa_cipher_abort( &operation1 ); + psa_cipher_abort( &operation2 ); mbedtls_free( output1 ); mbedtls_free( output2 ); psa_destroy_key( handle ); @@ -3777,6 +3787,8 @@ void cipher_verify_output_multipart( int alg_arg, ASSERT_COMPARE( input->x, input->len, output2, output2_length ); exit: + psa_cipher_abort( &operation1 ); + psa_cipher_abort( &operation2 ); mbedtls_free( output1 ); mbedtls_free( output2 ); psa_destroy_key( handle ); From e92c68a8786ae82302e2b9877ef86de4691ae620 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 26 Aug 2020 00:06:25 +0200 Subject: [PATCH 06/11] Note that a failure in cleanup is intentional In the cleanup code for persistent_key_load_key_from_storage(), we only attempt to reopen the key so that it will be deleted if it exists at that point. It's intentional that we do nothing if psa_open_key() fails here. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 635114137..e48cb9054 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -5651,7 +5651,7 @@ exit: /* In case there was a test failure after creating the persistent key * but while it was not open, try to re-open the persistent key * to delete it. */ - psa_open_key( key_id, &handle ); + (void) psa_open_key( key_id, &handle ); } psa_destroy_key( handle ); PSA_DONE(); From cd65f4ccac29dc64b6fb045a63bc9e8af6738159 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 23:11:21 +0200 Subject: [PATCH 07/11] Add empty-output-buffer test cases for single-part hash functions Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.data | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index d982f81f6..cd2601796 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -799,6 +799,10 @@ hash_compute_fail:PSA_ALG_ANY_HASH:"":32:PSA_ERROR_NOT_SUPPORTED PSA hash compute: bad algorithm (not a hash) hash_compute_fail:PSA_ALG_HMAC(PSA_ALG_SHA_256):"":32:PSA_ERROR_INVALID_ARGUMENT +PSA hash compute: output buffer empty +depends_on:MBEDTLS_SHA256_C +hash_compute_fail:PSA_ALG_SHA_256:"":0:PSA_ERROR_BUFFER_TOO_SMALL + PSA hash compute: output buffer too small depends_on:MBEDTLS_SHA256_C hash_compute_fail:PSA_ALG_SHA_256:"":31:PSA_ERROR_BUFFER_TOO_SMALL @@ -828,6 +832,10 @@ PSA hash compare: truncated hash depends_on:MBEDTLS_SHA256_C hash_compare_fail:PSA_ALG_SHA_256:"":"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b8":PSA_ERROR_INVALID_SIGNATURE +PSA hash compare: empty hash +depends_on:MBEDTLS_SHA256_C +hash_compare_fail:PSA_ALG_SHA_256:"":"":PSA_ERROR_INVALID_SIGNATURE + PSA hash compare: good depends_on:MBEDTLS_SHA256_C hash_compare_fail:PSA_ALG_SHA_256:"":"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855":PSA_SUCCESS From 34f063ca4761fb95e6060eeda26544f9f8c2c65d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 26 Aug 2020 10:24:13 +0200 Subject: [PATCH 08/11] Add missing cleanup to hash multipart operation tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_hash.function | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_hash.function b/tests/suites/test_suite_psa_crypto_hash.function index 6c577c06a..1bc93313a 100644 --- a/tests/suites/test_suite_psa_crypto_hash.function +++ b/tests/suites/test_suite_psa_crypto_hash.function @@ -31,6 +31,7 @@ void hash_finish( int alg_arg, data_t *input, data_t *expected_hash ) actual_hash, actual_hash_length ); exit: + psa_hash_abort( &operation ); PSA_DONE( ); } /* END_CASE */ @@ -52,6 +53,7 @@ void hash_verify( int alg_arg, data_t *input, data_t *expected_hash ) expected_hash->len ) ); exit: + psa_hash_abort( &operation ); PSA_DONE( ); } /* END_CASE */ @@ -95,6 +97,8 @@ void hash_multi_part( int alg_arg, data_t *input, data_t *expected_hash ) } while( len++ != input->len ); exit: + psa_hash_abort( &operation ); + psa_hash_abort( &operation2 ); PSA_DONE( ); } /* END_CASE */ From 6c75152b9f1a5dac1d92b793cb0c1d93b161c690 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 26 Aug 2020 10:24:26 +0200 Subject: [PATCH 09/11] Explain the purpose of check_parity Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_metadata.function | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_metadata.function b/tests/suites/test_suite_psa_crypto_metadata.function index 96d3afb3d..abee922e7 100644 --- a/tests/suites/test_suite_psa_crypto_metadata.function +++ b/tests/suites/test_suite_psa_crypto_metadata.function @@ -57,6 +57,16 @@ TEST_ASSERT( PSA_##flag( alg ) == !! ( ( flags ) & flag ) ) /* Check the parity of value. + * + * There are several numerical encodings for which the PSA Cryptography API + * specification deliberately defines encodings that all have the same + * parity. This way, a data glitch that flips one bit in the data cannot + * possibly turn a valid encoding into another valid encoding. Here in + * the tests, we check that the values (including Mbed TLS vendor-specific + * values) have the expected parity. + * + * The expected parity is even so that 0 is considered a valid encoding. + * * Return 0 if value has even parity and a nonzero value otherwise. */ int check_parity( uint32_t value ) { From ed9fbc6443caf4b357145d362075321082be1641 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 26 Aug 2020 11:16:50 +0200 Subject: [PATCH 10/11] Clearer function name for parity check Return a name that more clearly returns nonzero=true=good, 0=bad. We'd normally expect check_xxx to return 0=pass, nonzero=fail so check_parity was a bad name. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_metadata.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_metadata.function b/tests/suites/test_suite_psa_crypto_metadata.function index abee922e7..2c069835a 100644 --- a/tests/suites/test_suite_psa_crypto_metadata.function +++ b/tests/suites/test_suite_psa_crypto_metadata.function @@ -68,7 +68,7 @@ * The expected parity is even so that 0 is considered a valid encoding. * * Return 0 if value has even parity and a nonzero value otherwise. */ -int check_parity( uint32_t value ) +int has_even_parity( uint32_t value ) { value ^= value >> 16; value ^= value >> 8; @@ -76,7 +76,7 @@ int check_parity( uint32_t value ) return( 0x9669 & 1 << ( value & 0xf ) ); } #define TEST_PARITY( value ) \ - TEST_ASSERT( check_parity( value ) ) + TEST_ASSERT( has_even_parity( value ) ) void algorithm_classification( psa_algorithm_t alg, unsigned flags ) { From a2e518daf51de2cb548786b37a7d6e55ac8ffd67 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 26 Aug 2020 12:14:37 +0200 Subject: [PATCH 11/11] Fix the documentation of has_even_parity The documentation had the boolean meaning of the return value inverted. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_metadata.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto_metadata.function b/tests/suites/test_suite_psa_crypto_metadata.function index 2c069835a..7c0929e29 100644 --- a/tests/suites/test_suite_psa_crypto_metadata.function +++ b/tests/suites/test_suite_psa_crypto_metadata.function @@ -67,7 +67,7 @@ * * The expected parity is even so that 0 is considered a valid encoding. * - * Return 0 if value has even parity and a nonzero value otherwise. */ + * Return a nonzero value if value has even parity and 0 otherwise. */ int has_even_parity( uint32_t value ) { value ^= value >> 16;