diff --git a/include/psa/crypto_struct.h b/include/psa/crypto_struct.h index fbfe77e62..28bbc6ac8 100644 --- a/include/psa/crypto_struct.h +++ b/include/psa/crypto_struct.h @@ -12,6 +12,26 @@ * In implementations with isolation between the application and the * cryptography module, it is expected that the front-end and the back-end * would have different versions of this file. + * + *

Design notes about multipart operation structures

+ * + * Each multipart operation structure contains a `psa_algorithm_t alg` + * field which indicates which specific algorithm the structure is for. + * When the structure is not in use, `alg` is 0. Most of the structure + * consists of a union which is discriminated by `alg`. + * + * Note that when `alg` is 0, the content of other fields is undefined. + * In particular, it is not guaranteed that a freshly-initialized structure + * is all-zero: we initialize structures to something like `{0, 0}`, which + * is only guaranteed to initializes the first member of the union; + * GCC and Clang initialize the whole structure to 0 (at the time of writing), + * but MSVC and CompCert don't. + * + * In Mbed Crypto, multipart operation structures live independently from + * the key. This allows Mbed Crypto to free the key objects when destroying + * a key slot. If a multipart operation needs to remember the key after + * the setup function returns, the operation structure needs to contain a + * copy of the key. */ /* * Copyright (C) 2018, ARM Limited, All Rights Reserved diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 3a78f5653..34a023190 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -994,18 +994,16 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) return( PSA_SUCCESS ); } -static void psa_abort_operations_using_key( psa_key_slot_t *slot ) -{ - /*FIXME how to implement this?*/ - (void) slot; -} - /** Completely wipe a slot in memory, including its policy. * Persistent storage is not affected. */ psa_status_t psa_wipe_key_slot( psa_key_slot_t *slot ) { psa_status_t status = psa_remove_key_data_from_memory( slot ); - psa_abort_operations_using_key( slot ); + /* Multipart operations may still be using the key. This is safe + * because all multipart operation objects are independent from + * the key slot: if they need to access the key after the setup + * phase, they have a copy of the key. Note that this means that + * key material can linger until all operations are completed. */ /* At this point, key material and other type-specific content has * been wiped. Clear remaining metadata. We can call memset and not * zeroize because the metadata is not particularly sensitive. */ @@ -1016,8 +1014,8 @@ psa_status_t psa_wipe_key_slot( psa_key_slot_t *slot ) psa_status_t psa_destroy_key( psa_key_handle_t handle ) { psa_key_slot_t *slot; - psa_status_t status = PSA_SUCCESS; - psa_status_t storage_status = PSA_SUCCESS; + psa_status_t status; /* status of the last operation */ + psa_status_t overall_status = PSA_SUCCESS; #if defined(MBEDTLS_PSA_CRYPTO_SE_C) psa_se_drv_table_entry_t *driver; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ @@ -1043,42 +1041,57 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle ) if( status != PSA_SUCCESS ) { (void) psa_crypto_stop_transaction( ); - /* TOnogrepDO: destroy what can be destroyed anyway */ - return( status ); + /* We should still try to destroy the key in the secure + * element and the key metadata in storage. This is especially + * important if the error is that the storage is full. + * But how to do it exactly without risking an inconsistent + * state after a reset? + * https://github.com/ARMmbed/mbed-crypto/issues/215 + */ + overall_status = status; + goto exit; } status = psa_destroy_se_key( driver, slot->data.se.slot_number ); + if( overall_status == PSA_SUCCESS ) + overall_status = status; } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) - if( slot->attr.lifetime == PSA_KEY_LIFETIME_PERSISTENT ) + if( slot->attr.lifetime != PSA_KEY_LIFETIME_VOLATILE ) { - storage_status = - psa_destroy_persistent_key( slot->attr.id ); + status = psa_destroy_persistent_key( slot->attr.id ); + if( overall_status == PSA_SUCCESS ) + overall_status = status; + + /* TODO: other slots may have a copy of the same key. We should + * invalidate them. + * https://github.com/ARMmbed/mbed-crypto/issues/214 + */ } #endif /* defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) */ #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( driver != NULL ) { - psa_status_t status2; status = psa_save_se_persistent_data( driver ); - status2 = psa_crypto_stop_transaction( ); - if( status == PSA_SUCCESS ) - status = status2; - if( status != PSA_SUCCESS ) - { - /* TOnogrepDO: destroy what can be destroyed anyway */ - return( status ); - } + if( overall_status == PSA_SUCCESS ) + overall_status = status; + status = psa_crypto_stop_transaction( ); + if( overall_status == PSA_SUCCESS ) + overall_status = status; } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ +#if defined(MBEDTLS_PSA_CRYPTO_SE_C) +exit: +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ status = psa_wipe_key_slot( slot ); - if( status != PSA_SUCCESS ) - return( status ); - return( storage_status ); + /* Prioritize CORRUPTION_DETECTED from wiping over a storage error */ + if( overall_status == PSA_SUCCESS ) + overall_status = status; + return( overall_status ); } void psa_reset_key_attributes( psa_key_attributes_t *attributes ) @@ -1201,8 +1214,10 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, case PSA_KEY_TYPE_RSA_KEY_PAIR: case PSA_KEY_TYPE_RSA_PUBLIC_KEY: #if defined(MBEDTLS_PSA_CRYPTO_SE_C) - /* TOnogrepDO: reporting the public exponent for opaque keys - * is not yet implemented. */ + /* TODO: reporting the public exponent for opaque keys + * is not yet implemented. + * https://github.com/ARMmbed/mbed-crypto/issues/216 + */ if( psa_key_slot_is_external( slot ) ) break; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ @@ -1722,10 +1737,12 @@ static void psa_fail_key_creation( psa_key_slot_t *slot, return; #if defined(MBEDTLS_PSA_CRYPTO_SE_C) - /* TOnogrepDO: If the key has already been created in the secure + /* TODO: If the key has already been created in the secure * element, and the failure happened later (when saving metadata * to internal storage), we need to destroy the key in the secure - * element. */ + * element. + * https://github.com/ARMmbed/mbed-crypto/issues/217 + */ /* Abort the ongoing transaction if any (there may not be one if * the creation process failed before starting one, or if the @@ -6075,8 +6092,10 @@ static psa_status_t psa_crypto_recover_transaction( { case PSA_CRYPTO_TRANSACTION_CREATE_KEY: case PSA_CRYPTO_TRANSACTION_DESTROY_KEY: - /* TOnogrepDO - fall through to the failure case until this - * is implemented */ + /* TODO - fall through to the failure case until this + * is implemented. + * https://github.com/ARMmbed/mbed-crypto/issues/218 + */ default: /* We found an unsupported transaction in the storage. * We don't know what state the storage is in. Give up. */ 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 202f18c6d..fc6f66816 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -162,6 +162,17 @@ static psa_status_t null_generate( psa_drv_se_context_t *context, return( PSA_SUCCESS ); } +/* Null destroy: do nothing, but pretend it worked. */ +static psa_status_t null_destroy( psa_drv_se_context_t *context, + void *persistent_data, + psa_key_slot_number_t slot_number ) +{ + (void) context; + (void) persistent_data; + (void) slot_number; + return( PSA_SUCCESS ); +} + /****************************************************************/ @@ -793,6 +804,9 @@ void key_creation_import_export( int min_slot, int restart ) exported, exported_length ); PSA_ASSERT( psa_destroy_key( handle ) ); + handle = 0; + TEST_EQUAL( psa_open_key( id, &handle ), + PSA_ERROR_DOES_NOT_EXIST ); /* Test that the key has been erased from the designated slot. */ TEST_ASSERT( ram_slots[min_slot].type == 0 ); @@ -864,6 +878,9 @@ void key_creation_in_chosen_slot( int slot_arg, PSA_ASSERT( psa_get_key_attributes( handle, &attributes ) ); PSA_ASSERT( psa_destroy_key( handle ) ); + handle = 0; + TEST_EQUAL( psa_open_key( id, &handle ), + PSA_ERROR_DOES_NOT_EXIST ); exit: PSA_DONE( ); @@ -892,6 +909,7 @@ void import_key_smoke( int type_arg, int alg_arg, driver.persistent_data_size = sizeof( psa_key_slot_number_t ); key_management.p_allocate = counter_allocate; key_management.p_import = null_import; + key_management.p_destroy = null_destroy; PSA_ASSERT( psa_register_se_driver( lifetime, &driver ) ); PSA_ASSERT( psa_crypto_init( ) ); @@ -923,6 +941,9 @@ void import_key_smoke( int type_arg, int alg_arg, /* We're done. */ PSA_ASSERT( psa_destroy_key( handle ) ); + handle = 0; + TEST_EQUAL( psa_open_key( id, &handle ), + PSA_ERROR_DOES_NOT_EXIST ); exit: PSA_DONE( ); @@ -986,6 +1007,7 @@ void generate_key_smoke( int type_arg, int bits_arg, int alg_arg ) driver.persistent_data_size = sizeof( psa_key_slot_number_t ); key_management.p_allocate = counter_allocate; key_management.p_generate = null_generate; + key_management.p_destroy = null_destroy; PSA_ASSERT( psa_register_se_driver( lifetime, &driver ) ); PSA_ASSERT( psa_crypto_init( ) ); @@ -1016,6 +1038,9 @@ void generate_key_smoke( int type_arg, int bits_arg, int alg_arg ) /* We're done. */ PSA_ASSERT( psa_destroy_key( handle ) ); + handle = 0; + TEST_EQUAL( psa_open_key( id, &handle ), + PSA_ERROR_DOES_NOT_EXIST ); exit: PSA_DONE( ); @@ -1208,10 +1233,11 @@ void register_key_smoke_test( int lifetime_arg, memset( &driver, 0, sizeof( driver ) ); driver.hal_version = PSA_DRV_SE_HAL_VERSION; + memset( &key_management, 0, sizeof( key_management ) ); + driver.key_management = &key_management; + key_management.p_destroy = null_destroy; if( validate >= 0 ) { - memset( &key_management, 0, sizeof( key_management ) ); - driver.key_management = &key_management; key_management.p_validate_slot_number = validate_slot_number_as_directed; validate_slot_number_directions.slot_number = wanted_slot; validate_slot_number_directions.method = PSA_KEY_CREATION_REGISTER; @@ -1250,6 +1276,9 @@ void register_key_smoke_test( int lifetime_arg, goto exit; /* This time, destroy the key. */ PSA_ASSERT( psa_destroy_key( handle ) ); + handle = 0; + TEST_EQUAL( psa_open_key( id, &handle ), + PSA_ERROR_DOES_NOT_EXIST ); exit: psa_reset_key_attributes( &attributes );