From 98435ddf849f2fe16e27f0e93ff67a7108e814d2 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 8 Jan 2021 19:19:40 +0100 Subject: [PATCH 1/4] Allow loading wrapped keys even when SE support is compiled in Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 55 +++++++++++-------- library/psa_crypto_slot_management.c | 51 +++++++++++++---- ...test_suite_psa_crypto_slot_management.data | 11 +++- 3 files changed, 81 insertions(+), 36 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 7ea2a1a9ad..e0ed35f0a3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2325,34 +2325,45 @@ psa_status_t psa_import_key( const psa_key_attributes_t *attributes, if( status != PSA_SUCCESS ) goto exit; -#if defined(MBEDTLS_PSA_CRYPTO_SE_C) - if( driver != NULL ) + if( psa_key_lifetime_is_external( psa_get_key_lifetime( attributes ) ) ) { - const psa_drv_se_t *drv = psa_get_se_driver_methods( driver ); - /* The driver should set the number of key bits, however in - * case it doesn't, we initialize bits to an invalid value. */ - size_t bits = PSA_MAX_KEY_BITS + 1; - if( drv->key_management == NULL || - drv->key_management->p_import == NULL ) +#if defined(MBEDTLS_PSA_CRYPTO_SE_C) + if( driver != NULL ) { - status = PSA_ERROR_NOT_SUPPORTED; + const psa_drv_se_t *drv = psa_get_se_driver_methods( driver ); + /* The driver should set the number of key bits, however in + * case it doesn't, we initialize bits to an invalid value. */ + size_t bits = PSA_MAX_KEY_BITS + 1; + if( drv->key_management == NULL || + drv->key_management->p_import == NULL ) + { + status = PSA_ERROR_NOT_SUPPORTED; + goto exit; + } + status = drv->key_management->p_import( + psa_get_se_driver_context( driver ), + slot->data.se.slot_number, attributes, data, data_length, + &bits ); + if( status != PSA_SUCCESS ) + goto exit; + if( bits > PSA_MAX_KEY_BITS ) + { + status = PSA_ERROR_NOT_SUPPORTED; + goto exit; + } + slot->attr.bits = (psa_key_bits_t) bits; + } + else +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ + { + /* Importing a key with external lifetime through the driver wrapper + * interface is not yet supported. Return as if this was an invalid + * lifetime. */ + status = PSA_ERROR_INVALID_ARGUMENT; goto exit; } - status = drv->key_management->p_import( - psa_get_se_driver_context( driver ), - slot->data.se.slot_number, attributes, data, data_length, - &bits ); - if( status != PSA_SUCCESS ) - goto exit; - if( bits > PSA_MAX_KEY_BITS ) - { - status = PSA_ERROR_NOT_SUPPORTED; - goto exit; - } - slot->attr.bits = (psa_key_bits_t) bits; } else -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ { status = psa_import_key_into_slot( slot, data, data_length ); if( status != PSA_SUCCESS ) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 4c4ad0331a..f30b75ada0 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -247,25 +247,42 @@ static psa_status_t psa_load_persistent_key_into_slot( psa_key_slot_t *slot ) if( status != PSA_SUCCESS ) goto exit; -#if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( psa_key_lifetime_is_external( slot->attr.lifetime ) ) { - psa_se_key_data_storage_t *data; - if( key_data_length != sizeof( *data ) ) +#if defined(MBEDTLS_PSA_CRYPTO_SE_C) + const psa_drv_se_t *drv; + psa_drv_se_context_t *drv_context; + if( psa_get_se_driver( slot->attr.lifetime, &drv, &drv_context ) ) { - status = PSA_ERROR_STORAGE_FAILURE; - goto exit; + psa_se_key_data_storage_t *data; + if( key_data_length != sizeof( *data ) ) + { + status = PSA_ERROR_STORAGE_FAILURE; + goto exit; + } + data = (psa_se_key_data_storage_t *) key_data; + memcpy( &slot->data.se.slot_number, &data->slot_number, + sizeof( slot->data.se.slot_number ) ); + } + else +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ + { + /* A key that is successfully loaded from storage with an + * external lifetime, but doesn't belong to an SE driver, + * must be a PSA driver-associated key which we can just + * load like an internal key. */ + if ( key_data == NULL ) + { + status = PSA_ERROR_STORAGE_FAILURE; + goto exit; + } + + status = psa_copy_key_material_into_slot( slot, key_data, key_data_length ); } - data = (psa_se_key_data_storage_t *) key_data; - memcpy( &slot->data.se.slot_number, &data->slot_number, - sizeof( slot->data.se.slot_number ) ); } else -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ { status = psa_copy_key_material_into_slot( slot, key_data, key_data_length ); - if( status != PSA_SUCCESS ) - goto exit; } exit: @@ -345,7 +362,14 @@ psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime, #if defined(MBEDTLS_PSA_CRYPTO_SE_C) psa_se_drv_table_entry_t *driver = psa_get_se_driver_entry( lifetime ); if( driver == NULL ) + { +#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) + /* Key location for external keys gets checked by the wrapper */ + return( PSA_SUCCESS ); +#else return( PSA_ERROR_INVALID_ARGUMENT ); +#endif + } else { if (p_drv != NULL) @@ -354,7 +378,12 @@ psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime, } #else (void) p_drv; +#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) + /* Key location for external keys gets checked by the wrapper */ + return( PSA_SUCCESS ); +#else return( PSA_ERROR_INVALID_ARGUMENT ); +#endif #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ } else diff --git a/tests/suites/test_suite_psa_crypto_slot_management.data b/tests/suites/test_suite_psa_crypto_slot_management.data index 396cdfb531..ece8f14421 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tests/suites/test_suite_psa_crypto_slot_management.data @@ -107,10 +107,15 @@ Open failure: non-existent identifier depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C open_fail:1:PSA_ERROR_DOES_NOT_EXIST -Create failure: invalid lifetime -create_fail:0x7fffffff:0:PSA_ERROR_INVALID_ARGUMENT +Create failure: invalid lifetime for a persistent key +depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C +create_fail:0x7fffffff:1:PSA_ERROR_INVALID_ARGUMENT -Create failure: invalid key id (0) +Create failure: invalid lifetime for a volatile key +depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C +create_fail:0x7fffff00:0:PSA_ERROR_INVALID_ARGUMENT + +Create failure: invalid key id (0) for a persistent key depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C create_fail:PSA_KEY_LIFETIME_PERSISTENT:0:PSA_ERROR_INVALID_HANDLE From ac3434fc190b1258bb8b111894582379ca792e53 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 15 Jan 2021 17:36:02 +0100 Subject: [PATCH 2/4] Apply review feedback Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 64 +++++++++++---------- library/psa_crypto_slot_management.c | 83 ++++++++++++---------------- 2 files changed, 66 insertions(+), 81 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e0ed35f0a3..4f7be40ec0 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2325,43 +2325,41 @@ psa_status_t psa_import_key( const psa_key_attributes_t *attributes, if( status != PSA_SUCCESS ) goto exit; - if( psa_key_lifetime_is_external( psa_get_key_lifetime( attributes ) ) ) - { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) - if( driver != NULL ) + if( driver != NULL ) + { + const psa_drv_se_t *drv = psa_get_se_driver_methods( driver ); + /* The driver should set the number of key bits, however in + * case it doesn't, we initialize bits to an invalid value. */ + size_t bits = PSA_MAX_KEY_BITS + 1; + if( drv->key_management == NULL || + drv->key_management->p_import == NULL ) { - const psa_drv_se_t *drv = psa_get_se_driver_methods( driver ); - /* The driver should set the number of key bits, however in - * case it doesn't, we initialize bits to an invalid value. */ - size_t bits = PSA_MAX_KEY_BITS + 1; - if( drv->key_management == NULL || - drv->key_management->p_import == NULL ) - { - status = PSA_ERROR_NOT_SUPPORTED; - goto exit; - } - status = drv->key_management->p_import( - psa_get_se_driver_context( driver ), - slot->data.se.slot_number, attributes, data, data_length, - &bits ); - if( status != PSA_SUCCESS ) - goto exit; - if( bits > PSA_MAX_KEY_BITS ) - { - status = PSA_ERROR_NOT_SUPPORTED; - goto exit; - } - slot->attr.bits = (psa_key_bits_t) bits; - } - else -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - { - /* Importing a key with external lifetime through the driver wrapper - * interface is not yet supported. Return as if this was an invalid - * lifetime. */ - status = PSA_ERROR_INVALID_ARGUMENT; + status = PSA_ERROR_NOT_SUPPORTED; goto exit; } + status = drv->key_management->p_import( + psa_get_se_driver_context( driver ), + slot->data.se.slot_number, attributes, data, data_length, + &bits ); + if( status != PSA_SUCCESS ) + goto exit; + if( bits > PSA_MAX_KEY_BITS ) + { + status = PSA_ERROR_NOT_SUPPORTED; + goto exit; + } + slot->attr.bits = (psa_key_bits_t) bits; + } + else +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ + if( psa_key_lifetime_is_external( psa_get_key_lifetime( attributes ) ) ) + { + /* Importing a key with external lifetime through the driver wrapper + * interface is not yet supported. Return as if this was an invalid + * lifetime. */ + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; } else { diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index f30b75ada0..3797dea9c6 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -247,43 +247,35 @@ static psa_status_t psa_load_persistent_key_into_slot( psa_key_slot_t *slot ) if( status != PSA_SUCCESS ) goto exit; - if( psa_key_lifetime_is_external( slot->attr.lifetime ) ) - { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) - const psa_drv_se_t *drv; - psa_drv_se_context_t *drv_context; - if( psa_get_se_driver( slot->attr.lifetime, &drv, &drv_context ) ) - { - psa_se_key_data_storage_t *data; - if( key_data_length != sizeof( *data ) ) - { - status = PSA_ERROR_STORAGE_FAILURE; - goto exit; - } - data = (psa_se_key_data_storage_t *) key_data; - memcpy( &slot->data.se.slot_number, &data->slot_number, - sizeof( slot->data.se.slot_number ) ); - } - else -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - { - /* A key that is successfully loaded from storage with an - * external lifetime, but doesn't belong to an SE driver, - * must be a PSA driver-associated key which we can just - * load like an internal key. */ - if ( key_data == NULL ) - { - status = PSA_ERROR_STORAGE_FAILURE; - goto exit; - } - - status = psa_copy_key_material_into_slot( slot, key_data, key_data_length ); - } - } - else + /* Special handling is required for loading keys associated with a + * dynamically registered SE interface. */ + const psa_drv_se_t *drv; + psa_drv_se_context_t *drv_context; + if( psa_get_se_driver( slot->attr.lifetime, &drv, &drv_context ) ) { - status = psa_copy_key_material_into_slot( slot, key_data, key_data_length ); + psa_se_key_data_storage_t *data; + if( key_data_length != sizeof( *data ) ) + { + status = PSA_ERROR_STORAGE_FAILURE; + goto exit; + } + data = (psa_se_key_data_storage_t *) key_data; + memcpy( &slot->data.se.slot_number, &data->slot_number, + sizeof( slot->data.se.slot_number ) ); + + status = PSA_SUCCESS; + goto exit; } +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ + + if ( key_data == NULL ) + { + status = PSA_ERROR_STORAGE_FAILURE; + goto exit; + } + + status = psa_copy_key_material_into_slot( slot, key_data, key_data_length ); exit: psa_free_persistent_key_data( key_data, key_data_length ); @@ -360,31 +352,26 @@ psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime, if ( psa_key_lifetime_is_external( lifetime ) ) { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) + /* Check whether a driver is registered against this lifetime */ psa_se_drv_table_entry_t *driver = psa_get_se_driver_entry( lifetime ); - if( driver == NULL ) - { -#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) - /* Key location for external keys gets checked by the wrapper */ - return( PSA_SUCCESS ); -#else - return( PSA_ERROR_INVALID_ARGUMENT ); -#endif - } - else + if( driver != NULL ) { if (p_drv != NULL) *p_drv = driver; return( PSA_SUCCESS ); } -#else +#else /* MBEDTLS_PSA_CRYPTO_SE_C */ (void) p_drv; +#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ + #if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) /* Key location for external keys gets checked by the wrapper */ return( PSA_SUCCESS ); -#else +#else /* MBEDTLS_PSA_CRYPTO_DRIVERS */ + /* No support for external lifetimes at all, or dynamic interface + * did not find driver for requested lifetime. */ return( PSA_ERROR_INVALID_ARGUMENT ); -#endif -#endif /* MBEDTLS_PSA_CRYPTO_SE_C */ +#endif /* MBEDTLS_PSA_CRYPTO_DRIVERS */ } else /* Local/internal keys are always valid */ From d80e8a41122a72588d6595e870b7caa6d6313756 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 26 Jan 2021 12:45:39 +0100 Subject: [PATCH 3/4] Check for existence of key material on store/load Signed-off-by: Steven Cooreman --- library/psa_crypto_slot_management.c | 6 ------ library/psa_crypto_storage.c | 11 ++++++++++- library/psa_crypto_storage.h | 11 ++++++++--- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 3797dea9c6..39d6dbb36c 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -269,12 +269,6 @@ static psa_status_t psa_load_persistent_key_into_slot( psa_key_slot_t *slot ) } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - if ( key_data == NULL ) - { - status = PSA_ERROR_STORAGE_FAILURE; - goto exit; - } - status = psa_copy_key_material_into_slot( slot, key_data, key_data_length ); exit: diff --git a/library/psa_crypto_storage.c b/library/psa_crypto_storage.c index 1ebd20ee37..10a1ad37b4 100644 --- a/library/psa_crypto_storage.c +++ b/library/psa_crypto_storage.c @@ -374,8 +374,12 @@ psa_status_t psa_save_persistent_key( const psa_core_key_attributes_t *attr, uint8_t *storage_data; psa_status_t status; + /* All keys saved to persistent storage always have a key context */ + if( data == NULL || data_length == 0 ) + return( PSA_ERROR_INVALID_ARGUMENT ); + if( data_length > PSA_CRYPTO_MAX_STORAGE_SIZE ) - return PSA_ERROR_INSUFFICIENT_STORAGE; + return( PSA_ERROR_INSUFFICIENT_STORAGE ); storage_data_length = data_length + sizeof( psa_persistent_key_storage_format ); storage_data = mbedtls_calloc( 1, storage_data_length ); @@ -426,6 +430,11 @@ psa_status_t psa_load_persistent_key( psa_core_key_attributes_t *attr, status = psa_parse_key_data_from_storage( loaded_data, storage_data_length, data, data_length, attr ); + /* All keys saved to persistent storage always have a key context */ + if( status == PSA_SUCCESS && + ( *data == NULL || *data_length == 0 ) ) + status = PSA_ERROR_STORAGE_FAILURE; + exit: mbedtls_free( loaded_data ); return( status ); diff --git a/library/psa_crypto_storage.h b/library/psa_crypto_storage.h index fbc94fc387..06128e9938 100644 --- a/library/psa_crypto_storage.h +++ b/library/psa_crypto_storage.h @@ -86,6 +86,9 @@ int psa_is_key_present_in_storage( const mbedtls_svc_key_id_t key ); * already occupied non-persistent key, as well as ensuring the key data is * validated. * + * Note: This function will only succeed for key buffers which are not + * empty. If passed a NULL pointer or zero-length, the function will fail + * with #PSA_ERROR_INVALID_ARGUMENT. * * \param[in] attr The attributes of the key to save. * The key identifier field in the attributes @@ -94,6 +97,7 @@ int psa_is_key_present_in_storage( const mbedtls_svc_key_id_t key ); * \param data_length The number of bytes that make up the key data. * * \retval #PSA_SUCCESS + * \retval #PSA_ERROR_INVALID_ARGUMENT * \retval #PSA_ERROR_INSUFFICIENT_MEMORY * \retval #PSA_ERROR_INSUFFICIENT_STORAGE * \retval #PSA_ERROR_STORAGE_FAILURE @@ -111,9 +115,10 @@ psa_status_t psa_save_persistent_key( const psa_core_key_attributes_t *attr, * metadata and writes them to the appropriate output parameters. * * Note: This function allocates a buffer and returns a pointer to it through - * the data parameter. psa_free_persistent_key_data() must be called after - * this function to zeroize and free this buffer, regardless of whether this - * function succeeds or fails. + * the data parameter. On succesful return, the pointer is guaranteed to be + * valid and contain at least one byte of data. + * psa_free_persistent_key_data() must be called on the data buffer + * afterwards to zeroize and free this buffer. * * \param[in,out] attr On input, the key identifier field identifies * the key to load. Other fields are ignored. From 7dadf14e7b93015814b0726d164f104da0d74f53 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 28 Jan 2021 19:46:52 +0100 Subject: [PATCH 4/4] Minor language correction after review Signed-off-by: Steven Cooreman --- library/psa_crypto_storage.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto_storage.h b/library/psa_crypto_storage.h index 06128e9938..1d597c090b 100644 --- a/library/psa_crypto_storage.h +++ b/library/psa_crypto_storage.h @@ -115,8 +115,8 @@ psa_status_t psa_save_persistent_key( const psa_core_key_attributes_t *attr, * metadata and writes them to the appropriate output parameters. * * Note: This function allocates a buffer and returns a pointer to it through - * the data parameter. On succesful return, the pointer is guaranteed to be - * valid and contain at least one byte of data. + * the data parameter. On successful return, the pointer is guaranteed to be + * valid and the buffer contains at least one byte of data. * psa_free_persistent_key_data() must be called on the data buffer * afterwards to zeroize and free this buffer. *