Merge pull request #3382 from stevew817/feature/volatile-keys-in-SE

Support volatile keys in external SE
2020-06-26 20:27:11 +02:00 · 2020-06-26 20:27:11 +02:00 · 961914df12
--- a/ChangeLog.d/do_not_persist_volatile_external_keys.txt
+++ b/ChangeLog.d/do_not_persist_volatile_external_keys.txt
@ -0,0 +1,4 @@
+Default behavior changes
+   * Stop storing persistent information about externally stored keys created
+     through PSA Crypto with a volatile lifetime. Reported in #3288 and
+     contributed by Steven Cooreman in #3382.
--- a/include/psa/crypto_values.h
+++ b/include/psa/crypto_values.h
@ -1611,7 +1611,7 @@
 */
 #define PSA_KEY_LIFETIME_IS_VOLATILE(lifetime)  \
    (PSA_KEY_LIFETIME_GET_PERSISTENCE(lifetime) == \
-     PSA_KEY_LIFETIME_PERSISTENCE_VOLATILE)
+     PSA_KEY_PERSISTENCE_VOLATILE)

 /** Construct a lifetime from a persistence level and a location.
 *
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@ -1499,16 +1499,17 @@ static psa_status_t psa_validate_key_attributes(
    const psa_key_attributes_t *attributes,
    psa_se_drv_table_entry_t **p_drv )
 {
-    psa_status_t status;
+    psa_status_t status = PSA_ERROR_INVALID_ARGUMENT;

-    if( attributes->core.lifetime != PSA_KEY_LIFETIME_VOLATILE )
-    {
-        status = psa_validate_persistent_key_parameters(
-            attributes->core.lifetime, attributes->core.id,
-            p_drv, 1 );
+    status = psa_validate_key_location( psa_get_key_lifetime( attributes ),
+                                        p_drv );
+    if( status != PSA_SUCCESS )
+        return( status );
+
+    status = psa_validate_key_persistence( psa_get_key_lifetime( attributes ),
+                                           psa_get_key_id( attributes ) );
    if( status != PSA_SUCCESS )
        return( status );
-    }

    status = psa_validate_key_policy( &attributes->core.policy );
    if( status != PSA_SUCCESS )
@ -1594,11 +1595,14 @@ static psa_status_t psa_start_key_creation(

 #if defined(MBEDTLS_PSA_CRYPTO_SE_C)
    /* For a key in a secure element, we need to do three things
-     * when creating or registering a key:
+     * when creating or registering a persistent key:
     * create the key file in internal storage, create the
     * key inside the secure element, and update the driver's
-     * persistent data. Start a transaction that will encompass these
-     * three actions. */
+     * persistent data. This is done by starting a transaction that will
+     * encompass these three actions.
+     * For registering a volatile key, we just need to find an appropriate
+     * slot number inside the SE. Since the key is designated volatile, creating
+     * a transaction is not required. */
    /* The first thing to do is to find a slot number for the new key.
     * We save the slot number in persistent storage as part of the
     * transaction data. It will be needed to recover if the power
@ -1613,6 +1617,9 @@ static psa_status_t psa_start_key_creation(
                                           &slot->data.se.slot_number );
        if( status != PSA_SUCCESS )
            return( status );
+
+        if( ! PSA_KEY_LIFETIME_IS_VOLATILE( attributes->core.lifetime ) )
+        {
            psa_crypto_prepare_transaction( PSA_CRYPTO_TRANSACTION_CREATE_KEY );
            psa_crypto_transaction.key.lifetime = slot->attr.lifetime;
            psa_crypto_transaction.key.slot = slot->data.se.slot_number;
@ -1624,6 +1631,7 @@ static psa_status_t psa_start_key_creation(
                return( status );
            }
        }
+    }

    if( *p_drv == NULL && method == PSA_KEY_CREATION_REGISTER )
    {
@ -1661,7 +1669,7 @@ static psa_status_t psa_finish_key_creation(
    (void) driver;

 #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C)
-    if( slot->attr.lifetime != PSA_KEY_LIFETIME_VOLATILE )
+    if( ! PSA_KEY_LIFETIME_IS_VOLATILE( slot->attr.lifetime ) )
    {
 #if defined(MBEDTLS_PSA_CRYPTO_SE_C)
        if( driver != NULL )
@ -1709,8 +1717,8 @@ static psa_status_t psa_finish_key_creation(
    /* Finish the transaction for a key creation. This does not
     * happen when registering an existing key. Detect this case
     * by checking whether a transaction is in progress (actual
-     * creation of a key in a secure element requires a transaction,
-     * but registration doesn't use one). */
+     * creation of a persistent key in a secure element requires a transaction,
+     * but registration or volatile key creation doesn't use one). */
    if( driver != NULL &&
        psa_crypto_transaction.unknown.type == PSA_CRYPTO_TRANSACTION_CREATE_KEY )
    {
--- a/library/psa_crypto_slot_management.c
+++ b/library/psa_crypto_slot_management.c
@ -184,36 +184,53 @@ static int psa_is_key_id_valid( psa_key_file_id_t file_id,
 }
 #endif /* defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) */

-psa_status_t psa_validate_persistent_key_parameters(
-    psa_key_lifetime_t lifetime,
-    psa_key_file_id_t id,
-    psa_se_drv_table_entry_t **p_drv,
-    int creating )
+psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime,
+                                        psa_se_drv_table_entry_t **p_drv )
 {
-    if( p_drv != NULL )
-        *p_drv = NULL;
-#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
-    if( psa_key_lifetime_is_external( lifetime ) )
+    if ( psa_key_lifetime_is_external( lifetime ) )
    {
-        *p_drv = psa_get_se_driver_entry( lifetime );
-        if( *p_drv == NULL )
+#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
+        psa_se_drv_table_entry_t *driver = psa_get_se_driver_entry( lifetime );
+        if( driver == NULL )
            return( PSA_ERROR_INVALID_ARGUMENT );
+        else
+        {
+            if (p_drv != NULL)
+                *p_drv = driver;
+            return( PSA_SUCCESS );
+        }
+#else
+        (void) p_drv;
+        return( PSA_ERROR_INVALID_ARGUMENT );
+#endif /* MBEDTLS_PSA_CRYPTO_SE_C */
    }
    else
-#endif /* MBEDTLS_PSA_CRYPTO_SE_C */
-    if( lifetime != PSA_KEY_LIFETIME_PERSISTENT )
-        return( PSA_ERROR_INVALID_ARGUMENT );
-
-#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C)
-    if( ! psa_is_key_id_valid( id, ! creating ) )
-        return( PSA_ERROR_INVALID_ARGUMENT );
+        /* Local/internal keys are always valid */
        return( PSA_SUCCESS );
+}

