From 4b7f340fbfb8b3e85e1fa44a9230ae933d0e58e3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:58:36 +0200 Subject: [PATCH] Clean up status code handling inside psa_destroy_key Adopt a simple method for tracking whether there was a failure: each fallible operation sets overall_status, unless overall_status is already non-successful. Thus in case of multiple failures, the function always reports whatever failed first. This may not always be the right thing, but it's simple. This revealed a bug whereby if the only failure was the call to psa_destroy_se_key(), i.e. if the driver reported a failure or if the driver lacked support for destroying keys, psa_destroy_key() would ignore that failure. For a key in a secure element, if creating a transaction file fails, don't touch storage, but close the key in memory. This may not be right, but it's no wronger than it was before. Tracked in https://github.com/ARMmbed/mbed-crypto/issues/215 --- library/psa_crypto.c | 46 ++++++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 66c515108..bce777b15 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1014,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 */ @@ -1041,18 +1041,30 @@ 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_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 @@ -1063,23 +1075,23 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle ) #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 )