Skip to content

Secure element keys: save the key size #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 5, 2019
Merged

Secure element keys: save the key size #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
424f894
SE keys: store the bit size internally (partial implementation)
gilles-peskine-arm Jul 15, 2019
dc5bfe9
SE keys: implement and test psa_get_key_attributes
gilles-peskine-arm Jul 24, 2019
1801740
SE driver: report the bit size on key import
gilles-peskine-arm Jul 24, 2019
e60d1d0
SE keys: save the bit size in storage
gilles-peskine-arm Jul 24, 2019
fc321f1
SE keys: test that the bit size is saved and loaded correctly
gilles-peskine-arm Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions include/psa/crypto_se_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
69 changes: 38 additions & 31 deletions library/psa_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: For the sake of consistency, this could be tested for equality explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function returns a boolean with the type int (since we're still not using stdbool). Mbed TLS usually (but not fully consistently) uses explicit comparison to 0 even for booleans, but PSA crypto code doesn't.

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)
Expand Down Expand Up @@ -1140,10 +1145,10 @@ static psa_status_t psa_get_rsa_public_exponent(
}
#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 )
Expand All @@ -1152,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.
Expand All @@ -1164,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;
Expand Down Expand Up @@ -1489,6 +1500,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 */

Expand Down Expand Up @@ -1523,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 );
Expand Down Expand Up @@ -1696,19 +1703,19 @@ 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 */
{
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:
Expand Down
1 change: 1 addition & 0 deletions library/psa_crypto_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ typedef struct
struct se
{
psa_key_slot_number_t slot_number;
size_t bits;
} se;
} data;
} psa_key_slot_t;
Expand Down
5 changes: 2 additions & 3 deletions library/psa_crypto_slot_management.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
51 changes: 48 additions & 3 deletions tests/suites/test_suite_psa_crypto_se_driver_hal.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 );
}

Expand Down Expand Up @@ -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 ) );
Expand All @@ -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 );
Expand Down Expand Up @@ -178,6 +183,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. */
Expand Down Expand Up @@ -445,6 +485,11 @@ 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. */
psa_set_key_bits( &attributes,
PSA_BYTES_TO_BITS( sizeof( key_material ) ) );
if( ! check_key_attributes( handle, &attributes ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this put inside an assertion macro?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion macros are in check_key_attributes. That function returns ok/not-ok, and if it returns not-ok, the test failure data has already been recorded, so test_fail should not be called again.

goto exit;
PSA_ASSERT( psa_export_key( handle,
exported, sizeof( exported ),
&exported_length ) );
Expand Down