+psa_status_t psa_validate_key_persistence( psa_key_lifetime_t lifetime,
+                                           psa_key_id_t key_id )
+{
+    if ( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) )
+    {
+        /* Volatile keys are always supported */
+        return( PSA_SUCCESS );
+    }
+    else
+    {
+        /* Persistent keys require storage support */
+#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C)
+        if( psa_is_key_id_valid( key_id,
+                                 psa_key_lifetime_is_external( lifetime ) ) )
+            return( PSA_SUCCESS );
+        else
+            return( PSA_ERROR_INVALID_ARGUMENT );
 #else /* MBEDTLS_PSA_CRYPTO_STORAGE_C */
-    (void) id;
-    (void) creating;
+        (void) key_id;
        return( PSA_ERROR_NOT_SUPPORTED );
 #endif /* !MBEDTLS_PSA_CRYPTO_STORAGE_C */
+    }
 }

 psa_status_t psa_open_key( psa_key_file_id_t id, psa_key_handle_t *handle )
@ -224,10 +241,8 @@ psa_status_t psa_open_key( psa_key_file_id_t id, psa_key_handle_t *handle )

    *handle = 0;

-    status = psa_validate_persistent_key_parameters(
-        PSA_KEY_LIFETIME_PERSISTENT, id, NULL, 0 );
-    if( status != PSA_SUCCESS )
-        return( status );
+    if( ! psa_is_key_id_valid( id, 1 ) )
+        return( PSA_ERROR_INVALID_ARGUMENT );

    status = psa_get_empty_key_slot( handle, &slot );
    if( status != PSA_SUCCESS )
