Merge pull request #221 from gilles-peskine-arm/annotate_todo_comment…

…s-20190813

SE keys: fix psa_destroy_key; add Github issue numbers for missing code

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.
+ *
+ * <h3>Design notes about multipart operation structures</h3>
+ *
+ * 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

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. */
@@ -1016,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 */
@@ -1043,42 +1041,57 @@ 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_PERSISTENT )
+    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
+         */
     }
 #endif /* defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) */
 
 #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 )
@@ -1201,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 */
@@ -1722,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
@@ -6075,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. */

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 );
+}
+
 
 
 /****************************************************************/
@@ -793,6 +804,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 +878,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( );
@@ -892,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( ) );
@@ -923,6 +941,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( );
@@ -986,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( ) );
@@ -1016,6 +1038,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( );
@@ -1208,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;
@@ -1250,6 +1276,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 );