Secure element key creation foundation #157

gilles-peskine-arm · 2019-06-26T18:18:26Z

This PR lays some foundations for creating a key in a secure element. It's work in progress, not yet usable.

What it does so far:

Support for import, export and destroy. Any other function on a secure element key is likely to cause a crash.
The metadata about SE keys is stored persistently. Not all failure cases are handled.
Under the hood, there's a mechanism that functions that operate on keys can use to find out which SE driver handles a key if any (implemented for export). Every such function will need to have this mechanism. I don't think the existing code is the final word on this.
The core stores a persistent data buffer on behalf of the driver. The content of the buffer is fully up to the driver. It can be, for example, a slot usage map.
Slot allocation is fully up to the driver. The core stores a 64-bit “slot number” for each key.

This PR has reached the goals I wanted to reach for a foundation:

Provide enough working code to pass a smoke test (I chose import-export-destroy).
Implement enough of the API to see if the information is in the right place where it's needed. I haven't yet implemented the two-phase commit that is necessary to handle power failures during key creation or destruction, but I'm confident that it can be implemented with the current API.

I think the path towards getting this merged is to implement just enough that doing something that isn't implemented yet on a key that is in a secure element would return an error instead of crashing. Implementing more secure element access hooks at this point would be premature: first we should beef up testing.

History:

Original https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-se_driver-create_key-2
- Secure element drivers must declare how many “slots” they support, and the core maintains a bitmap indicating which slots are free.
- Secure element drivers can enforce restrictions on which slot numbers can store which types of keys.
2019-07-12
- The core stores a persistent data buffer on behalf of the driver. The content of the buffer is fully up to the driver. It can be, for example, a slot usage map.
- Slot allocation is fully up to the driver. The core stores a 64-bit “slot number” for each key.

AndrzejKurek · 2019-07-01T13:17:56Z

library/psa_crypto_se.c

+    psa_key_lifetime_t lifetime )
+{
+    size_t i;
+    if( lifetime == 0 )


I have the same remark as in a different PR - I would suggest swapping zero with a defined constant to signal what does this value mean.

Here 0 means no entry. I'll add a comment to explain.

AndrzejKurek · 2019-07-01T13:18:48Z

library/psa_crypto_se.c

@@ -83,4 +123,10 @@ void psa_unregister_all_se_drivers( void )
    memset( driver_table, 0, sizeof( driver_table ) );
 }

+
+
+/****************************************************************/


What is the reason behind this comment?

Makes it visible that this finishes the "Driver registration" section.

It's the end of the file anyway and there's nothing after this section, that's why I ask. It seems redundant to me.

No changes planned here.

AndrzejKurek · 2019-07-01T13:25:58Z

library/psa_crypto.c

    if( slot == NULL )
        return;
+
+#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
+    /* TOnogrepDO: If the key has already been created in the secure


This shouldn't be here, at least not in such strange form.

That's why it's a TODO. Spelled this way so that check-files doesn't tell me something I already know.

Why not fix check-files in a separate commit instead?

Also, please add a reference in this code comment to a GitHub issue for this TODO, if it's important enough to track fixing.

It was a conscious decision to reject TODO in check-files.sh, so I'm respecting it. Changing our criteria for acceptable code is out of scope of this PR.

TOnogrepDO will stay in this PR. When Mbed-TLS/mbedtls#2730 makes its way to this branch, we can use TODO.

AndrzejKurek · 2019-07-01T13:33:34Z

library/psa_crypto.c

+            slot->data.se.slot_number,
+            slot->lifetime, slot->type, slot->policy.alg, slot->policy.usage,
+            data, data_length );
+        /* TOnogrepDO: psa_check_key_slot_attributes? */


This shouldn't be here, at least not in such strange form.

Will be resolved in a follow-up PR.

AndrzejKurek · 2019-07-01T13:35:33Z

include/psa/crypto_se_driver.h

+ * \param before            The end of the range.
+ *                          It is not included in the search.
+ * \param[out] found        On success, a free slot number such that
+ *                          `from <= found < before`.


What is the reason behind such rule? It will be hard to remember, since it's inconsistent even within this one function call.

I don't understand. Why is it so strange that the slot is within an interval, since that's the whole point of the function? What inconsistency are you talking about?

I'm talking about having the beginning of the range inclusive, but the end - not.

That's the usual way to do ranges, so that end - start = length. What would you expect?

I would expect having the same rules for both sides of the range. I'm not acquainted with other crypto APIs though, so I won't press on changing it.

It's not about crypto APIs, it's about array slices. Most array slicing APIs take (start_inclusive, number_of_items) or (start_inclusive, end_exclusive) as parameters, with end_exclusive = start_inclusive + number_of_items. If you use start_inclusive and end_inclusive, a lot of operations need a +1 or -1, which is awkward and error-prone.

But maybe this would look more familiar with a length parameter instead of an end parameter. I'm considering changing it.

Moot because this interface has been completely redesigned.

AndrzejKurek · 2019-07-01T13:36:46Z