--- a/library/psa_crypto_slot_management.h
+++ b/library/psa_crypto_slot_management.h
@ -89,42 +89,40 @@ psa_status_t psa_get_empty_key_slot( psa_key_handle_t *handle,
 */
 static inline int psa_key_lifetime_is_external( psa_key_lifetime_t lifetime )
 {
-    return( lifetime != PSA_KEY_LIFETIME_VOLATILE &&
-            lifetime != PSA_KEY_LIFETIME_PERSISTENT );
+    return( PSA_KEY_LIFETIME_GET_LOCATION( lifetime )
+                != PSA_KEY_LOCATION_LOCAL_STORAGE );
 }

-/** Test whether the given parameters are acceptable for a persistent key.
+/** Validate a key's location.
 *
- * This function does not access the storage in any way. It only tests
- * whether the parameters are meaningful and permitted by general policy.
- * It does not test whether the a file by the given id exists or could be
- * created.
+ * This function checks whether the key's attributes point to a location that
+ * is known to the PSA Core, and returns the driver function table if the key
+ * is to be found in an external location.
 *
- * If the key is in external storage, this function returns the corresponding
- * driver.
+ * \param[in] lifetime      The key lifetime attribute.
+ * \param[out] p_drv        On success, when a key is located in external
+ *                          storage, returns a pointer to the driver table
+ *                          associated with the key's storage location.
 *
- * \param lifetime      The lifetime to test.
- * \param id            The key id to test.
- * \param[out] p_drv    On output, if \p lifetime designates a key
- *                      in an external processor, \c *p_drv is a pointer
- *                      to the driver table entry fot this lifetime.
- *                      If \p lifetime designates a transparent key,
- *                      \c *p_drv is \c NULL.
- * \param creating      0 if attempting to open an existing key.
- *                      Nonzero if attempting to create a key.
- *
- * \retval PSA_SUCCESS
- *         The given parameters are valid.
- * \retval PSA_ERROR_INVALID_ARGUMENT
- *         \p lifetime is volatile or is invalid.
- * \retval PSA_ERROR_INVALID_ARGUMENT
- *         \p id is invalid.
+ * \retval #PSA_SUCCESS
+ * \retval #PSA_ERROR_INVALID_ARGUMENT
 */
-psa_status_t psa_validate_persistent_key_parameters(
-    psa_key_lifetime_t lifetime,
-    psa_key_file_id_t id,
-    psa_se_drv_table_entry_t **p_drv,
-    int creating );
+psa_status_t psa_validate_key_location( psa_key_lifetime_t lifetime,
+                                        psa_se_drv_table_entry_t **p_drv );
+
+/** Validate that a key's persistence attributes are valid.
+ *
+ * This function checks whether a key's declared persistence level and key ID
+ * attributes are valid and known to the PSA Core in its actual configuration.
+ *
+ * \param[in] lifetime    The key lifetime attribute.
+ * \param[in] key_id      The key ID attribute
+ *
+ * \retval #PSA_SUCCESS
+ * \retval #PSA_ERROR_INVALID_ARGUMENT
+ */
+psa_status_t psa_validate_key_persistence( psa_key_lifetime_t lifetime,
+                                           psa_key_id_t key_id );


 #endif /* PSA_CRYPTO_SLOT_MANAGEMENT_H */
--- a/tests/scripts/docker_env.sh
+++ b/tests/scripts/docker_env.sh
@ -60,12 +60,19 @@ else
    DOCKER="sudo docker"
 fi

+# Figure out the number of processors available
+if [ "$(uname)" == "Darwin" ]; then
+    NUM_PROC="$(sysctl -n hw.logicalcpu)"
+else
+    NUM_PROC="$(nproc)"
+fi
+
 # Build the Docker image
 echo "Getting docker image up to date (this may take a few minutes)..."
 ${DOCKER} image build \
    -t ${DOCKER_IMAGE_TAG} \
    --cache-from=${DOCKER_IMAGE_TAG} \
-    --build-arg MAKEFLAGS_PARALLEL="-j $(nproc)" \
+    --build-arg MAKEFLAGS_PARALLEL="-j ${NUM_PROC}" \
    --network host \
    ${http_proxy+--build-arg http_proxy=${http_proxy}} \
    ${https_proxy+--build-arg https_proxy=${https_proxy}} \
--- a/tests/suites/test_suite_psa_crypto_se_driver_hal.data
+++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.data
@ -24,17 +24,29 @@ register_twice:3
 Register SE driver: maximum number of drivers
 register_max:

-SE key import-export (p_allocate allows all slots)
-key_creation_import_export:0:0
+SE key import-export persistent (p_allocate allows all slots)
+key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:0:0

-SE key import-export (p_allocate allows 1 slot)
-key_creation_import_export:ARRAY_LENGTH( ram_slots ) - 1:0
+SE key import-export persistent (p_allocate allows 1 slot)
+key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:ARRAY_LENGTH( ram_slots ) - 1:0

-SE key import-export, check after restart (slot 0)
-key_creation_import_export:0:1
+SE key import-export persistent, check after restart (slot 0)
+key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:0:1

-SE key import-export, check after restart (slot 3)
-key_creation_import_export:3:1
+SE key import-export persistent, check after restart (slot 3)
+key_creation_import_export:TEST_SE_PERSISTENT_LIFETIME:3:1
+
+SE key import-export volatile (p_allocate allows all slots)
+key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:0:0
+
+SE key import-export volatile (p_allocate allows 1 slot)
+key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:ARRAY_LENGTH( ram_slots ) - 1:0
+
+SE key import-export volatile, check after restart (slot 0)
+key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:0:1
+
+SE key import-export volatile, check after restart (slot 3)
+key_creation_import_export:TEST_SE_VOLATILE_LIFETIME:3:1

 Key creation in a specific slot (0)
 key_creation_in_chosen_slot:0:0:PSA_SUCCESS
@ -118,22 +130,28 @@ Key generation smoke test: HMAC-SHA-256
 generate_key_smoke:PSA_KEY_TYPE_HMAC:256:PSA_ALG_HMAC( PSA_ALG_SHA_256 )

 Key registration: smoke test
-register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:PSA_SUCCESS
+register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:1:PSA_SUCCESS

-Key registration: invalid lifetime (volatile)
-register_key_smoke_test:PSA_KEY_LIFETIME_VOLATILE:1:PSA_ERROR_INVALID_ARGUMENT
+Key registration: invalid lifetime (volatile internal storage)
+register_key_smoke_test:PSA_KEY_LIFETIME_VOLATILE:1:1:PSA_ERROR_INVALID_ARGUMENT

 Key registration: invalid lifetime (internal storage)
-register_key_smoke_test:PSA_KEY_LIFETIME_PERSISTENT:1:PSA_ERROR_INVALID_ARGUMENT
+register_key_smoke_test:PSA_KEY_LIFETIME_PERSISTENT:1:1:PSA_ERROR_INVALID_ARGUMENT

 Key registration: invalid lifetime (no registered driver)
-register_key_smoke_test:PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( PSA_KEY_PERSISTENCE_DEFAULT, TEST_DRIVER_LOCATION + 1 ):1:PSA_ERROR_INVALID_ARGUMENT
+register_key_smoke_test:PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( PSA_KEY_PERSISTENCE_DEFAULT, TEST_DRIVER_LOCATION + 1 ):1:1:PSA_ERROR_INVALID_ARGUMENT

 Key registration: rejected
-register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:0:PSA_ERROR_NOT_PERMITTED
+register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:0:PSA_ERROR_NOT_PERMITTED

 Key registration: not supported
-register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:-1:PSA_ERROR_NOT_SUPPORTED
+register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:-1:PSA_ERROR_NOT_SUPPORTED
+
+Key registration: key id out of range
+register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:PSA_KEY_ID_VENDOR_MAX+1:-1:PSA_ERROR_INVALID_ARGUMENT
+
+Key registration: key id in vendor range
+register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:PSA_KEY_ID_VENDOR_MAX:1:PSA_SUCCESS

 Import-sign-verify: sign in driver, ECDSA
 depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
--- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function
+++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function
@ -27,6 +27,10 @@
    ( PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(           \
        PSA_KEY_PERSISTENCE_DEFAULT, TEST_DRIVER_LOCATION ) )

+#define TEST_SE_VOLATILE_LIFETIME                               \
+    ( PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(           \
+        PSA_KEY_PERSISTENCE_VOLATILE, TEST_DRIVER_LOCATION ) )
+
 /** The driver detected a condition that shouldn't happen.
 * This is probably a bug in the library. */
 #define PSA_ERROR_DETECTED_BY_DRIVER ((psa_status_t)( -500 ))
@ -609,6 +613,20 @@ exit:
    return( ok );
 }

