Merge pull request #221 from gilles-peskine-arm/annotate_todo_comments-20190813
SE keys: fix psa_destroy_key; add Github issue numbers for missing code
diff --git a/include/psa/crypto_struct.h b/include/psa/crypto_struct.h
index fbfe77e..28bbc6a 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.
+ *
+ * <h3>Design notes about multipart operation structures</h3>
+ *
+ * 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 3a78f56..34a0231 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -994,18 +994,16 @@
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_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 @@
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 @@
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 @@
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 @@
{
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 202f18c..fc6f668 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 @@
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 @@
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 @@
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 @@
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 @@
/* 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 @@
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 @@
/* 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 @@
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 @@
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 );