include/psa/crypto_se_driver.h

+ *         Success. \c *found contains a slot number which is between
+ *         \p from and \p before-1 inclusive and which is currently free.
+ * \retval #PSA_ERROR_INSUFFICIENT_STORAGE
+ *         All slots in the indicated range are occupied.


Minor suggestion: indicated -> requested

Moot because this chunk of text is no longer present.

AndrzejKurek · 2019-07-01T13:46:08Z

library/psa_crypto_se.c

+        return( PSA_ERROR_INSUFFICIENT_MEMORY );
+    drv->slot_usage->size = drv->methods->key_management->slot_count;
+
+    /* TOnogrepDO: load */


Leftover comment

No, it's still to do.

Should this PR be blocked on loading not being done yet?

It shouldn't be merged as is because it includes a feature that's on by default (SE support) but leads to undefined behavior if used according to the documentation. I don't have precise criteria in mind for when this PR will be mergeable: ideally it would do everything but I also want it to be usable ASAP.

I'd recommend not making this PR do everything as a precondition to merging.

Let's try to pick a safe subset to enable by default, and implement that bit. Or, make this off by default until we are comfortable enough with the feature to turn it on by default. The latter approach is quite common and widely known in the industry as a "feature toggle".

We don't want a repeat of PRs we've had trouble merging in the past: ones that keep growing bigger and bigger, and more and more stale.

I'm not planning to remove TOnogrepDO's from this PR in general. This particular one has actually been done.

AndrzejKurek · 2019-07-01T14:15:24Z

library/psa_crypto_se.c

+#define mbedtls_free   free
+#endif
+
+


Triple whitespace

What's triple whitespace and why is it a problem?

Pars pro toto - Three consecutive newline whitespaces. I'm quite certain I haven't seen these in any of the previous MbedTLS files, they seem a bit spacious. Not sure if we have any rule against it though. Pointing it out just in case it's an oversight.

No changes planned.

Maybe we should make this 7 consecutive newlines...

7 is better than 3

library/psa_crypto_se.c

Patater · 2019-07-01T17:17:07Z

library/psa_crypto.c

    if( slot == NULL )
        return;
+
+#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
+    /* TOnogrepDO: If the key has already been created in the secure


Why not fix check-files in a separate commit instead?

Also, please add a reference in this code comment to a GitHub issue for this TODO, if it's important enough to track fixing.

library/psa_crypto_se.c

Patater · 2019-07-01T17:19:22Z

library/psa_crypto_se.c

+        return( PSA_ERROR_INSUFFICIENT_MEMORY );
+    drv->slot_usage->size = drv->methods->key_management->slot_count;
+
+    /* TOnogrepDO: load */


Should this PR be blocked on loading not being done yet?

library/psa_crypto_se.c

include/psa/crypto_se_driver.h

AndrzejKurek · 2019-07-10T13:16:29Z

library/psa_crypto.c