+/* Check that no persistent data exists for the given location. */
+static int check_no_persistent_data( psa_key_location_t location )
+{
+    psa_storage_uid_t uid = file_uid_for_location( location );
+    struct psa_storage_info_t info;
+    int ok = 0;
+
+    TEST_EQUAL( psa_its_get_info( uid, &info ), PSA_ERROR_DOES_NOT_EXIST );
+    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. */
@ -829,11 +847,11 @@ exit:
 /* END_CASE */

 /* BEGIN_CASE */
-void key_creation_import_export( int min_slot, int restart )
+void key_creation_import_export( int lifetime_arg, int min_slot, int restart )
 {
    psa_drv_se_t driver;
    psa_drv_se_key_management_t key_management;
-    psa_key_lifetime_t lifetime = TEST_SE_PERSISTENT_LIFETIME;
+    psa_key_lifetime_t lifetime = (psa_key_lifetime_t) lifetime_arg;
    psa_key_location_t location = PSA_KEY_LIFETIME_GET_LOCATION( lifetime );
    psa_key_id_t id = 1;
    psa_key_handle_t handle = 0;
@ -864,10 +882,25 @@ void key_creation_import_export( int min_slot, int restart )
    PSA_ASSERT( psa_import_key( &attributes,
                                key_material, sizeof( key_material ),
                                &handle ) );
+
+
+    if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) )
+    {
+        /* For volatile keys, check no persistent data was created */
+        if( ! check_no_persistent_data( location ) )
+            goto exit;
+    }
+    else
+    {
+        /* For persistent keys, check persistent data */
        if( ! check_persistent_data( location,
                             &ram_shadow_slot_usage,
                             sizeof( ram_shadow_slot_usage ) ) )
            goto exit;
+    }
+
+    /* Test that the key was created in the expected slot. */
+    TEST_EQUAL( ram_slots[min_slot].type, PSA_KEY_TYPE_RAW_DATA );

    /* Maybe restart, to check that the information is saved correctly. */
    if( restart )
