From 095dadc5bc05450a70697fba939ed1c42b1186a9 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 33e7c95328..b36299fe24 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -2305,6 +2305,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 647791da5b10885feb97d2fef908c1472acf4736 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 b36299fe24..5c44979cda 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; @@ -1919,19 +1924,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 ) ); @@ -2255,11 +2266,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 +2295,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 ) ); @@ -2306,12 +2323,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 ) ); @@ -2336,10 +2356,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 38e62aebc3168e195deafbdc19999d7c58be9fbb 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 a0acc3fd9f..2ffcb22a36 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2365,19 +2365,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, @@ -2399,6 +2407,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 ); @@ -2411,11 +2420,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 ) { @@ -3257,12 +3270,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 ) @@ -3297,19 +3312,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 @@ -3328,11 +3350,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, @@ -3341,6 +3366,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 ); @@ -3356,17 +3383,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 5ae6f7547cda932ebf9d41c6fe4bdfef225f57e9 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 5c44979cda..34d038b6b9 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1545,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.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, @@ -1579,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. */ @@ -1632,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 ) ); @@ -1876,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 ) ); @@ -2244,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, From 685b6a742b457b92af9d1d64090ea3f2d2e1074f Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 11:49:14 +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 2ffcb22a36..75404aa951 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2080,34 +2080,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 ); @@ -2139,13 +2156,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 54648243cd71ebfb01f8ef53e99a051dbb0f6c4c Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 24 Jun 2021 11:49:45 +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 75404aa951..9ab7561f78 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2294,11 +2294,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, @@ -2306,7 +2308,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 @@ -3216,18 +3218,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 b5dd7c794d9567cbc2a4fafee5083fa34d6fc252 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 9ab7561f78..43787cf270 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2083,12 +2083,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; } @@ -2101,7 +2103,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; } @@ -2112,7 +2114,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; } @@ -2160,7 +2163,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; } @@ -2297,7 +2301,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; } @@ -2399,19 +2404,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; } @@ -2449,12 +2457,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; } @@ -3224,13 +3234,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; } @@ -3345,17 +3357,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 8036bddb0189fc02c23f7a5c2038bf6a47a573e8 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 43787cf270..e70aa4484f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2394,12 +2394,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; } @@ -2428,6 +2424,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, @@ -2444,7 +2441,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 6f7105818cdc4a8a01f39ea65ce1121195bf525c 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 34d038b6b9..41e29729b5 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1559,7 +1559,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 90d1cb83a030b12fb5b18aad6fe5c0b7afa34393 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 e70aa4484f..76086c7fb1 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2397,13 +2397,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) @@ -2411,20 +2411,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, @@ -2456,25 +2456,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 );