From 424f89453b27091a1fc7e51c8f9848a1a8a944e6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 15 Jul 2019 21:59:53 +0200 Subject: [PATCH 1/5] SE keys: store the bit size internally (partial implementation) This commit blindingly copies the size from the attributes. This is not correct for copy and import. --- library/psa_crypto.c | 9 +++++++++ library/psa_crypto_core.h | 1 + 2 files changed, 10 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 0b33d764b..fc9161d8e 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1035,6 +1035,11 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle ) /* Return the size of the key in the given slot, in bits. */ static size_t psa_get_key_slot_bits( const psa_key_slot_t *slot ) { +#if defined(MBEDTLS_PSA_CRYPTO_SE_C) + if( psa_get_se_driver( slot->lifetime, NULL, NULL ) ) + return( slot->data.se.bits ); +#endif /* defined(MBEDTLS_PSA_CRYPTO_SE_C) */ + if( key_type_is_raw_bytes( slot->type ) ) return( slot->data.raw.bytes * 8 ); #if defined(MBEDTLS_RSA_C) @@ -1489,6 +1494,10 @@ static psa_status_t psa_start_key_creation( (void) psa_crypto_stop_transaction( ); return( status ); } + + /* TOnogrepDO: validate bits. How to do this depends on the key + * creation method, so setting bits might not belong here. */ + slot->data.se.bits = psa_get_key_bits( attributes ); } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index 6096810f4..86584907c 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -64,6 +64,7 @@ typedef struct struct se { psa_key_slot_number_t slot_number; + size_t bits; } se; } data; } psa_key_slot_t; From dc5bfe97842667e89ac1394effc02875d85342b2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Jul 2019 19:09:30 +0200 Subject: [PATCH 2/5] SE keys: implement and test psa_get_key_attributes --- library/psa_crypto.c | 18 ++++++--- ...st_suite_psa_crypto_se_driver_hal.function | 38 +++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index fc9161d8e..b3a6f8a9a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1145,10 +1145,10 @@ exit: } #endif /* MBEDTLS_RSA_C */ -/** Retrieve the readily-accessible attributes of a key in a slot. +/** Retrieve the generic attributes of a key in a slot. * - * This function does not compute attributes that are not directly - * stored in the slot, such as the bit size of a transparent key. + * This function does not retrieve domain parameters, which require + * additional memory management. */ static void psa_get_key_slot_attributes( psa_key_slot_t *slot, psa_key_attributes_t *attributes ) @@ -1157,6 +1157,7 @@ static void psa_get_key_slot_attributes( psa_key_slot_t *slot, attributes->lifetime = slot->lifetime; attributes->policy = slot->policy; attributes->type = slot->type; + attributes->bits = psa_get_key_slot_bits( slot ); } /** Retrieve all the publicly-accessible attributes of a key. @@ -1169,21 +1170,26 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, psa_reset_key_attributes( attributes ); - status = psa_get_transparent_key( handle, &slot, 0, 0 ); + status = psa_get_key_from_slot( handle, &slot, 0, 0 ); if( status != PSA_SUCCESS ) return( status ); psa_get_key_slot_attributes( slot, attributes ); - attributes->bits = psa_get_key_slot_bits( slot ); switch( slot->type ) { #if defined(MBEDTLS_RSA_C) 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. */ + if( psa_get_se_driver( slot->lifetime, NULL, NULL ) ) + break; +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ status = psa_get_rsa_public_exponent( slot->data.rsa, attributes ); break; -#endif +#endif /* MBEDTLS_RSA_C */ default: /* Nothing else to do. */ break; 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 e0b8d29a5..f6b480ff1 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -178,6 +178,41 @@ static psa_status_t ram_allocate( psa_drv_se_context_t *context, /* Other test helper functions */ /****************************************************************/ +/* Check that the attributes of a key reported by psa_get_key_attributes() + * are consistent with the attributes used when creating the key. */ +static int check_key_attributes( + psa_key_handle_t handle, + const psa_key_attributes_t *reference_attributes ) +{ + int ok = 0; + psa_key_attributes_t actual_attributes = PSA_KEY_ATTRIBUTES_INIT; + + PSA_ASSERT( psa_get_key_attributes( handle, &actual_attributes ) ); + + TEST_EQUAL( psa_get_key_id( &actual_attributes ), + psa_get_key_id( reference_attributes ) ); + TEST_EQUAL( psa_get_key_lifetime( &actual_attributes ), + psa_get_key_lifetime( reference_attributes ) ); + TEST_EQUAL( psa_get_key_type( &actual_attributes ), + psa_get_key_type( reference_attributes ) ); + TEST_EQUAL( psa_get_key_usage_flags( &actual_attributes ), + psa_get_key_usage_flags( reference_attributes ) ); + TEST_EQUAL( psa_get_key_algorithm( &actual_attributes ), + psa_get_key_algorithm( reference_attributes ) ); + TEST_EQUAL( psa_get_key_enrollment_algorithm( &actual_attributes ), + psa_get_key_enrollment_algorithm( reference_attributes ) ); + if( psa_get_key_bits( reference_attributes ) != 0 ) + { + TEST_EQUAL( psa_get_key_bits( &actual_attributes ), + psa_get_key_bits( reference_attributes ) ); + } + + 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. */ @@ -445,6 +480,9 @@ void key_creation_import_export( 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 the key attributes and the key data. */ + if( ! check_key_attributes( handle, &attributes ) ) + goto exit; PSA_ASSERT( psa_export_key( handle, exported, sizeof( exported ), &exported_length ) ); From 1801740a7c82137f637a2ad68384e22a60f826cf Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Jul 2019 20:25:59 +0200 Subject: [PATCH 3/5] SE driver: report the bit size on key import Add a parameter to the key import method of a secure element driver to make it report the key size in bits. This is necessary (otherwise the core has no idea what the bit-size is), and making import report it is easier than adding a separate method (for other key creation methods, this information is an input, not an output). --- include/psa/crypto_se_driver.h | 11 ++++++++--- library/psa_crypto.c | 10 +++++----- .../test_suite_psa_crypto_se_driver_hal.function | 11 ++++++++--- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/include/psa/crypto_se_driver.h b/include/psa/crypto_se_driver.h index 9aebc45c1..f95eaeb33 100644 --- a/include/psa/crypto_se_driver.h +++ b/include/psa/crypto_se_driver.h @@ -833,14 +833,18 @@ typedef psa_status_t (*psa_drv_se_allocate_key_t)( * * \param[in,out] drv_context The driver context structure. * \param[in] key_slot Slot where the key will be stored - * This must be a valid slot for a key of the chosen - * type. It must be unoccupied. + * This must be a valid slot for a key of the + * chosen type. It must be unoccupied. * \param[in] lifetime The required lifetime of the key storage * \param[in] type Key type (a \c PSA_KEY_TYPE_XXX value) * \param[in] algorithm Key algorithm (a \c PSA_ALG_XXX value) * \param[in] usage The allowed uses of the key * \param[in] p_data Buffer containing the key data * \param[in] data_length Size of the `data` buffer in bytes + * \param[out] bits On success, the key size in bits. The driver + * must determine this value after parsing the + * key according to the key type. + * This value is not used if the function fails. * * \retval #PSA_SUCCESS * Success. @@ -852,7 +856,8 @@ typedef psa_status_t (*psa_drv_se_import_key_t)(psa_drv_se_context_t *drv_contex psa_algorithm_t algorithm, psa_key_usage_t usage, const uint8_t *p_data, - size_t data_length); + size_t data_length, + size_t *bits); /** * \brief A function that destroys a secure element key and restore the slot to diff --git a/library/psa_crypto.c b/library/psa_crypto.c index b3a6f8a9a..b2e863e6f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1711,8 +1711,8 @@ psa_status_t psa_import_key( const psa_key_attributes_t *attributes, psa_get_se_driver_context( driver ), slot->data.se.slot_number, slot->lifetime, slot->type, slot->policy.alg, slot->policy.usage, - data, data_length ); - /* TOnogrepDO: psa_check_key_slot_attributes? */ + data, data_length, + &slot->data.se.bits ); } else #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ @@ -1720,10 +1720,10 @@ psa_status_t psa_import_key( const psa_key_attributes_t *attributes, status = psa_import_key_into_slot( slot, data, data_length ); if( status != PSA_SUCCESS ) goto exit; - status = psa_check_key_slot_attributes( slot, attributes ); - if( status != PSA_SUCCESS ) - goto exit; } + status = psa_check_key_slot_attributes( slot, attributes ); + if( status != PSA_SUCCESS ) + goto exit; status = psa_finish_key_creation( slot, driver ); exit: 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 f6b480ff1..261058258 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -62,7 +62,8 @@ static psa_status_t null_import( psa_drv_se_context_t *context, psa_algorithm_t algorithm, psa_key_usage_t usage, const uint8_t *p_data, - size_t data_length ) + size_t data_length, + size_t *bits ) { (void) context; (void) slot_number; @@ -71,7 +72,9 @@ static psa_status_t null_import( psa_drv_se_context_t *context, (void) algorithm; (void) usage; (void) p_data; - (void) data_length; + /* We're supposed to return a key size. Return one that's correct for + * plain data keys. */ + *bits = PSA_BYTES_TO_BITS( data_length ); return( PSA_SUCCESS ); } @@ -110,7 +113,8 @@ static psa_status_t ram_import( psa_drv_se_context_t *context, psa_algorithm_t algorithm, psa_key_usage_t usage, const uint8_t *p_data, - size_t data_length ) + size_t data_length, + size_t *bits ) { (void) context; DRIVER_ASSERT( slot_number < ARRAY_LENGTH( ram_slots ) ); @@ -119,6 +123,7 @@ static psa_status_t ram_import( psa_drv_se_context_t *context, ram_slots[slot_number].lifetime = lifetime; ram_slots[slot_number].type = type; ram_slots[slot_number].bits = PSA_BYTES_TO_BITS( data_length ); + *bits = PSA_BYTES_TO_BITS( data_length ); (void) algorithm; (void) usage; memcpy( ram_slots[slot_number].content, p_data, data_length ); From e60d1d08a4c746eba03502c6a178efa20256eb1b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Jul 2019 20:27:59 +0200 Subject: [PATCH 4/5] SE keys: save the bit size in storage For a key in a secure element, save the bit size alongside the slot number. This is a quick-and-dirty implementation where the storage format depends on sizeof(size_t), which is fragile. This should be replaced by a more robust implementation before going into production. --- library/psa_crypto.c | 32 +++++++++++----------------- library/psa_crypto_slot_management.c | 5 ++--- 2 files changed, 14 insertions(+), 23 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index b2e863e6f..875252803 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1538,40 +1538,32 @@ static psa_status_t psa_finish_key_creation( #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) if( slot->lifetime != PSA_KEY_LIFETIME_VOLATILE ) { - uint8_t *buffer = NULL; - size_t buffer_size = 0; - size_t length = 0; + psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; + psa_get_key_slot_attributes( slot, &attributes ); #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( driver != NULL ) { - buffer = (uint8_t*) &slot->data.se.slot_number; - length = sizeof( slot->data.se.slot_number ); + status = psa_save_persistent_key( &attributes, + (uint8_t*) &slot->data.se, + sizeof( slot->data.se ) ); } else #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ { - buffer_size = PSA_KEY_EXPORT_MAX_SIZE( slot->type, - psa_get_key_slot_bits( slot ) ); - buffer = mbedtls_calloc( 1, buffer_size ); + size_t buffer_size = + PSA_KEY_EXPORT_MAX_SIZE( slot->type, + psa_get_key_bits( &attributes ) ); + uint8_t *buffer = mbedtls_calloc( 1, buffer_size ); + size_t length = 0; if( buffer == NULL && buffer_size != 0 ) return( PSA_ERROR_INSUFFICIENT_MEMORY ); status = psa_internal_export_key( slot, buffer, buffer_size, &length, 0 ); - } + if( status == PSA_SUCCESS ) + status = psa_save_persistent_key( &attributes, buffer, length ); - if( status == PSA_SUCCESS ) - { - psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; - psa_get_key_slot_attributes( slot, &attributes ); - status = psa_save_persistent_key( &attributes, buffer, length ); - } - -#if defined(MBEDTLS_PSA_CRYPTO_SE_C) - if( driver == NULL ) -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - { if( buffer_size != 0 ) mbedtls_platform_zeroize( buffer, buffer_size ); mbedtls_free( buffer ); diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 6b87ea0b0..e63dcdae6 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -138,13 +138,12 @@ static psa_status_t psa_load_persistent_key_into_slot( psa_key_slot_t *p_slot ) #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( psa_key_lifetime_is_external( p_slot->lifetime ) ) { - if( key_data_length != sizeof( p_slot->data.se.slot_number ) ) + if( key_data_length != sizeof( p_slot->data.se ) ) { status = PSA_ERROR_STORAGE_FAILURE; goto exit; } - memcpy( &p_slot->data.se.slot_number, key_data, - sizeof( p_slot->data.se.slot_number ) ); + memcpy( &p_slot->data.se, key_data, sizeof( p_slot->data.se ) ); } else #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ From fc321f1a5e687d55bcbf63996b69a537d090326e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Jul 2019 20:30:14 +0200 Subject: [PATCH 5/5] SE keys: test that the bit size is saved and loaded correctly --- tests/suites/test_suite_psa_crypto_se_driver_hal.function | 2 ++ 1 file changed, 2 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 261058258..6ac19a60e 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -486,6 +486,8 @@ void key_creation_import_export( int min_slot, int restart ) TEST_ASSERT( ram_slots[min_slot].type == PSA_KEY_TYPE_RAW_DATA ); /* Test the key attributes and the key data. */ + psa_set_key_bits( &attributes, + PSA_BYTES_TO_BITS( sizeof( key_material ) ) ); if( ! check_key_attributes( handle, &attributes ) ) goto exit; PSA_ASSERT( psa_export_key( handle,