From db06445ad68edf185e4bb16fd46e666334e04b75 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 1 Jun 2020 12:29:26 +0200 Subject: [PATCH 01/14] Fix typo in currently unused macro constant Signed-off-by: Steven Cooreman --- include/psa/crypto_values.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/psa/crypto_values.h b/include/psa/crypto_values.h index 9fed276e7..f33946ab9 100644 --- a/include/psa/crypto_values.h +++ b/include/psa/crypto_values.h @@ -1611,7 +1611,7 @@ */ #define PSA_KEY_LIFETIME_IS_VOLATILE(lifetime) \ (PSA_KEY_LIFETIME_GET_PERSISTENCE(lifetime) == \ - PSA_KEY_LIFETIME_PERSISTENCE_VOLATILE) + PSA_KEY_PERSISTENCE_VOLATILE) /** Construct a lifetime from a persistence level and a location. * From 8335f41cda5fe927efd533f1495a451bc871ef43 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 2 Jun 2020 11:04:15 +0200 Subject: [PATCH 02/14] Enable figuring out number of cores when running on OS X Signed-off-by: Steven Cooreman --- tests/scripts/docker_env.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/scripts/docker_env.sh b/tests/scripts/docker_env.sh index 8bdc42579..582a17d1a 100755 --- a/tests/scripts/docker_env.sh +++ b/tests/scripts/docker_env.sh @@ -60,12 +60,19 @@ else DOCKER="sudo docker" fi +# Figure out the number of processors available +if [ "$(uname)" == "Darwin" ]; then + NUM_PROC="$(sysctl -n hw.logicalcpu)" +else + NUM_PROC="$(nproc)" +fi + # Build the Docker image echo "Getting docker image up to date (this may take a few minutes)..." ${DOCKER} image build \ -t ${DOCKER_IMAGE_TAG} \ --cache-from=${DOCKER_IMAGE_TAG} \ - --build-arg MAKEFLAGS_PARALLEL="-j $(nproc)" \ + --build-arg MAKEFLAGS_PARALLEL="-j ${NUM_PROC}" \ --network host \ ${http_proxy+--build-arg http_proxy=${http_proxy}} \ ${https_proxy+--build-arg https_proxy=${https_proxy}} \ From c59de6ab7e7b8f006e7a70fccabb363bb9edb200 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:28:25 +0200 Subject: [PATCH 03/14] Refactor lifetime checking to reflect split in location and persistence Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 4 ++-- library/psa_crypto_slot_management.c | 5 ++++- library/psa_crypto_slot_management.h | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 69323184d..1c348e8b0 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1500,7 +1500,7 @@ static psa_status_t psa_validate_key_attributes( { psa_status_t status; - if( attributes->core.lifetime != PSA_KEY_LIFETIME_VOLATILE ) + if( ! PSA_KEY_LIFETIME_IS_VOLATILE( attributes->core.lifetime ) ) { status = psa_validate_persistent_key_parameters( attributes->core.lifetime, attributes->core.id, @@ -1660,7 +1660,7 @@ static psa_status_t psa_finish_key_creation( (void) driver; #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) - if( slot->attr.lifetime != PSA_KEY_LIFETIME_VOLATILE ) + if( ! PSA_KEY_LIFETIME_IS_VOLATILE( slot->attr.lifetime ) ) { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( driver != NULL ) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 6cd6a1135..193959aba 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -200,7 +200,10 @@ psa_status_t psa_validate_persistent_key_parameters( } else #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - if( lifetime != PSA_KEY_LIFETIME_PERSISTENT ) + if( ( PSA_KEY_LIFETIME_GET_LOCATION( lifetime ) + != PSA_KEY_LOCATION_LOCAL_STORAGE ) || + ( PSA_KEY_LIFETIME_GET_PERSISTENCE( lifetime ) + != PSA_KEY_PERSISTENCE_DEFAULT ) ) return( PSA_ERROR_INVALID_ARGUMENT ); #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index 472253dd9..db2aa964c 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -88,8 +88,8 @@ psa_status_t psa_get_empty_key_slot( psa_key_handle_t *handle, */ static inline int psa_key_lifetime_is_external( psa_key_lifetime_t lifetime ) { - return( lifetime != PSA_KEY_LIFETIME_VOLATILE && - lifetime != PSA_KEY_LIFETIME_PERSISTENT ); + return( PSA_KEY_LIFETIME_GET_LOCATION( lifetime ) + != PSA_KEY_LOCATION_LOCAL_STORAGE ); } /** Test whether the given parameters are acceptable for a persistent key. From bbeaf18eac24d142b79139fbac916b60d0b389ff Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:29:44 +0200 Subject: [PATCH 04/14] Do not persist transactions on volatile external keys Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 1c348e8b0..a0851c7f7 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1593,11 +1593,14 @@ static psa_status_t psa_start_key_creation( #if defined(MBEDTLS_PSA_CRYPTO_SE_C) /* For a key in a secure element, we need to do three things - * when creating or registering a key: + * when creating or registering a persistent key: * create the key file in internal storage, create the * key inside the secure element, and update the driver's - * persistent data. Start a transaction that will encompass these - * three actions. */ + * persistent data. This is done by starting a transaction that will + * encompass these three actions. + * For registering a volatile key, we just need to find an appropriate + * slot number inside the SE. Since the key is designated volatile, creating + * a transaction is not required. */ /* The first thing to do is to find a slot number for the new key. * We save the slot number in persistent storage as part of the * transaction data. It will be needed to recover if the power @@ -1612,15 +1615,19 @@ static psa_status_t psa_start_key_creation( &slot->data.se.slot_number ); if( status != PSA_SUCCESS ) return( status ); - psa_crypto_prepare_transaction( PSA_CRYPTO_TRANSACTION_CREATE_KEY ); - psa_crypto_transaction.key.lifetime = slot->attr.lifetime; - psa_crypto_transaction.key.slot = slot->data.se.slot_number; - psa_crypto_transaction.key.id = slot->attr.id; - status = psa_crypto_save_transaction( ); - if( status != PSA_SUCCESS ) + + if( ! PSA_KEY_LIFETIME_IS_VOLATILE( attributes->core.lifetime ) ) { - (void) psa_crypto_stop_transaction( ); - return( status ); + psa_crypto_prepare_transaction( PSA_CRYPTO_TRANSACTION_CREATE_KEY ); + psa_crypto_transaction.key.lifetime = slot->attr.lifetime; + psa_crypto_transaction.key.slot = slot->data.se.slot_number; + psa_crypto_transaction.key.id = slot->attr.id; + status = psa_crypto_save_transaction( ); + if( status != PSA_SUCCESS ) + { + (void) psa_crypto_stop_transaction( ); + return( status ); + } } } @@ -1708,8 +1715,8 @@ static psa_status_t psa_finish_key_creation( /* Finish the transaction for a key creation. This does not * happen when registering an existing key. Detect this case * by checking whether a transaction is in progress (actual - * creation of a key in a secure element requires a transaction, - * but registration doesn't use one). */ + * creation of a persistent key in a secure element requires a transaction, + * but registration or volatile key creation doesn't use one). */ if( driver != NULL && psa_crypto_transaction.unknown.type == PSA_CRYPTO_TRANSACTION_CREATE_KEY ) { From 223f2877bee96defc611bcee8c255c349d62f7c5 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:30:20 +0200 Subject: [PATCH 05/14] Add test to check that volatile external keys do not get persisted Signed-off-by: Steven Cooreman --- .../test_suite_psa_crypto_se_driver_hal.data | 28 +++++-- ...st_suite_psa_crypto_se_driver_hal.function | 77 ++++++++++++++++--- 2 files changed, 86 insertions(+), 19 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.data b/tests/suites/test_suite_psa_crypto_se_driver_hal.data index 55c34266b..5333e570d 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.data +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.data @@ -24,17 +24,29 @@ register_twice:3 Register SE driver: maximum number of drivers register_max: -SE key import-export (p_allocate allows all slots) -key_creation_import_export:0:0 +SE key import-export persistent (p_allocate allows all slots) +key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:0:0 -SE key import-export (p_allocate allows 1 slot) -key_creation_import_export:ARRAY_LENGTH( ram_slots ) - 1:0 +SE key import-export persistent (p_allocate allows 1 slot) +key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:ARRAY_LENGTH( ram_slots ) - 1:0 -SE key import-export, check after restart (slot 0) -key_creation_import_export:0:1 +SE key import-export persistent, check after restart (slot 0) +key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:0:1 -SE key import-export, check after restart (slot 3) -key_creation_import_export:3:1 +SE key import-export persistent, check after restart (slot 3) +key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:3:1 + +SE key import-export volatile (p_allocate allows all slots) +key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:0:0 + +SE key import-export volatile (p_allocate allows 1 slot) +key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:ARRAY_LENGTH( ram_slots ) - 1:0 + +SE key import-export volatile, check after restart (slot 0) +key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:0:1 + +SE key import-export volatile, check after restart (slot 3) +key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:3:1 Key creation in a specific slot (0) key_creation_in_chosen_slot:0:0:PSA_SUCCESS diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.function b/tests/suites/test_suite_psa_crypto_se_driver_hal.function index f95f7e526..9b0cf45f3 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -27,6 +27,10 @@ ( PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( \ PSA_KEY_PERSISTENCE_DEFAULT, TEST_DRIVER_LOCATION ) ) +#define TEST_SE_VOLATILE_LIFETIME \ + ( PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( \ + PSA_KEY_PERSISTENCE_VOLATILE, TEST_DRIVER_LOCATION ) ) + /** The driver detected a condition that shouldn't happen. * This is probably a bug in the library. */ #define PSA_ERROR_DETECTED_BY_DRIVER ((psa_status_t)( -500 )) @@ -609,6 +613,20 @@ exit: return( ok ); } +/* Check that no persistent data exists for the given location. */ +static int check_no_persistent_data( psa_key_location_t location ) +{ + psa_storage_uid_t uid = file_uid_for_location( location ); + struct psa_storage_info_t info; + int ok = 0; + + TEST_ASSERT( psa_its_get_info( uid, &info ) == PSA_ERROR_DOES_NOT_EXIST ); + ok = 1; + +exit: + return( ok ); +} + /* Check that a function's return status is "smoke-free", i.e. that * it's an acceptable error code when calling an API function that operates * on a key with potentially bogus parameters. */ @@ -829,11 +847,11 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void key_creation_import_export( int min_slot, int restart ) +void key_creation_import_export( int lifetime_arg, int min_slot, int restart ) { psa_drv_se_t driver; psa_drv_se_key_management_t key_management; - psa_key_lifetime_t lifetime = TEST_SE_PERSISTENT_LIFETIME; + psa_key_lifetime_t lifetime = (psa_key_lifetime_t) lifetime_arg; psa_key_location_t location = PSA_KEY_LIFETIME_GET_LOCATION( lifetime ); psa_key_id_t id = 1; psa_key_handle_t handle = 0; @@ -864,10 +882,25 @@ void key_creation_import_export( int min_slot, int restart ) PSA_ASSERT( psa_import_key( &attributes, key_material, sizeof( key_material ), &handle ) ); - if( ! check_persistent_data( location, - &ram_shadow_slot_usage, - sizeof( ram_shadow_slot_usage ) ) ) - goto exit; + + + if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) ) + { + /* For volatile keys, check no persistent data was created */ + if( ! check_no_persistent_data( location ) ) + goto exit; + } + else + { + /* For persistent keys, check persistent data */ + if( ! check_persistent_data( location, + &ram_shadow_slot_usage, + sizeof( ram_shadow_slot_usage ) ) ) + goto exit; + } + + /* Test that the key was created in the expected slot. */ + TEST_ASSERT( ram_slots[min_slot].type == PSA_KEY_TYPE_RAW_DATA ); /* Maybe restart, to check that the information is saved correctly. */ if( restart ) @@ -875,11 +908,33 @@ void key_creation_import_export( int min_slot, int restart ) mbedtls_psa_crypto_free( ); PSA_ASSERT( psa_register_se_driver( location, &driver ) ); PSA_ASSERT( psa_crypto_init( ) ); - if( ! check_persistent_data( location, - &ram_shadow_slot_usage, - sizeof( ram_shadow_slot_usage ) ) ) - goto exit; - PSA_ASSERT( psa_open_key( id, &handle ) ); + + if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) ) + { + /* Check that the PSA core has no knowledge of the volatile key */ + TEST_ASSERT( psa_open_key( id, &handle ) == PSA_ERROR_DOES_NOT_EXIST ); + + /* Drop data from our mockup driver */ + ram_slots_reset(); + ram_min_slot = min_slot; + + /* Re-import key */ + PSA_ASSERT( psa_import_key( &attributes, + key_material, sizeof( key_material ), + &handle ) ); + } + else + { + + /* Check we can re-open the persistent key */ + if( ! check_persistent_data( location, + &ram_shadow_slot_usage, + sizeof( ram_shadow_slot_usage ) ) ) + goto exit; + + /* Check that the PSA core still knows about the key */ + PSA_ASSERT( psa_open_key( id, &handle ) ); + } } /* Test that the key was created in the expected slot. */ From a50f28338b8daa5ed22669598e1c9883de9e7972 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:35:57 +0200 Subject: [PATCH 06/14] Add test to check key ID's from the vendor range are accepted Signed-off-by: Steven Cooreman --- .../test_suite_psa_crypto_se_driver_hal.data | 20 ++++++++++++------- ...st_suite_psa_crypto_se_driver_hal.function | 3 ++- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.data b/tests/suites/test_suite_psa_crypto_se_driver_hal.data index 5333e570d..023024d39 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.data +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.data @@ -130,22 +130,28 @@ Key generation smoke test: HMAC-SHA-256 generate_key_smoke:PSA_KEY_TYPE_HMAC:256:PSA_ALG_HMAC( PSA_ALG_SHA_256 ) Key registration: smoke test -register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:PSA_SUCCESS +register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:1:PSA_SUCCESS -Key registration: invalid lifetime (volatile) -register_key_smoke_test:PSA_KEY_LIFETIME_VOLATILE:1:PSA_ERROR_INVALID_ARGUMENT +Key registration: invalid lifetime (volatile internal storage) +register_key_smoke_test:PSA_KEY_LIFETIME_VOLATILE:1:1:PSA_ERROR_INVALID_ARGUMENT Key registration: invalid lifetime (internal storage) -register_key_smoke_test:PSA_KEY_LIFETIME_PERSISTENT:1:PSA_ERROR_INVALID_ARGUMENT +register_key_smoke_test:PSA_KEY_LIFETIME_PERSISTENT:1:1:PSA_ERROR_INVALID_ARGUMENT Key registration: invalid lifetime (no registered driver) -register_key_smoke_test:PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( PSA_KEY_PERSISTENCE_DEFAULT, TEST_DRIVER_LOCATION + 1 ):1:PSA_ERROR_INVALID_ARGUMENT +register_key_smoke_test:PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( PSA_KEY_PERSISTENCE_DEFAULT, TEST_DRIVER_LOCATION + 1 ):1:1:PSA_ERROR_INVALID_ARGUMENT Key registration: rejected -register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:0:PSA_ERROR_NOT_PERMITTED +register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:0:PSA_ERROR_NOT_PERMITTED Key registration: not supported -register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:-1:PSA_ERROR_NOT_SUPPORTED +register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:-1:PSA_ERROR_NOT_SUPPORTED + +Key registration: key id out of range +register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:PSA_KEY_ID_VENDOR_MAX+1:-1:PSA_ERROR_INVALID_ARGUMENT + +Key registration: key id in vendor range +register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:PSA_KEY_ID_VENDOR_MAX:1:PSA_SUCCESS Import-sign-verify: sign in driver, ECDSA depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.function b/tests/suites/test_suite_psa_crypto_se_driver_hal.function index 9b0cf45f3..ead8b5ab2 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -1410,6 +1410,7 @@ exit: /* BEGIN_CASE */ void register_key_smoke_test( int lifetime_arg, + int id_arg, int validate, int expected_status_arg ) { @@ -1419,7 +1420,7 @@ void register_key_smoke_test( int lifetime_arg, psa_drv_se_t driver; psa_drv_se_key_management_t key_management; psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; - psa_key_id_t id = 1; + psa_key_id_t id = id_arg; size_t bit_size = 48; psa_key_slot_number_t wanted_slot = 0x123456789; psa_key_handle_t handle = 0; From 81fe7c311ac18321f66c5949be7b8312c7d4aa59 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:37:19 +0200 Subject: [PATCH 07/14] Split 'validate persistent key parameters' into independent validation Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 17 ++++--- library/psa_crypto_slot_management.c | 69 +++++++++++++++++----------- library/psa_crypto_slot_management.h | 51 ++++++++++---------- 3 files changed, 72 insertions(+), 65 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index a0851c7f7..aea4924d3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1498,16 +1498,15 @@ static psa_status_t psa_validate_key_attributes( const psa_key_attributes_t *attributes, psa_se_drv_table_entry_t **p_drv ) { - psa_status_t status; + psa_status_t status = PSA_ERROR_INVALID_ARGUMENT; - if( ! PSA_KEY_LIFETIME_IS_VOLATILE( attributes->core.lifetime ) ) - { - status = psa_validate_persistent_key_parameters( - attributes->core.lifetime, attributes->core.id, - p_drv, 1 ); - if( status != PSA_SUCCESS ) - return( status ); - } + status = psa_validate_key_location( attributes, p_drv ); + if( status != PSA_SUCCESS ) + return( status ); + + status = psa_validate_key_persistence( attributes ); + if( status != PSA_SUCCESS ) + return( status ); status = psa_validate_key_policy( &attributes->core.policy ); if( status != PSA_SUCCESS ) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 193959aba..01fd04816 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -183,39 +183,54 @@ static int psa_is_key_id_valid( psa_key_file_id_t file_id, } #endif /* defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) */ -psa_status_t psa_validate_persistent_key_parameters( - psa_key_lifetime_t lifetime, - psa_key_file_id_t id, - psa_se_drv_table_entry_t **p_drv, - int creating ) +psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, + psa_se_drv_table_entry_t **p_drv ) { - if( p_drv != NULL ) - *p_drv = NULL; -#if defined(MBEDTLS_PSA_CRYPTO_SE_C) - if( psa_key_lifetime_is_external( lifetime ) ) + psa_key_lifetime_t lifetime = psa_get_key_lifetime( attributes ); + if ( psa_key_lifetime_is_external( lifetime ) ) { - *p_drv = psa_get_se_driver_entry( lifetime ); - if( *p_drv == NULL ) +#if defined(MBEDTLS_PSA_CRYPTO_SE_C) + psa_se_drv_table_entry_t *p_drv_e = psa_get_se_driver_entry( lifetime ); + if( p_drv_e == NULL ) return( PSA_ERROR_INVALID_ARGUMENT ); + else + { + if (p_drv != NULL) + *p_drv = p_drv_e; + return( PSA_SUCCESS ); + } +#else + (void) p_drv; + return( PSA_ERROR_INVALID_ARGUMENT ); +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ } else -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - if( ( PSA_KEY_LIFETIME_GET_LOCATION( lifetime ) - != PSA_KEY_LOCATION_LOCAL_STORAGE ) || - ( PSA_KEY_LIFETIME_GET_PERSISTENCE( lifetime ) - != PSA_KEY_PERSISTENCE_DEFAULT ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); + /* Local/internal keys are always valid */ + return( PSA_SUCCESS ); +} +psa_status_t psa_validate_key_persistence( const psa_key_attributes_t *attributes ) +{ + psa_key_lifetime_t lifetime = psa_get_key_lifetime( attributes ); + + if ( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) ) + { + /* Volatile keys are always supported */ + return( PSA_SUCCESS ); + } + else + { + /* Persistent keys require storage support */ #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) - if( ! psa_is_key_id_valid( id, ! creating ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); - return( PSA_SUCCESS ); - + if( psa_is_key_id_valid( psa_get_key_id( attributes ), + psa_key_lifetime_is_external( lifetime ) ) ) + return( PSA_SUCCESS ); + else + return( PSA_ERROR_INVALID_ARGUMENT ); #else /* MBEDTLS_PSA_CRYPTO_STORAGE_C */ - (void) id; - (void) creating; - return( PSA_ERROR_NOT_SUPPORTED ); + return( PSA_ERROR_NOT_SUPPORTED ); #endif /* !MBEDTLS_PSA_CRYPTO_STORAGE_C */ + } } psa_status_t psa_open_key( psa_key_file_id_t id, psa_key_handle_t *handle ) @@ -226,10 +241,8 @@ psa_status_t psa_open_key( psa_key_file_id_t id, psa_key_handle_t *handle ) *handle = 0; - status = psa_validate_persistent_key_parameters( - PSA_KEY_LIFETIME_PERSISTENT, id, NULL, 0 ); - if( status != PSA_SUCCESS ) - return( status ); + if( ! psa_is_key_id_valid( id, 1 ) ) + return( PSA_ERROR_INVALID_ARGUMENT ); status = psa_get_empty_key_slot( handle, &slot ); if( status != PSA_SUCCESS ) diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index db2aa964c..8841284cd 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -92,38 +92,33 @@ static inline int psa_key_lifetime_is_external( psa_key_lifetime_t lifetime ) != PSA_KEY_LOCATION_LOCAL_STORAGE ); } -/** Test whether the given parameters are acceptable for a persistent key. +/** Validate that a key's attributes point to a known location. * - * This function does not access the storage in any way. It only tests - * whether the parameters are meaningful and permitted by general policy. - * It does not test whether the a file by the given id exists or could be - * created. + * This function checks whether the key's attributes point to a location that + * is known to the PSA Core, and returns the driver function table if the key + * is to be found in an external location. * - * If the key is in external storage, this function returns the corresponding - * driver. + * \param[in] attributes The key attributes. + * \param[out] p_drv On success, when a key is located in external + * storage, returns a pointer to the driver table + * associated with the key's storage location. * - * \param lifetime The lifetime to test. - * \param id The key id to test. - * \param[out] p_drv On output, if \p lifetime designates a key - * in an external processor, \c *p_drv is a pointer - * to the driver table entry fot this lifetime. - * If \p lifetime designates a transparent key, - * \c *p_drv is \c NULL. - * \param creating 0 if attempting to open an existing key. - * Nonzero if attempting to create a key. - * - * \retval PSA_SUCCESS - * The given parameters are valid. - * \retval PSA_ERROR_INVALID_ARGUMENT - * \p lifetime is volatile or is invalid. - * \retval PSA_ERROR_INVALID_ARGUMENT - * \p id is invalid. + * \retval #PSA_SUCCESS + * \retval #PSA_ERROR_INVALID_ARGUMENT */ -psa_status_t psa_validate_persistent_key_parameters( - psa_key_lifetime_t lifetime, - psa_key_file_id_t id, - psa_se_drv_table_entry_t **p_drv, - int creating ); +psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, + psa_se_drv_table_entry_t **p_drv ); + +/** Validate that a key's persistence is consistent. + * + * This function checks whether a key's persistence attribute is consistent. + * + * \param[in] attributes The key attributes. + * + * \retval #PSA_SUCCESS + * \retval #PSA_ERROR_INVALID_ARGUMENT + */ +psa_status_t psa_validate_key_persistence( const psa_key_attributes_t *attributes ); #endif /* PSA_CRYPTO_SLOT_MANAGEMENT_H */ From de183388653f91136536b3a0476a141274ad3e31 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:48:01 +0200 Subject: [PATCH 08/14] Add changelog entry for #3382 Signed-off-by: Steven Cooreman --- ChangeLog.d/do_not_persist_volatile_external_keys.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/do_not_persist_volatile_external_keys.txt diff --git a/ChangeLog.d/do_not_persist_volatile_external_keys.txt b/ChangeLog.d/do_not_persist_volatile_external_keys.txt new file mode 100644 index 000000000..c10b22ca2 --- /dev/null +++ b/ChangeLog.d/do_not_persist_volatile_external_keys.txt @@ -0,0 +1,4 @@ +Default behavior changes + * Stop storing persistent information about externally stored keys created + through PSA Crypto with a volatile lifetime. Reported in #3288 and + contributed by Steven Cooreman in #3382. \ No newline at end of file From f5a5e45ed1eceb45f9c537c200d592b48d8237af Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:53:13 +0200 Subject: [PATCH 09/14] Refactor equality-testing asserts in SE driver tests to TEST_EQUAL Signed-off-by: Steven Cooreman --- .../test_suite_psa_crypto_se_driver_hal.function | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.function b/tests/suites/test_suite_psa_crypto_se_driver_hal.function index ead8b5ab2..80c50e329 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -620,7 +620,7 @@ static int check_no_persistent_data( psa_key_location_t location ) struct psa_storage_info_t info; int ok = 0; - TEST_ASSERT( psa_its_get_info( uid, &info ) == PSA_ERROR_DOES_NOT_EXIST ); + TEST_EQUAL( psa_its_get_info( uid, &info ), PSA_ERROR_DOES_NOT_EXIST ); ok = 1; exit: @@ -900,7 +900,7 @@ void key_creation_import_export( int lifetime_arg, int min_slot, int restart ) } /* Test that the key was created in the expected slot. */ - TEST_ASSERT( ram_slots[min_slot].type == PSA_KEY_TYPE_RAW_DATA ); + TEST_EQUAL( ram_slots[min_slot].type, PSA_KEY_TYPE_RAW_DATA ); /* Maybe restart, to check that the information is saved correctly. */ if( restart ) @@ -938,7 +938,7 @@ void key_creation_import_export( int lifetime_arg, int min_slot, int restart ) } /* Test that the key was created in the expected slot. */ - TEST_ASSERT( ram_slots[min_slot].type == PSA_KEY_TYPE_RAW_DATA ); + TEST_EQUAL( ram_slots[min_slot].type, PSA_KEY_TYPE_RAW_DATA ); /* Test the key attributes, including the reported slot number. */ psa_set_key_bits( &attributes, @@ -964,7 +964,7 @@ void key_creation_import_export( int lifetime_arg, int min_slot, int restart ) PSA_ERROR_DOES_NOT_EXIST ); /* Test that the key has been erased from the designated slot. */ - TEST_ASSERT( ram_slots[min_slot].type == 0 ); + TEST_EQUAL( ram_slots[min_slot].type, 0 ); exit: PSA_DONE( ); @@ -1318,7 +1318,7 @@ void sign_verify( int flow, * generate material, store the desired result of generation in * the mock secure element storage. */ PSA_ASSERT( psa_get_key_attributes( drv_handle, &drv_attributes ) ); - TEST_ASSERT( key_material->len == PSA_BITS_TO_BYTES( bits ) ); + TEST_EQUAL( key_material->len, PSA_BITS_TO_BYTES( bits ) ); memcpy( ram_slots[ram_min_slot].content, key_material->x, key_material->len ); } From 00106a12c9ff7fca4cc1ece3a2152254e9079093 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 18:54:23 +0200 Subject: [PATCH 10/14] Minor edit to comply with pointer naming standard Signed-off-by: Steven Cooreman --- library/psa_crypto_slot_management.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 01fd04816..0cab75779 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -190,13 +190,13 @@ psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, if ( psa_key_lifetime_is_external( lifetime ) ) { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) - psa_se_drv_table_entry_t *p_drv_e = psa_get_se_driver_entry( lifetime ); - if( p_drv_e == NULL ) + psa_se_drv_table_entry_t *driver = psa_get_se_driver_entry( lifetime ); + if( driver == NULL ) return( PSA_ERROR_INVALID_ARGUMENT ); else { if (p_drv != NULL) - *p_drv = p_drv_e; + *p_drv = driver; return( PSA_SUCCESS ); } #else From 74161ceaca1dbf98b965ef0188840aa594826a4d Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 8 Jun 2020 19:04:38 +0200 Subject: [PATCH 11/14] Clarify behaviour of psa_validate_key_location Signed-off-by: Steven Cooreman --- library/psa_crypto_slot_management.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index 8841284cd..46a73257b 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -109,9 +109,10 @@ static inline int psa_key_lifetime_is_external( psa_key_lifetime_t lifetime ) psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, psa_se_drv_table_entry_t **p_drv ); -/** Validate that a key's persistence is consistent. +/** Validate that a key's persistence is valid. * - * This function checks whether a key's persistence attribute is consistent. + * This function checks whether a key's declared persistence and key ID are + * valid and supported by the PSA Core in its actual configuration. * * \param[in] attributes The key attributes. * From 14b8184db1e61fcc878531dd8b9cbb7194152704 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 15 Jun 2020 11:33:41 +0200 Subject: [PATCH 12/14] Added missing newline in changelog entry Signed-off-by: Steven Cooreman --- ChangeLog.d/do_not_persist_volatile_external_keys.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/do_not_persist_volatile_external_keys.txt b/ChangeLog.d/do_not_persist_volatile_external_keys.txt index c10b22ca2..b27292c90 100644 --- a/ChangeLog.d/do_not_persist_volatile_external_keys.txt +++ b/ChangeLog.d/do_not_persist_volatile_external_keys.txt @@ -1,4 +1,4 @@ Default behavior changes * Stop storing persistent information about externally stored keys created through PSA Crypto with a volatile lifetime. Reported in #3288 and - contributed by Steven Cooreman in #3382. \ No newline at end of file + contributed by Steven Cooreman in #3382. From 8c1e759e1d5b64d7c8cd074ab73e0e6292a920d1 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Wed, 17 Jun 2020 14:52:05 +0200 Subject: [PATCH 13/14] Documentation and new function signature update Inline with review comments. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 6 ++++-- library/psa_crypto_slot_management.c | 10 ++++------ library/psa_crypto_slot_management.h | 18 ++++++++++-------- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index aea4924d3..a511c2774 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1500,11 +1500,13 @@ static psa_status_t psa_validate_key_attributes( { psa_status_t status = PSA_ERROR_INVALID_ARGUMENT; - status = psa_validate_key_location( attributes, p_drv ); + status = psa_validate_key_location( psa_get_key_lifetime( attributes ), + p_drv ); if( status != PSA_SUCCESS ) return( status ); - status = psa_validate_key_persistence( attributes ); + status = psa_validate_key_persistence( psa_get_key_lifetime( attributes ), + psa_get_key_id( attributes ) ); if( status != PSA_SUCCESS ) return( status ); diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 0cab75779..ab66b1213 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -183,10 +183,9 @@ static int psa_is_key_id_valid( psa_key_file_id_t file_id, } #endif /* defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) */ -psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, +psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime, psa_se_drv_table_entry_t **p_drv ) { - psa_key_lifetime_t lifetime = psa_get_key_lifetime( attributes ); if ( psa_key_lifetime_is_external( lifetime ) ) { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) @@ -209,10 +208,9 @@ psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, return( PSA_SUCCESS ); } -psa_status_t psa_validate_key_persistence( const psa_key_attributes_t *attributes ) +psa_status_t psa_validate_key_persistence( psa_key_lifetime_t lifetime, + psa_key_id_t key_id ) { - psa_key_lifetime_t lifetime = psa_get_key_lifetime( attributes ); - if ( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) ) { /* Volatile keys are always supported */ @@ -222,7 +220,7 @@ psa_status_t psa_validate_key_persistence( const psa_key_attributes_t *attribute { /* Persistent keys require storage support */ #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) - if( psa_is_key_id_valid( psa_get_key_id( attributes ), + if( psa_is_key_id_valid( key_id, psa_key_lifetime_is_external( lifetime ) ) ) return( PSA_SUCCESS ); else diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index 46a73257b..e65de2e6a 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -92,13 +92,13 @@ static inline int psa_key_lifetime_is_external( psa_key_lifetime_t lifetime ) != PSA_KEY_LOCATION_LOCAL_STORAGE ); } -/** Validate that a key's attributes point to a known location. +/** Validate a key's location. * * This function checks whether the key's attributes point to a location that * is known to the PSA Core, and returns the driver function table if the key * is to be found in an external location. * - * \param[in] attributes The key attributes. + * \param[in] lifetime The key lifetime attribute. * \param[out] p_drv On success, when a key is located in external * storage, returns a pointer to the driver table * associated with the key's storage location. @@ -106,20 +106,22 @@ static inline int psa_key_lifetime_is_external( psa_key_lifetime_t lifetime ) * \retval #PSA_SUCCESS * \retval #PSA_ERROR_INVALID_ARGUMENT */ -psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes, +psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime, psa_se_drv_table_entry_t **p_drv ); -/** Validate that a key's persistence is valid. +/** Validate that a key's persistence attributes are valid. * - * This function checks whether a key's declared persistence and key ID are - * valid and supported by the PSA Core in its actual configuration. + * This function checks whether a key's declared persistence level and key ID + * attributes are valid and known to the PSA Core in its actual configuration. * - * \param[in] attributes The key attributes. + * \param[in] lifetime The key lifetime attribute. + * \param[in] key_id The key ID attribute * * \retval #PSA_SUCCESS * \retval #PSA_ERROR_INVALID_ARGUMENT */ -psa_status_t psa_validate_key_persistence( const psa_key_attributes_t *attributes ); +psa_status_t psa_validate_key_persistence( psa_key_lifetime_t lifetime, + psa_key_id_t key_id ); #endif /* PSA_CRYPTO_SLOT_MANAGEMENT_H */ From fa6860933d94f99c040fc2f4c054dad5b240370f Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 18 Jun 2020 14:37:31 +0200 Subject: [PATCH 14/14] Declare unused parameter Signed-off-by: Steven Cooreman --- library/psa_crypto_slot_management.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index ab66b1213..b312ff624 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -226,6 +226,7 @@ psa_status_t psa_validate_key_persistence( psa_key_lifetime_t lifetime, else return( PSA_ERROR_INVALID_ARGUMENT ); #else /* MBEDTLS_PSA_CRYPTO_STORAGE_C */ + (void) key_id; return( PSA_ERROR_NOT_SUPPORTED ); #endif /* !MBEDTLS_PSA_CRYPTO_STORAGE_C */ }