@ -875,15 +908,37 @@ void key_creation_import_export( int min_slot, int restart )
        mbedtls_psa_crypto_free( );
        PSA_ASSERT( psa_register_se_driver( location, &driver ) );
        PSA_ASSERT( psa_crypto_init( ) );
+
+        if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) )
+        {
+            /* Check that the PSA core has no knowledge of the volatile key */
+            TEST_ASSERT( psa_open_key( id, &handle ) == PSA_ERROR_DOES_NOT_EXIST );
+
+            /* Drop data from our mockup driver */
+            ram_slots_reset();
+            ram_min_slot = min_slot;
+
+            /* Re-import key */
+            PSA_ASSERT( psa_import_key( &attributes,
+                                key_material, sizeof( key_material ),
+                                &handle ) );
+        }
+        else
+        {
+
+            /* Check we can re-open the persistent key */
            if( ! check_persistent_data( location,
                                         &ram_shadow_slot_usage,
                                         sizeof( ram_shadow_slot_usage ) ) )
                goto exit;
+
+            /* Check that the PSA core still knows about the key */
            PSA_ASSERT( psa_open_key( id, &handle ) );
        }
+    }

    /* Test that the key was created in the expected slot. */
-    TEST_ASSERT( ram_slots[min_slot].type == PSA_KEY_TYPE_RAW_DATA );
+    TEST_EQUAL( ram_slots[min_slot].type, PSA_KEY_TYPE_RAW_DATA );

    /* Test the key attributes, including the reported slot number. */
    psa_set_key_bits( &attributes,
@ -909,7 +964,7 @@ void key_creation_import_export( int min_slot, int restart )
                PSA_ERROR_DOES_NOT_EXIST );

    /* Test that the key has been erased from the designated slot. */
-    TEST_ASSERT( ram_slots[min_slot].type == 0 );
+    TEST_EQUAL( ram_slots[min_slot].type, 0 );

 exit:
    PSA_DONE( );
@ -1263,7 +1318,7 @@ void sign_verify( int flow,
         * generate material, store the desired result of generation in
         * the mock secure element storage. */
        PSA_ASSERT( psa_get_key_attributes( drv_handle, &drv_attributes ) );
-        TEST_ASSERT( key_material->len == PSA_BITS_TO_BYTES( bits ) );
+        TEST_EQUAL( key_material->len, PSA_BITS_TO_BYTES( bits ) );
        memcpy( ram_slots[ram_min_slot].content, key_material->x,
                key_material->len );
    }
@ -1355,6 +1410,7 @@ exit:

 /* BEGIN_CASE */
 void register_key_smoke_test( int lifetime_arg,
+                              int id_arg,
                              int validate,
                              int expected_status_arg )
 {
@ -1364,7 +1420,7 @@ void register_key_smoke_test( int lifetime_arg,
    psa_drv_se_t driver;
    psa_drv_se_key_management_t key_management;
    psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
-    psa_key_id_t id = 1;
+    psa_key_id_t id = id_arg;
    size_t bit_size = 48;
    psa_key_slot_number_t wanted_slot = 0x123456789;
    psa_key_handle_t handle = 0;