From 33b58eeb36d60b4c01afedeb9b9c6a65c7734997 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 23 Jun 2021 12:48:52 +0100 Subject: [PATCH 01/10] Fix error in psa_crypto test suite The cipher_bad_order test happened to pass, but was not testing the failure case it intended to test. Signed-off-by: Dave Rodgman --- tests/suites/test_suite_psa_crypto.function | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 678cb777a..02e1bb0e8 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -2366,6 +2366,7 @@ 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 ) ); TEST_EQUAL( psa_cipher_update( &operation, text, sizeof( text ), buffer, sizeof( buffer ), From 34b147d1e6db67bfd8b34edf5b18630628790935 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 23 Jun 2021 12:49:59 +0100 Subject: [PATCH 02/10] Add negative tests for psa_abort in cipher and mac functions Various functions for PSA cipher and mac operations call abort on failure; test that this is done. The PSA spec does not require this behaviour, but it makes our implementation more robust in case the user does not abort the operation as required by the PSA spec. Signed-off-by: Dave Rodgman --- tests/suites/test_suite_psa_crypto.function | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 02e1bb0e8..b4495f041 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; @@ -1980,19 +1985,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 ) ); @@ -2316,11 +2327,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 ) ); @@ -2342,10 +2356,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 ) ); @@ -2367,12 +2384,15 @@ void cipher_bad_order( ) /* 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 ) ); @@ -2397,10 +2417,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 ) ); From c88b0a57daea936e7864fba7c3bd8f6925663bd4 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 23 Jun 2021 11:38:39 +0100 Subject: [PATCH 03/10] Update cipher and mac functions to abort on error Signed-off-by: Dave Rodgman --- library/psa_crypto.c | 80 +++++++++++++++++++++++++++++++------------- 1 file changed, 56 insertions(+), 24 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 016c24a90..de625ad2f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2454,19 +2454,27 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, * unachievable MAC. */ *mac_length = mac_size; - if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + if( operation->id == 0 ) { + status = PSA_ERROR_BAD_STATE; + goto cleanup; + } - if( ! operation->is_sign ) - return( PSA_ERROR_BAD_STATE ); + if( ! operation->is_sign ) { + status = PSA_ERROR_BAD_STATE; + goto cleanup; + } /* 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 ); + if( operation->mac_size == 0 ) { + status = PSA_ERROR_BAD_STATE; + goto cleanup; + } - if( mac_size < operation->mac_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); + if( mac_size < operation->mac_size ) { + status = PSA_ERROR_BUFFER_TOO_SMALL; + goto cleanup; + } status = psa_driver_wrapper_mac_sign_finish( operation, mac, operation->mac_size, @@ -2488,6 +2496,7 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, memset( &mac[operation->mac_size], '!', mac_size - operation->mac_size ); +cleanup: abort_status = psa_mac_abort( operation ); return( status == PSA_SUCCESS ? abort_status : status ); @@ -2500,11 +2509,15 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; - if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + if( operation->id == 0 ) { + status = PSA_ERROR_BAD_STATE; + goto cleanup; + } - if( operation->is_sign ) - return( PSA_ERROR_BAD_STATE ); + if( operation->is_sign ) { + status = PSA_ERROR_BAD_STATE; + goto cleanup; + } if( operation->mac_size != mac_length ) { @@ -3341,12 +3354,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 ) @@ -3381,19 +3396,26 @@ 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 ); + if( operation->id == 0 ) { + status = PSA_ERROR_BAD_STATE; + goto exit; + } - if( operation->iv_set || ! operation->iv_required ) - return( PSA_ERROR_BAD_STATE ); + if( operation->iv_set || ! operation->iv_required ) { + status = PSA_ERROR_BAD_STATE; + goto exit; + } - if( iv_length > PSA_CIPHER_IV_MAX_SIZE ) - return( PSA_ERROR_INVALID_ARGUMENT ); + if( iv_length > PSA_CIPHER_IV_MAX_SIZE ) { + 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 @@ -3412,11 +3434,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, @@ -3425,6 +3450,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 ); @@ -3440,17 +3467,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 From ff8d52b398bc491ca0960e49941cfe4877549ce6 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 11:36:14 +0100 Subject: [PATCH 04/10] Add negative tests for psa_abort in hash functions Various functions for PSA hash operations call abort on failure; test that this is done. The PSA spec does not require this behaviour, but it makes our implementation more robust in case the user does not abort the operation as required by the PSA spec. Signed-off-by: Dave Rodgman --- tests/suites/test_suite_psa_crypto.function | 29 +++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index b4495f041..5c4d2f597 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1606,15 +1606,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.ctx.mbedtls_ctx.alg = PSA_ALG_XTS; + 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, @@ -1640,11 +1653,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. */ @@ -1693,8 +1709,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 ) ); @@ -1937,9 +1957,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 ) ); @@ -2305,15 +2328,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, From 4e0a82e274d404faa8320b1eec7fbad990983785 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 11:52:23 +0100 Subject: [PATCH 05/10] Update multipart hash operations to abort on error Signed-off-by: Dave Rodgman --- library/psa_crypto.c | 57 ++++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index de625ad2f..99fc751f6 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2169,34 +2169,51 @@ 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 ) { - /* A context must be freshly initialized before it can be set up. */ - if( operation->id != 0 ) - return( PSA_ERROR_BAD_STATE ); + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - if( !PSA_ALG_IS_HASH( alg ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); + /* A context must be freshly initialized before it can be set up. */ + if( operation->id != 0 ) { + status = PSA_ERROR_BAD_STATE; + goto exit; + } + + if( !PSA_ALG_IS_HASH( alg ) ) { + 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 ) { - if( operation->id == 0 ) - return( PSA_ERROR_BAD_STATE ); + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + + if( operation->id == 0 ) { + 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 ); @@ -2228,13 +2245,23 @@ 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 ); - if( actual_hash_length != hash_length ) - return( PSA_ERROR_INVALID_SIGNATURE ); + goto exit; + + if( actual_hash_length != hash_length ) { + 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, From cccb05def4bd71d2ef6582051770a3f9572ad49c Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 11:52:47 +0100 Subject: [PATCH 06/10] Call abort on error in psa_mac/cipher setup Signed-off-by: Dave Rodgman --- library/psa_crypto.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 99fc751f6..001f884a7 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2383,11 +2383,13 @@ 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 ); + if( operation->id != 0 ) { + status = PSA_ERROR_BAD_STATE; + goto exit; + } status = psa_get_and_lock_key_slot_with_policy( key, @@ -2395,7 +2397,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 @@ -3300,18 +3302,22 @@ 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 ); + if( operation->id != 0 ) { + 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 ); + if( ! PSA_ALG_IS_CIPHER( alg ) ) { + 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 ); From db861797c1ca04e7be5b7c0aeb7e99de423e2d94 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 16:17:43 +0100 Subject: [PATCH 07/10] Correct coding style issues Signed-off-by: Dave Rodgman --- library/psa_crypto.c | 47 +++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 001f884a7..c0f419c43 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2172,12 +2172,14 @@ psa_status_t psa_hash_setup( psa_hash_operation_t *operation, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; /* A context must be freshly initialized before it can be set up. */ - if( operation->id != 0 ) { + if( operation->id != 0 ) + { status = PSA_ERROR_BAD_STATE; goto exit; } - if( !PSA_ALG_IS_HASH( alg ) ) { + if( !PSA_ALG_IS_HASH( alg ) ) + { status = PSA_ERROR_INVALID_ARGUMENT; goto exit; } @@ -2190,7 +2192,7 @@ psa_status_t psa_hash_setup( psa_hash_operation_t *operation, exit: if( status != PSA_SUCCESS ) - psa_hash_abort(operation); + psa_hash_abort( operation ); return status; } @@ -2201,7 +2203,8 @@ psa_status_t psa_hash_update( psa_hash_operation_t *operation, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - if( operation->id == 0 ) { + if( operation->id == 0 ) + { status = PSA_ERROR_BAD_STATE; goto exit; } @@ -2249,7 +2252,8 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation, if( status != PSA_SUCCESS ) goto exit; - if( actual_hash_length != hash_length ) { + if( actual_hash_length != hash_length ) + { status = PSA_ERROR_INVALID_SIGNATURE; goto exit; } @@ -2386,7 +2390,8 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation, psa_key_slot_t *slot = NULL; /* A context must be freshly initialized before it can be set up. */ - if( operation->id != 0 ) { + if( operation->id != 0 ) + { status = PSA_ERROR_BAD_STATE; goto exit; } @@ -2488,19 +2493,22 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, goto cleanup; } - if( ! operation->is_sign ) { + if( ! operation->is_sign ) + { status = PSA_ERROR_BAD_STATE; goto cleanup; } /* 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 ) { + if( operation->mac_size == 0 ) + { status = PSA_ERROR_BAD_STATE; goto cleanup; } - if( mac_size < operation->mac_size ) { + if( mac_size < operation->mac_size ) + { status = PSA_ERROR_BUFFER_TOO_SMALL; goto cleanup; } @@ -2538,12 +2546,14 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; - if( operation->id == 0 ) { + if( operation->id == 0 ) + { status = PSA_ERROR_BAD_STATE; goto cleanup; } - if( operation->is_sign ) { + if( operation->is_sign ) + { status = PSA_ERROR_BAD_STATE; goto cleanup; } @@ -3308,13 +3318,15 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, PSA_KEY_USAGE_DECRYPT ); /* A context must be freshly initialized before it can be set up. */ - if( operation->id != 0 ) { + if( operation->id != 0 ) + { 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 ) ) { + if( ! PSA_ALG_IS_CIPHER( alg ) ) + { status = PSA_ERROR_INVALID_ARGUMENT; goto exit; } @@ -3429,17 +3441,20 @@ psa_status_t psa_cipher_set_iv( psa_cipher_operation_t *operation, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - if( operation->id == 0 ) { + if( operation->id == 0 ) + { status = PSA_ERROR_BAD_STATE; goto exit; } - if( operation->iv_set || ! operation->iv_required ) { + if( operation->iv_set || ! operation->iv_required ) + { status = PSA_ERROR_BAD_STATE; goto exit; } - if( iv_length > PSA_CIPHER_IV_MAX_SIZE ) { + if( iv_length > PSA_CIPHER_IV_MAX_SIZE ) + { status = PSA_ERROR_INVALID_ARGUMENT; goto exit; } From d73e1b0ccd835df397f4fb3cfcfb52df0cd6e646 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 16:19:08 +0100 Subject: [PATCH 08/10] Tidy up logic in psa_mac_sign_finish Simplify the logic in psa_mac_sign_finish. Signed-off-by: Dave Rodgman --- library/psa_crypto.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index c0f419c43..a3a4b7ba8 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2483,12 +2483,8 @@ 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 ) { + if( operation->id == 0 ) + { status = PSA_ERROR_BAD_STATE; goto cleanup; } @@ -2517,6 +2513,7 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, mac, operation->mac_size, mac_length ); +cleanup: /* 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, @@ -2533,7 +2530,6 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, memset( &mac[operation->mac_size], '!', mac_size - operation->mac_size ); -cleanup: abort_status = psa_mac_abort( operation ); return( status == PSA_SUCCESS ? abort_status : status ); From 54f7351d12d22c56bfc14494881132d0f3609494 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 18:14:52 +0100 Subject: [PATCH 09/10] Improve psa_hash_update negative test Signed-off-by: Dave Rodgman --- 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 5c4d2f597..6f923ea0e 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1620,7 +1620,7 @@ void hash_bad_order( ) /* Check that update calls abort on error. */ PSA_ASSERT( psa_hash_setup( &operation, alg ) ); - operation.ctx.mbedtls_ctx.alg = PSA_ALG_XTS; + operation.id = UINT_MAX; ASSERT_OPERATION_IS_ACTIVE( operation ); TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), PSA_ERROR_BAD_STATE ); From 478ab5443b542d227aa8b289364b1e8dd1bb7703 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 25 Jun 2021 09:09:02 +0100 Subject: [PATCH 10/10] Use more standard label name Signed-off-by: Dave Rodgman --- library/psa_crypto.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index a3a4b7ba8..4bb663c92 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2486,13 +2486,13 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, if( operation->id == 0 ) { status = PSA_ERROR_BAD_STATE; - goto cleanup; + goto exit; } if( ! operation->is_sign ) { status = PSA_ERROR_BAD_STATE; - goto cleanup; + goto exit; } /* Sanity check. This will guarantee that mac_size != 0 (and so mac != NULL) @@ -2500,20 +2500,20 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, if( operation->mac_size == 0 ) { status = PSA_ERROR_BAD_STATE; - goto cleanup; + goto exit; } if( mac_size < operation->mac_size ) { status = PSA_ERROR_BUFFER_TOO_SMALL; - goto cleanup; + goto exit; } status = psa_driver_wrapper_mac_sign_finish( operation, mac, operation->mac_size, mac_length ); -cleanup: +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, @@ -2545,25 +2545,25 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, if( operation->id == 0 ) { status = PSA_ERROR_BAD_STATE; - goto cleanup; + goto exit; } if( operation->is_sign ) { status = PSA_ERROR_BAD_STATE; - goto cleanup; + 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 );