diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 8b102bb05b..5884bc7968 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2088,34 +2088,54 @@ psa_status_t psa_hash_abort( psa_hash_operation_t *operation ) psa_status_t psa_hash_setup( psa_hash_operation_t *operation, psa_algorithm_t alg ) { + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + /* A context must be freshly initialized before it can be set up. */ if( operation->id != 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( !PSA_ALG_IS_HASH( alg ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } /* Ensure all of the context is zeroized, since PSA_HASH_OPERATION_INIT only * directly zeroes the int-sized dummy member of the context union. */ memset( &operation->ctx, 0, sizeof( operation->ctx ) ); - return( psa_driver_wrapper_hash_setup( operation, alg ) ); + status = psa_driver_wrapper_hash_setup( operation, alg ); + +exit: + if( status != PSA_SUCCESS ) + psa_hash_abort( operation ); + + return status; } psa_status_t psa_hash_update( psa_hash_operation_t *operation, const uint8_t *input, size_t input_length ) { + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } /* Don't require hash implementations to behave correctly on a * zero-length input, which may have an invalid pointer. */ if( input_length == 0 ) return( PSA_SUCCESS ); - psa_status_t status = psa_driver_wrapper_hash_update( operation, - input, input_length ); + status = psa_driver_wrapper_hash_update( operation, input, input_length ); + +exit: if( status != PSA_SUCCESS ) psa_hash_abort( operation ); @@ -2147,13 +2167,24 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation, operation, actual_hash, sizeof( actual_hash ), &actual_hash_length ); + if( status != PSA_SUCCESS ) - return( status ); + goto exit; + if( actual_hash_length != hash_length ) - return( PSA_ERROR_INVALID_SIGNATURE ); + { + status = PSA_ERROR_INVALID_SIGNATURE; + goto exit; + } + if( mbedtls_psa_safer_memcmp( hash, actual_hash, actual_hash_length ) != 0 ) - return( PSA_ERROR_INVALID_SIGNATURE ); - return( PSA_SUCCESS ); + status = PSA_ERROR_INVALID_SIGNATURE; + +exit: + if( status != PSA_SUCCESS ) + psa_hash_abort(operation); + + return( status ); } psa_status_t psa_hash_compute( psa_algorithm_t alg, @@ -2275,11 +2306,14 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; - psa_key_slot_t *slot; + psa_key_slot_t *slot = NULL; /* A context must be freshly initialized before it can be set up. */ if( operation->id != 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } status = psa_get_and_lock_key_slot_with_policy( key, @@ -2287,7 +2321,7 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation, is_sign ? PSA_KEY_USAGE_SIGN_HASH : PSA_KEY_USAGE_VERIFY_HASH, alg ); if( status != PSA_SUCCESS ) - return( status ); + goto exit; psa_key_attributes_t attributes = { .core = slot->attr @@ -2368,29 +2402,37 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; - /* Set the output length and content to a safe default, such that in - * case the caller misses an error check, the output would be an - * unachievable MAC. */ - *mac_length = mac_size; - if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( ! operation->is_sign ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } /* Sanity check. This will guarantee that mac_size != 0 (and so mac != NULL) * once all the error checks are done. */ if( operation->mac_size == 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( mac_size < operation->mac_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); + { + status = PSA_ERROR_BUFFER_TOO_SMALL; + goto exit; + } status = psa_driver_wrapper_mac_sign_finish( operation, mac, operation->mac_size, mac_length ); +exit: /* In case of success, set the potential excess room in the output buffer * to an invalid value, to avoid potentially leaking a longer MAC. * In case of error, set the output length and content to a safe default, @@ -2420,21 +2462,27 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( operation->is_sign ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( operation->mac_size != mac_length ) { status = PSA_ERROR_INVALID_SIGNATURE; - goto cleanup; + goto exit; } status = psa_driver_wrapper_mac_verify_finish( operation, mac, mac_length ); -cleanup: +exit: abort_status = psa_mac_abort( operation ); return( status == PSA_SUCCESS ? abort_status : status ); @@ -3184,18 +3232,24 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; - psa_key_slot_t *slot; + psa_key_slot_t *slot = NULL; psa_key_usage_t usage = ( cipher_operation == MBEDTLS_ENCRYPT ? PSA_KEY_USAGE_ENCRYPT : PSA_KEY_USAGE_DECRYPT ); /* A context must be freshly initialized before it can be set up. */ if( operation->id != 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } /* The requested algorithm must be one that can be processed by cipher. */ if( ! PSA_ALG_IS_CIPHER( alg ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } /* Fetch key material from key storage. */ status = psa_get_and_lock_key_slot_with_policy( key, &slot, usage, alg ); @@ -3265,12 +3319,14 @@ psa_status_t psa_cipher_generate_iv( psa_cipher_operation_t *operation, if( operation->id == 0 ) { - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto exit; } if( operation->iv_set || ! operation->iv_required ) { - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto exit; } if( iv_size < operation->default_iv_length ) @@ -3306,18 +3362,28 @@ psa_status_t psa_cipher_set_iv( psa_cipher_operation_t *operation, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( operation->iv_set || ! operation->iv_required ) - return( PSA_ERROR_BAD_STATE ); + { + status = PSA_ERROR_BAD_STATE; + goto exit; + } if( iv_length > PSA_CIPHER_IV_MAX_SIZE ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } status = psa_driver_wrapper_cipher_set_iv( operation, iv, iv_length ); +exit: if( status == PSA_SUCCESS ) operation->iv_set = 1; else @@ -3336,11 +3402,14 @@ psa_status_t psa_cipher_update( psa_cipher_operation_t *operation, if( operation->id == 0 ) { - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto exit; } + if( operation->iv_required && ! operation->iv_set ) { - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto exit; } status = psa_driver_wrapper_cipher_update( operation, @@ -3349,6 +3418,8 @@ psa_status_t psa_cipher_update( psa_cipher_operation_t *operation, output, output_size, output_length ); + +exit: if( status != PSA_SUCCESS ) psa_cipher_abort( operation ); @@ -3364,17 +3435,22 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, if( operation->id == 0 ) { - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto exit; } + if( operation->iv_required && ! operation->iv_set ) { - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto exit; } status = psa_driver_wrapper_cipher_finish( operation, output, output_size, output_length ); + +exit: if( status == PSA_SUCCESS ) return( psa_cipher_abort( operation ) ); else diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 33e7c95328..41e29729b5 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -19,6 +19,11 @@ /* If this comes up, it's a bug in the test code or in the test data. */ #define UNUSED 0xdeadbeef +/* Assert that an operation is (not) active. + * This serves as a proxy for checking if the operation is aborted. */ +#define ASSERT_OPERATION_IS_ACTIVE( operation ) TEST_ASSERT( operation.id != 0 ) +#define ASSERT_OPERATION_IS_INACTIVE( operation ) TEST_ASSERT( operation.id == 0 ) + /** An invalid export length that will never be set by psa_export_key(). */ static const size_t INVALID_EXPORT_LENGTH = ~0U; @@ -1540,15 +1545,28 @@ void hash_bad_order( ) /* Call setup twice in a row. */ PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_hash_setup( &operation, alg ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_hash_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Call update without calling setup beforehand. */ TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), PSA_ERROR_BAD_STATE ); PSA_ASSERT( psa_hash_abort( &operation ) ); + /* Check that update calls abort on error. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + operation.id = UINT_MAX; + ASSERT_OPERATION_IS_ACTIVE( operation ); + TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); + /* Call update after finish. */ PSA_ASSERT( psa_hash_setup( &operation, alg ) ); PSA_ASSERT( psa_hash_finish( &operation, @@ -1574,11 +1592,14 @@ void hash_bad_order( ) /* Call verify twice in a row. */ PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); PSA_ASSERT( psa_hash_verify( &operation, valid_hash, sizeof( valid_hash ) ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); TEST_EQUAL( psa_hash_verify( &operation, valid_hash, sizeof( valid_hash ) ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_hash_abort( &operation ) ); /* Call finish without calling setup beforehand. */ @@ -1627,8 +1648,12 @@ void hash_verify_bad_args( ) /* psa_hash_verify with a smaller hash than expected */ PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_hash_verify( &operation, hash, expected_size - 1 ), PSA_ERROR_INVALID_SIGNATURE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* psa_hash_verify with a non-matching hash */ PSA_ASSERT( psa_hash_setup( &operation, alg ) ); @@ -1871,9 +1896,12 @@ void mac_bad_order( ) /* Call setup twice in a row. */ PSA_ASSERT( psa_mac_sign_setup( &operation, key, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_mac_sign_setup( &operation, key, alg ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_mac_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Call update after sign finish. */ PSA_ASSERT( psa_mac_sign_setup( &operation, key, alg ) ); @@ -1919,19 +1947,25 @@ void mac_bad_order( ) /* Setup sign but try verify. */ PSA_ASSERT( psa_mac_sign_setup( &operation, key, alg ) ); PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_mac_verify_finish( &operation, verify_mac, sizeof( verify_mac ) ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_mac_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Setup verify but try sign. */ PSA_ASSERT( psa_mac_verify_setup( &operation, key, alg ) ); PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_mac_sign_finish( &operation, sign_mac, sizeof( sign_mac ), &sign_mac_length ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_mac_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_destroy_key( key ) ); @@ -2233,15 +2267,21 @@ void cipher_bad_order( ) /* Call encrypt setup twice in a row. */ PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_cipher_encrypt_setup( &operation, key, alg ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_cipher_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Call decrypt setup twice in a row. */ PSA_ASSERT( psa_cipher_decrypt_setup( &operation, key, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_cipher_decrypt_setup( &operation, key, alg ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_cipher_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Generate an IV without calling setup beforehand. */ TEST_EQUAL( psa_cipher_generate_iv( &operation, @@ -2255,11 +2295,14 @@ void cipher_bad_order( ) PSA_ASSERT( psa_cipher_generate_iv( &operation, buffer, sizeof( buffer ), &length ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_cipher_generate_iv( &operation, buffer, sizeof( buffer ), &length ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_cipher_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Generate an IV after it's already set. */ PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); @@ -2281,10 +2324,13 @@ void cipher_bad_order( ) PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); PSA_ASSERT( psa_cipher_set_iv( &operation, iv, sizeof( iv ) ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_cipher_set_iv( &operation, iv, sizeof( iv ) ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_cipher_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Set an IV after it's already generated. */ PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); @@ -2305,12 +2351,16 @@ void cipher_bad_order( ) PSA_ASSERT( psa_cipher_abort( &operation ) ); /* Call update without an IV where an IV is required. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_cipher_update( &operation, text, sizeof( text ), buffer, sizeof( buffer ), &length ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_cipher_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Call update after finish. */ PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); @@ -2335,10 +2385,13 @@ void cipher_bad_order( ) PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); /* Not calling update means we are encrypting an empty buffer, which is OK * for cipher modes with padding. */ + ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_cipher_finish( &operation, buffer, sizeof( buffer ), &length ), PSA_ERROR_BAD_STATE ); + ASSERT_OPERATION_IS_INACTIVE( operation ); PSA_ASSERT( psa_cipher_abort( &operation ) ); + ASSERT_OPERATION_IS_INACTIVE( operation ); /* Call finish twice in a row. */ PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) );