Merge pull request #3611 from gilles-peskine-arm/psa-coverity-cleanups-202008
Minor fixes in PSA code and tests
diff --git a/library/psa_crypto_storage.c b/library/psa_crypto_storage.c
index 3782053..103c9bb 100644
--- a/library/psa_crypto_storage.c
+++ b/library/psa_crypto_storage.c
@@ -174,7 +174,13 @@
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 );
}
diff --git a/library/psa_its_file.c b/library/psa_its_file.c
index 34a75dc..2fbff20 100644
--- a/library/psa_its_file.c
+++ b/library/psa_its_file.c
@@ -233,7 +233,12 @@
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 );
}
diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data
index d982f81..cd26017 100644
--- a/tests/suites/test_suite_psa_crypto.data
+++ b/tests/suites/test_suite_psa_crypto.data
@@ -799,6 +799,10 @@
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 @@
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
diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index b0b4ed6..665580b 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -3083,6 +3083,7 @@
}
exit:
+ psa_mac_abort( &operation );
psa_destroy_key( handle );
PSA_DONE( );
mbedtls_free( actual_mac );
@@ -3161,6 +3162,7 @@
}
exit:
+ psa_mac_abort( &operation );
psa_destroy_key( handle );
PSA_DONE( );
mbedtls_free( perturbed_mac );
@@ -3241,6 +3243,7 @@
#endif
exit:
+ psa_cipher_abort( &operation );
PSA_DONE( );
}
/* END_CASE */
@@ -3393,6 +3396,7 @@
PSA_ASSERT( psa_destroy_key( handle ) );
exit:
+ psa_cipher_abort( &operation );
PSA_DONE( );
}
/* END_CASE */
@@ -3451,6 +3455,7 @@
}
exit:
+ psa_cipher_abort( &operation );
mbedtls_free( output );
psa_destroy_key( handle );
PSA_DONE( );
@@ -3519,6 +3524,7 @@
output, total_output_length );
exit:
+ psa_cipher_abort( &operation );
mbedtls_free( output );
psa_destroy_key( handle );
PSA_DONE( );
@@ -3590,6 +3596,7 @@
output, total_output_length );
exit:
+ psa_cipher_abort( &operation );
mbedtls_free( output );
psa_destroy_key( handle );
PSA_DONE( );
@@ -3651,6 +3658,7 @@
}
exit:
+ psa_cipher_abort( &operation );
mbedtls_free( output );
psa_destroy_key( handle );
PSA_DONE( );
@@ -3732,6 +3740,8 @@
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 );
@@ -3835,6 +3845,8 @@
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 );
@@ -5697,7 +5709,7 @@
/* 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();
diff --git a/tests/suites/test_suite_psa_crypto_hash.function b/tests/suites/test_suite_psa_crypto_hash.function
index 6c577c0..1bc9331 100644
--- a/tests/suites/test_suite_psa_crypto_hash.function
+++ b/tests/suites/test_suite_psa_crypto_hash.function
@@ -31,6 +31,7 @@
actual_hash, actual_hash_length );
exit:
+ psa_hash_abort( &operation );
PSA_DONE( );
}
/* END_CASE */
@@ -52,6 +53,7 @@
expected_hash->len ) );
exit:
+ psa_hash_abort( &operation );
PSA_DONE( );
}
/* END_CASE */
@@ -95,6 +97,8 @@
} while( len++ != input->len );
exit:
+ psa_hash_abort( &operation );
+ psa_hash_abort( &operation2 );
PSA_DONE( );
}
/* END_CASE */
diff --git a/tests/suites/test_suite_psa_crypto_metadata.function b/tests/suites/test_suite_psa_crypto_metadata.function
index 1ba8466..7c0929e 100644
--- a/tests/suites/test_suite_psa_crypto_metadata.function
+++ b/tests/suites/test_suite_psa_crypto_metadata.function
@@ -57,8 +57,18 @@
TEST_ASSERT( PSA_##flag( alg ) == !! ( ( flags ) & flag ) )
/* Check the parity of value.
- * Return 0 if value has even parity and a nonzero value otherwise. */
-int test_parity( uint32_t 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 a nonzero value if value has even parity and 0 otherwise. */
+int has_even_parity( uint32_t value )
{
value ^= value >> 16;
value ^= value >> 8;
@@ -66,7 +76,7 @@
return( 0x9669 & 1 << ( value & 0xf ) );
}
#define TEST_PARITY( value ) \
- TEST_ASSERT( test_parity( value ) )
+ TEST_ASSERT( has_even_parity( value ) )
void algorithm_classification( psa_algorithm_t alg, unsigned flags )
{
@@ -497,7 +507,7 @@
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 +524,7 @@
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 );
diff --git a/tests/suites/test_suite_psa_its.function b/tests/suites/test_suite_psa_its.function
index b6cc488..a7ce7b1 100644
--- a/tests/suites/test_suite_psa_its.function
+++ b/tests/suites/test_suite_psa_its.function
@@ -40,16 +40,23 @@
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;
}