From 2493401af48ba57a778e7e4e76795a7271f07960 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 8 Oct 2019 15:43:13 +0200 Subject: [PATCH 1/5] Document that psa_close_key(0) and psa_destroy_key(0) succeed Document that passing 0 to a close/destroy function does nothing and returns PSA_SUCCESS. Although this was not written explicitly, the specification strongly suggested that this would return PSA_ERROR_INVALID_HANDLE. While returning INVALID_HANDLE makes sense, it was awkward for a very common programming style where applications can store 0 in a handle variable to indicate that the handle has been closed or has never been open: applications had to either check if (handle != 0) before calling psa_close_key(handle) or psa_destroy_key(handle), or ignore errors from the close/destroy function. Now applications following this style can just call psa_close_key(handle) or psa_destroy_key(handle). --- include/psa/crypto.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/include/psa/crypto.h b/include/psa/crypto.h index d3b7522ab..7291c3e57 100644 --- a/include/psa/crypto.h +++ b/include/psa/crypto.h @@ -459,9 +459,12 @@ psa_status_t psa_open_key(psa_key_id_t id, * maintain the key handle until after the multipart operation has finished. * * \param handle The key handle to close. + * If this is \c 0, do nothing and return \c PSA_SUCCESS. * * \retval #PSA_SUCCESS + * \p handle was a valid handle or \c 0. It is now closed. * \retval #PSA_ERROR_INVALID_HANDLE + * \p handle is not a valid handle nor \c 0. * \retval #PSA_ERROR_COMMUNICATION_FAILURE * \retval #PSA_ERROR_CORRUPTION_DETECTED * \retval #PSA_ERROR_BAD_STATE @@ -579,13 +582,17 @@ psa_status_t psa_copy_key(psa_key_handle_t source_handle, * key will cause the multipart operation to fail. * * \param handle Handle to the key to erase. + * If this is \c 0, do nothing and return \c PSA_SUCCESS. * * \retval #PSA_SUCCESS - * The key material has been erased. + * \p handle was a valid handle and the key material that it + * referred to has been erased. + * Alternatively, \p handle is \c 0. * \retval #PSA_ERROR_NOT_PERMITTED * The key cannot be erased because it is * read-only, either due to a policy or due to physical restrictions. * \retval #PSA_ERROR_INVALID_HANDLE + * \p handle is not a valid handle nor \c 0. * \retval #PSA_ERROR_COMMUNICATION_FAILURE * There was an failure in communication with the cryptoprocessor. * The key material may still be present in the cryptoprocessor. From f102e4e4f655ded6af873677b54b32f1db0ab208 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 8 Oct 2019 15:47:31 +0200 Subject: [PATCH 2/5] Test that psa_close_key(0) and psa_destroy_key(0) succeed --- tests/suites/test_suite_psa_crypto.data | 27 ++++++++----- tests/suites/test_suite_psa_crypto.function | 42 ++++++++++++++------- 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 6efdc01d1..d5b14fe79 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -22,6 +22,24 @@ persistence_attributes:0x1234:3:0x1235:0x1235:3 PSA key attributes: slot number slot_number_attribute: +psa_destroy_key(0) +destroy_invalid:0:PSA_SUCCESS + +psa_destroy_key(invalid) +destroy_invalid:1:PSA_ERROR_INVALID_HANDLE + +psa_destroy_key(huge) +destroy_invalid:-1:PSA_ERROR_INVALID_HANDLE + +psa_close_key(0) +close_invalid:0:PSA_SUCCESS + +psa_close_key(invalid) +close_invalid:1:PSA_ERROR_INVALID_HANDLE + +psa_close_key(huge) +close_invalid:-1:PSA_ERROR_INVALID_HANDLE + PSA import/export raw: 1 bytes import_export:"2a":PSA_KEY_TYPE_RAW_DATA:PSA_KEY_USAGE_EXPORT:0:8:0:PSA_SUCCESS:1 @@ -43,15 +61,6 @@ PSA import/export AES-256 depends_on:MBEDTLS_AES_C import_export:"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef":PSA_KEY_TYPE_AES:PSA_KEY_USAGE_EXPORT:PSA_ALG_CTR:256:0:PSA_SUCCESS:1 -PSA invalid handle (0) -invalid_handle:0 - -PSA invalid handle (smallest plausible handle) -invalid_handle:1 - -PSA invalid handle (largest plausible handle) -invalid_handle:-1 - PSA import: bad usage flag import_with_policy:PSA_KEY_TYPE_RAW_DATA:0x40000000:0:PSA_ERROR_INVALID_ARGUMENT diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 87529ac6c..9eb2803a7 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1103,9 +1103,6 @@ static int test_operations_on_invalid_handle( psa_key_handle_t handle ) buffer, sizeof( buffer ), &length ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_close_key( handle ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_destroy_key( handle ), PSA_ERROR_INVALID_HANDLE ); - ok = 1; exit: @@ -1271,6 +1268,34 @@ void slot_number_attribute( ) } /* END_CASE */ +/* BEGIN_CASE */ +void destroy_invalid( int handle_arg, int expected_status_arg ) +{ + psa_key_handle_t handle = handle_arg; + psa_status_t expected_status = expected_status_arg; + + PSA_ASSERT( psa_crypto_init( ) ); + TEST_EQUAL( psa_destroy_key( handle ), expected_status ); + +exit: + PSA_DONE( ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void close_invalid( int handle_arg, int expected_status_arg ) +{ + psa_key_handle_t handle = handle_arg; + psa_status_t expected_status = expected_status_arg; + + PSA_ASSERT( psa_crypto_init( ) ); + TEST_EQUAL( psa_close_key( handle ), expected_status ); + +exit: + PSA_DONE( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void import_with_policy( int type_arg, int usage_arg, int alg_arg, @@ -1535,17 +1560,6 @@ exit: } /* END_CASE */ -/* BEGIN_CASE */ -void invalid_handle( int handle ) -{ - PSA_ASSERT( psa_crypto_init( ) ); - test_operations_on_invalid_handle( handle ); - -exit: - PSA_DONE( ); -} -/* END_CASE */ - /* BEGIN_CASE */ void import_export_public_key( data_t *data, int type_arg, From 1841cf43ee438e31512bdf5bc43c673c9a26e015 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 8 Oct 2019 15:48:25 +0200 Subject: [PATCH 3/5] Make psa_close_key(0) and psa_destroy_key(0) succeed --- library/psa_crypto.c | 3 +++ library/psa_crypto_slot_management.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index b9ea00f2c..e8ab01f63 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1013,6 +1013,9 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle ) psa_se_drv_table_entry_t *driver; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ + if( handle == 0 ) + return( PSA_SUCCESS ); + status = psa_get_key_slot( handle, &slot ); if( status != PSA_SUCCESS ) return( status ); diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 59be319ce..6cd6a1135 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -255,6 +255,9 @@ psa_status_t psa_close_key( psa_key_handle_t handle ) psa_status_t status; psa_key_slot_t *slot; + if( handle == 0 ) + return( PSA_SUCCESS ); + status = psa_get_key_slot( handle, &slot ); if( status != PSA_SUCCESS ) return( status ); From 04129a0d960d5b7831fcfb1f71574ff3ffc6fe5e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 9 Oct 2019 16:23:49 +0200 Subject: [PATCH 4/5] Update slot management tests now that {close,destroy}_key(0) succeed --- ..._suite_psa_crypto_slot_management.function | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_slot_management.function b/tests/suites/test_suite_psa_crypto_slot_management.function index 3b9eada83..c269280bf 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tests/suites/test_suite_psa_crypto_slot_management.function @@ -643,12 +643,21 @@ void invalid_handle( ) TEST_ASSERT( handle1 != 0 ); /* Attempt to close and destroy some invalid handles. */ - TEST_EQUAL( psa_close_key( 0 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_close_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_destroy_key( 0 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_destroy_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_destroy_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE ); + if( handle1 - 1 != 0 ) + { + TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE ); + TEST_EQUAL( psa_destroy_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE ); + } + if( handle1 + 1 != 0 ) + { + TEST_EQUAL( psa_close_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE ); + TEST_EQUAL( psa_destroy_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE ); + } + + /* 0 is special: it isn't a valid handle, but close/destroy + * succeeds on it. */ + TEST_EQUAL( psa_close_key( 0 ), PSA_SUCCESS ); + TEST_EQUAL( psa_destroy_key( 0 ), PSA_SUCCESS ); /* After all this, check that the original handle is intact. */ PSA_ASSERT( psa_get_key_attributes( handle1, &attributes ) ); From b8cde4ec039bcf69b789e019502c8835129343c4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 11 Oct 2019 11:44:48 +0200 Subject: [PATCH 5/5] Consolidate invalid-handle tests Consolidate the invalid-handle tests from test_suite_psa_crypto and test_suite_psa_crypto_slot_management. Start with the code in test_suite_psa_crypto_slot_management and adapt it to test one invalid handle value per run of the test function. --- tests/suites/test_suite_psa_crypto.data | 18 ----- tests/suites/test_suite_psa_crypto.function | 28 -------- ...test_suite_psa_crypto_slot_management.data | 13 +++- ..._suite_psa_crypto_slot_management.function | 68 +++++++++++++------ 4 files changed, 60 insertions(+), 67 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index d5b14fe79..fdeb0f3f4 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -22,24 +22,6 @@ persistence_attributes:0x1234:3:0x1235:0x1235:3 PSA key attributes: slot number slot_number_attribute: -psa_destroy_key(0) -destroy_invalid:0:PSA_SUCCESS - -psa_destroy_key(invalid) -destroy_invalid:1:PSA_ERROR_INVALID_HANDLE - -psa_destroy_key(huge) -destroy_invalid:-1:PSA_ERROR_INVALID_HANDLE - -psa_close_key(0) -close_invalid:0:PSA_SUCCESS - -psa_close_key(invalid) -close_invalid:1:PSA_ERROR_INVALID_HANDLE - -psa_close_key(huge) -close_invalid:-1:PSA_ERROR_INVALID_HANDLE - PSA import/export raw: 1 bytes import_export:"2a":PSA_KEY_TYPE_RAW_DATA:PSA_KEY_USAGE_EXPORT:0:8:0:PSA_SUCCESS:1 diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 9eb2803a7..40e9e57e6 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1268,34 +1268,6 @@ void slot_number_attribute( ) } /* END_CASE */ -/* BEGIN_CASE */ -void destroy_invalid( int handle_arg, int expected_status_arg ) -{ - psa_key_handle_t handle = handle_arg; - psa_status_t expected_status = expected_status_arg; - - PSA_ASSERT( psa_crypto_init( ) ); - TEST_EQUAL( psa_destroy_key( handle ), expected_status ); - -exit: - PSA_DONE( ); -} -/* END_CASE */ - -/* BEGIN_CASE */ -void close_invalid( int handle_arg, int expected_status_arg ) -{ - psa_key_handle_t handle = handle_arg; - psa_status_t expected_status = expected_status_arg; - - PSA_ASSERT( psa_crypto_init( ) ); - TEST_EQUAL( psa_close_key( handle ), expected_status ); - -exit: - PSA_DONE( ); -} -/* END_CASE */ - /* BEGIN_CASE */ void import_with_policy( int type_arg, int usage_arg, int alg_arg, diff --git a/tests/suites/test_suite_psa_crypto_slot_management.data b/tests/suites/test_suite_psa_crypto_slot_management.data index 6fa872312..803917dbe 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tests/suites/test_suite_psa_crypto_slot_management.data @@ -148,8 +148,17 @@ Copy persistent to same depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C copy_to_occupied:PSA_KEY_LIFETIME_PERSISTENT:1:PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_COPY:PSA_ALG_CTR:PSA_KEY_TYPE_AES:"404142434445464748494a4b4c4d4e4f":PSA_KEY_LIFETIME_PERSISTENT:1:PSA_KEY_USAGE_EXPORT:PSA_ALG_CTR:PSA_KEY_TYPE_AES:"404142434445464748494a4b4c4d4e4f" -Close/destroy invalid handle -invalid_handle: +invalid handle: 0 +invalid_handle:INVALID_HANDLE_0:PSA_SUCCESS:PSA_ERROR_INVALID_HANDLE + +invalid handle: never opened +invalid_handle:INVALID_HANDLE_UNOPENED:PSA_ERROR_INVALID_HANDLE:PSA_ERROR_INVALID_HANDLE + +invalid handle: already closed +invalid_handle:INVALID_HANDLE_CLOSED:PSA_ERROR_INVALID_HANDLE:PSA_ERROR_INVALID_HANDLE + +invalid handle: huge +invalid_handle:INVALID_HANDLE_HUGE:PSA_ERROR_INVALID_HANDLE:PSA_ERROR_INVALID_HANDLE Open many transient handles many_transient_handles:42 diff --git a/tests/suites/test_suite_psa_crypto_slot_management.function b/tests/suites/test_suite_psa_crypto_slot_management.function index c269280bf..4c824f7de 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tests/suites/test_suite_psa_crypto_slot_management.function @@ -20,6 +20,14 @@ typedef enum CLOSE_AFTER, } reopen_policy_t; +typedef enum +{ + INVALID_HANDLE_0, + INVALID_HANDLE_UNOPENED, + INVALID_HANDLE_CLOSED, + INVALID_HANDLE_HUGE, +} invalid_handle_construction_t; + /* All test functions that create persistent keys must call * `TEST_USES_KEY_ID( key_id )` before creating a persistent key with this * identifier, and must call psa_purge_key_storage() in their cleanup @@ -625,9 +633,13 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void invalid_handle( ) +void invalid_handle( int handle_construction, + int close_status_arg, int usage_status_arg ) { - psa_key_handle_t handle1 = 0; + psa_key_handle_t valid_handle = 0; + psa_key_handle_t invalid_handle = 0; + psa_status_t close_status = close_status_arg; + psa_status_t usage_status = usage_status_arg; psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; uint8_t material[1] = "a"; @@ -639,32 +651,50 @@ void invalid_handle( ) psa_set_key_algorithm( &attributes, 0 ); PSA_ASSERT( psa_import_key( &attributes, material, sizeof( material ), - &handle1 ) ); - TEST_ASSERT( handle1 != 0 ); + &valid_handle ) ); + TEST_ASSERT( valid_handle != 0 ); - /* Attempt to close and destroy some invalid handles. */ - if( handle1 - 1 != 0 ) + /* Construct an invalid handle as specified in the test case data. */ + switch( handle_construction ) { - TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_destroy_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE ); - } - if( handle1 + 1 != 0 ) - { - TEST_EQUAL( psa_close_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE ); - TEST_EQUAL( psa_destroy_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE ); + case INVALID_HANDLE_0: + invalid_handle = 0; + break; + case INVALID_HANDLE_UNOPENED: + /* We can't easily construct a handle that's never been opened + * without knowing how the implementation constructs handle + * values. The current test code assumes that valid handles + * are in a range between 1 and some maximum. */ + if( valid_handle == 1 ) + invalid_handle = 2; + else + invalid_handle = valid_handle - 1; + break; + case INVALID_HANDLE_CLOSED: + PSA_ASSERT( psa_import_key( &attributes, + material, sizeof( material ), + &invalid_handle ) ); + PSA_ASSERT( psa_destroy_key( invalid_handle ) ); + break; + case INVALID_HANDLE_HUGE: + invalid_handle = (psa_key_handle_t) ( -1 ); + break; + default: + TEST_ASSERT( ! "unknown handle construction" ); } - /* 0 is special: it isn't a valid handle, but close/destroy - * succeeds on it. */ - TEST_EQUAL( psa_close_key( 0 ), PSA_SUCCESS ); - TEST_EQUAL( psa_destroy_key( 0 ), PSA_SUCCESS ); + /* Attempt to use the invalid handle. */ + TEST_EQUAL( psa_get_key_attributes( invalid_handle, &attributes ), + usage_status ); + TEST_EQUAL( psa_close_key( invalid_handle ), close_status ); + TEST_EQUAL( psa_destroy_key( invalid_handle ), close_status ); /* After all this, check that the original handle is intact. */ - PSA_ASSERT( psa_get_key_attributes( handle1, &attributes ) ); + PSA_ASSERT( psa_get_key_attributes( valid_handle, &attributes ) ); TEST_EQUAL( psa_get_key_type( &attributes ), PSA_KEY_TYPE_RAW_DATA ); TEST_EQUAL( psa_get_key_bits( &attributes ), PSA_BYTES_TO_BITS( sizeof( material ) ) ); - PSA_ASSERT( psa_close_key( handle1 ) ); + PSA_ASSERT( psa_close_key( valid_handle ) ); exit: PSA_DONE( );