From b4e73e9747ecc5ffa749100f91b18c2b79bcf5b4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:00:57 +0200 Subject: [PATCH 1/8] Add some design notes about multipart operation structures --- include/psa/crypto_struct.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) 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 From 3f7cd62ff5cfd8e6e23800bca93f7f56c0592d84 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:01:08 +0200 Subject: [PATCH 2/8] Document better what wiping a key slot does not do When a key slot is wiped, a copy of the key material may remain in operations. This is undesirable, but does not violate the safety of the code. Tracked in https://github.com/ARMmbed/mbed-crypto/issues/86 --- library/psa_crypto.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 3a78f5653..6041732fd 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. */ From 8fe253ae4abe8e5f3fb7436cedee09e1ec67cd8d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:11:25 +0200 Subject: [PATCH 3/8] SE keys: test that psa_destroy_key removes the key from storage --- .../test_suite_psa_crypto_se_driver_hal.function | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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..867016b3e 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -793,6 +793,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 +867,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( ); @@ -923,6 +929,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( ); @@ -1016,6 +1025,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( ); @@ -1250,6 +1262,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 ); From caec27821fab3db60bc52bea08602d7b1528d724 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:11:49 +0200 Subject: [PATCH 4/8] SE keys: make psa_destroy_key remove the key from storage --- library/psa_crypto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6041732fd..4fee3cd02 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1050,7 +1050,7 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle ) #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 ); From 9ce31c466d181848f31ba932428db3d4da398a6d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:14:20 +0200 Subject: [PATCH 5/8] Note about destroying a key with other open handles https://github.com/ARMmbed/mbed-crypto/issues/214 --- library/psa_crypto.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 4fee3cd02..66c515108 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1052,8 +1052,11 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle ) #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) if( slot->attr.lifetime != PSA_KEY_LIFETIME_VOLATILE ) { - storage_status = - psa_destroy_persistent_key( slot->attr.id ); + storage_status = psa_destroy_persistent_key( slot->attr.id ); + /* 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) */ From 4b7f340fbfb8b3e85e1fa44a9230ae933d0e58e3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 15:58:36 +0200 Subject: [PATCH 6/8] 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 ) From 5da7b3e55cf01c85da5a70bdb48c6ec80451a135 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 16:03:28 +0200 Subject: [PATCH 7/8] Drivers must have a psa_destroy_key method Drivers that allow destroying a key must have a destroy method. This test bug was previously not caught because of an implementation bug that lost the error triggered by the missing destroy method. --- ...est_suite_psa_crypto_se_driver_hal.function | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 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 867016b3e..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 ); +} + /****************************************************************/ @@ -898,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( ) ); @@ -995,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( ) ); @@ -1220,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; From c9d7f94a654815fe8dbf1fef1ca074e865423f46 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 16:17:16 +0200 Subject: [PATCH 8/8] Add issue numbers for some missing parts of secure element support --- library/psa_crypto.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index bce777b15..34a023190 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1214,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 */ @@ -1735,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 @@ -6088,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. */