+            status = PSA_ERROR_NOT_SUPPORTED;
+            goto exit;
+        }
+        status = drv->key_management->p_import(


Bug found by 028eb3d: Lack of error checking

What error checking specifically? Since we already have a test, whatever that bug is, I propose to fix it in a follow-up PR.

Expose the type of an entry in the SE driver table as an opaque type to other library modules. Soon, driver table entries will have state, and callers will need to be able to access this state through functions using this opaque type. Provide functions to look up a driver by its lifetime and to retrieve the method table from an entry.

Instead of having one giant table containing all possible methods, represent a driver's method table as a structure containing pointers to substructures. This way a driver that doesn't implement a certain class of operations can use NULL for this class as a whole instead of storing NULL for each method.

When creating a key with a lifetime that places it in a secure element, retrieve the appropriate driver table entry. This commit doesn't yet achieve behavior: so far the code only retrieves the driver, it doesn't call the driver.

This slightly increases storage requirements, but works in more use cases. In particular, it allows drivers to treat choose slot numbers with a monotonic counter that is incremented each time a key is created, without worrying about overflow in practice.

Define a structure that is to be instantiated once per driver instance. Define a driver initialization method and pass it the driver context.

Pass the driver context to all driver methods except the ones that operate on an already-setup operation context. Rename `p_context` arguments to `op_context` to avoid confusion between contexts.

Add a driver method to allocate a key slot for a key that is about to be created.

Most driver methods are not allowed to modify the persistent data, so the driver context structure contains a const pointer to it. Pass a non-const pointer to the persstent data to the driver methods that need it: init, allocate, destroy.

Create the driver context when registering the driver. Implement some helper functions to access driver information.

Since driver table entries contain the driver context, which is mutable, they can't be const anymore.

Be stylistically consistent.

Explain what it can be used for and when it is saved to storage.

gilles-peskine-arm · 2019-07-25T16:04:58Z

@Patater @AndrzejKurek Thanks for the reviews. I believe I've addressed all your comments except for a few where I'm still waiting for further feedback.

Patater · 2019-07-26T10:39:31Z

library/psa_crypto_storage.h

+ * If the system crashes while a transaction is in progress, psa_crypto_init()
+ * calls psa_crypto_load_transaction() and takes care of completing or
+ * rewinding the transaction. This is done in psa_crypto_recover_transaction()
+ * in psa_crypto.c. If you add a new type of transactions, be


- If you add a new type of transactions + If you add a new type of transaction

Fixed by amending the commit.

library/psa_crypto_slot_management.h

Patater

LGTM

Just one English nit and the rest are non-blocking issues.

Nothing has been saved to disk yet, but there is stale data in psa_crypto_transaction. This stale data should not be reused, but do wipe it to reduce the risk of it mattering somehow in the future.

gilles-peskine-arm requested review from andresag01 and AndrzejKurek June 26, 2019 18:18

AndrzejKurek reviewed Jul 1, 2019

View reviewed changes

Patater reviewed Jul 1, 2019

View reviewed changes

AndrzejKurek reviewed Jul 10, 2019

View reviewed changes

gilles-peskine-arm added 13 commits July 12, 2019 11:47

Add public-key export method

e62b74e

Look up the SE driver when creating a key

011e428

When creating a key with a lifetime that places it in a secure element, retrieve the appropriate driver table entry. This commit doesn't yet achieve behavior: so far the code only retrieves the driver, it doesn't call the driver.

Define a driver context structure type

7a86da1

Define a structure that is to be instantiated once per driver instance. Define a driver initialization method and pass it the driver context.

Pass the driver context to most driver methods

8597bc1

Pass the driver context to all driver methods except the ones that operate on an already-setup operation context. Rename `p_context` arguments to `op_context` to avoid confusion between contexts.

New driver method: allocate

f2223c8

Add a driver method to allocate a key slot for a key that is about to be created.

Driver context manipulation functions

5243a20

Create the driver context when registering the driver. Implement some helper functions to access driver information.

Driver table entries are now mutable

8abe6a2

Since driver table entries contain the driver context, which is mutable, they can't be const anymore.

SE keys: store the slot number in the memory slot

73167e1

SE keys: allocate a slot before creating the key

cbaff46

gilles-peskine-arm added 8 commits July 25, 2019 11:32

psa_key_slot_is_external exists. Use it.

adad813

Bug fix: save the driver's persistent data in destroy_key

725f22a

Improve comments

60450a4

Don't explicitly dereference function pointers

2e0f388

Be stylistically consistent.

Improve documentation of SE driver persistent state

0c3ae1f

Explain what it can be used for and when it is saved to storage.

psa_destroy_se_key: explain why the error is NOT_PERMITTED

340b127

Bug fix: don't start a transaction for non-SE keys

4aea103

Add boilerplate to recover a transaction during init

f9bb29e

gilles-peskine-arm removed the needs: work The pull request needs rework before it can be merged. label Jul 25, 2019

gilles-peskine-arm requested review from Patater and AndrzejKurek July 25, 2019 16:03

AndrzejKurek previously approved these changes Jul 26, 2019

View reviewed changes

Patater reviewed Jul 26, 2019

View reviewed changes

Patater previously approved these changes Jul 26, 2019

View reviewed changes

gilles-peskine-arm dismissed stale reviews from Patater and AndrzejKurek via 66be51c July 26, 2019 11:23

gilles-peskine-arm force-pushed the psa-se_driver-create_key branch from cb2e247 to 66be51c Compare July 26, 2019 11:23

gilles-peskine-arm added 2 commits July 26, 2019 13:23

Improve documentation of transaction storage

2ea06fd

If starting a transaction fails, wipe the transaction data

66be51c

Nothing has been saved to disk yet, but there is stale data in psa_crypto_transaction. This stale data should not be reused, but do wipe it to reduce the risk of it mattering somehow in the future.

gilles-peskine-arm requested review from Patater and AndrzejKurek July 26, 2019 11:23

Patater approved these changes Jul 26, 2019

View reviewed changes

Patater added the needs: ci Needs a passing full CI run label Jul 26, 2019

gilles-peskine-arm removed needs: ci Needs a passing full CI run needs: review The pull request is ready for review. This generally means that it has no known issues. labels Jul 26, 2019

gilles-peskine-arm merged commit adb1c52 into ARMmbed:psa-api-1.0-beta Jul 26, 2019

gilles-peskine-arm mentioned this pull request Aug 2, 2019

Add mock tests for hooks for secure element drivers #174

Merged

This was referenced Aug 7, 2019

Update the example to the new driver API ARMmbed/mbed-os-example-atecc608a#10

Merged

Adjust the driver to the new PSA API ARMmbed/mbed-os-atecc608a#7

Merged

		#define mbedtls_free free
		#endif

Secure element key creation foundation #157

Secure element key creation foundation #157

Uh oh!

Conversation

gilles-peskine-arm commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm commented Jun 26, 2019 •

edited

Loading

AndrzejKurek Jul 1, 2019 •

edited

Loading

Patater Jul 2, 2019 •

edited

Loading

AndrzejKurek Jul 10, 2019 •

edited

Loading