Merge pull request #3611 from gilles-peskine-arm/psa-coverity-cleanups-202008

Minor fixes in PSA code and tests

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 );
 }
 

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 );
 }
 

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

tests/suites/test_suite_psa_crypto.function

@@ -3083,6 +3083,7 @@ void mac_sign( int key_type_arg,
     }
 
 exit:
+    psa_mac_abort( &operation );
     psa_destroy_key( handle );
     PSA_DONE( );
     mbedtls_free( actual_mac );
@@ -3161,6 +3162,7 @@ void mac_verify( int key_type_arg,
     }
 
 exit:
+    psa_mac_abort( &operation );
     psa_destroy_key( handle );
     PSA_DONE( );
     mbedtls_free( perturbed_mac );
@@ -3241,6 +3243,7 @@ void cipher_setup( int key_type_arg,
 #endif
 
 exit:
+    psa_cipher_abort( &operation );
     PSA_DONE( );
 }
 /* END_CASE */
@@ -3393,6 +3396,7 @@ void cipher_bad_order( )
     PSA_ASSERT( psa_destroy_key( handle ) );
 
 exit:
+    psa_cipher_abort( &operation );
     PSA_DONE( );
 }
 /* END_CASE */
@@ -3451,6 +3455,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( );
@@ -3519,6 +3524,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( );
@@ -3590,6 +3596,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( );
@@ -3651,6 +3658,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( );
@@ -3732,6 +3740,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 );
@@ -3835,6 +3845,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 );
@@ -5697,7 +5709,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();

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 */

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 @@ int test_parity( uint32_t value )
     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 @@ 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 +524,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 );

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 );
+    (void) remove( filename );
-    remove( PSA_ITS_STORAGE_TEMP );
+    (void) remove( PSA_ITS_STORAGE_TEMP );
     uid_max = 0;
 }