Platform hook for built-in keys with slot numbers in drivers #3822

gilles-peskine-arm · 2020-10-26T09:41:39Z

Allow a platform to declare some built-in keys as specified in #3878. This can be, for example, keys that are derived from a hardware unique key, or keys that are pre-provisioned in a secure element.

Unlike #3803, opaque drivers expose built-in keys via a new entry point "get_builtin_key" which identifies the key via a slot number.

Status: work in progress. @stevew817

See include/mbedtls/config.h and include/psa/crypto_extra.h for documentation.
The implementation is untested code which I wrote only to estimate the impact and ensure that data is available where it needs to be. It will need to be heavily rewritten for openless key management.
See tests/src/helpers.c for sample usage. (This code is likely to move to a separate file.)
There are no tests.

stevew817 · 2020-10-26T10:05:38Z

include/mbedtls/config.h

+/** \def MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS
+ *
+ * Enable support for platform built-in keys. If you enable this feature,
+ * you must implement the function mbedtls_psa_platform_get_builtin_key().


I'd like to get an assertion statement in here that the platform function is meant to be autogenerated in the future, as agreed.

The platform function is not meant to be autogenerated. It can also declare non-secure-element keys, and the assignment of key ids is not always automatic.

I'd like to simplify the work for integrators who want to just plug in a driver with a minimum of fuss, but it isn't as simple as making this function autogenerated.

On Thursday, it was still going to be autogenerated:

In the future, this platform callback is intended to be generated from JSON that declares the built-in keys of each driver. There are many details to work out, which is why for the time being platform vendors will hardcode what they need in this callback.

What changed?

I realized that it can't be generated solely from driver descriptions, since keys derived from a HUK would typically not involve a driver.

The answer may be that HUK derivation should be covered by a driver that only provides a key derivation interface. Or that the autogeneration needs an extra hook. Or that there should be an intermediate interface. Or something else. I'm still not sure what the right thing is at this stage.

keys derived from a HUK wouldn't strictly be builtin keys right? They'd be keys derived from a builtin key. And for that, there's the PSA key derivation interface...

From the point of view of an application, there's typically a “master” device-bound key. Whether this key is stored directly in trusted storage, or derived from a key that's stored in a special location (e.g. fuses), or resides in a secure element, is an implementation detail that the platform code should hide. If the key is derived from a HUK, the application doesn't see the HUK, and in particular can't derive arbitrary keys from the HUK: it only has access to one specific key that's derived from the HUK, the application identity and possibly the device state.

At the end of the day, in order to access any key, there needs to be both a way to know that the key is there, and a way to either access the key or exercise the key. This can be solved by the core using autogeneration for keys exposed by drivers. For derived keys that drivers aren't involved in, such as potentially the use case you're describing, you'd need a provisioning and access wrapper regardless (which IMO is not part of PSA Crypto's mission to provide).

There can be autogeneration (and I think there should be) for key access that can be automated. That means keys defined by drivers, potentially mixed with information from application- (or platform-)provided JSON (which is going to happen anyway in order for the application to make sure its algorithmic requirements are provided for) leads to a hardcoded key ID to location/slot map. Potentially with hooks for some type of access control mechanism. That should be enough to set up the mapping to the keys that the hardware driver knows about. The rest is IMO platform-specific logic that would be so broad that there won't be a good way to solve all use cases. For platform-derived keys that don't have anything to do with the hardware, I'd rather think about a different mechanism, since it's a different problem.

stevew817 · 2020-10-26T10:18:44Z

tests/src/helpers.c

+    {0, 0},
+};
+
+psa_status_t mbedtls_psa_platform_get_builtin_key(


If this is going to be autogenerated code, wouldn't it make sense to have it inside the _wrappers.c file?

stevew817 · 2020-10-26T10:22:28Z

tests/src/helpers.c

+    size_t offset = app_key_id - MBEDTLS_PSA_KEY_ID_BUILTIN_MIN + 1;
+    if( offset >= sizeof( builtin_keys ) )
+        return( PSA_ERROR_DOES_NOT_EXIST );
+    const mbedtls_psa_builtin_key_description_t *descr = &builtin_keys[offset];


This means that the builtin_keys array always needs to be as big as the largest offset, even though smaller offsets aren't mapped? This doesn't make sense to me, maybe it's better to add the ID hardcoded into the builtin_keys array too, and do a search instead of a lookup?

Maybe. I meant this code as a demo, not as the only way to do it.

Allow a platform to declare some built-in keys. This can be, for example, keys that are derived from a hardware unique key, or keys that are pre-provisioned in a secure element. This commit adds the documentation only. Subsequent commit will add library support and tests. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Assign a range of key identifiers for built-in keys: MBEDTLS_PSA_KEY_ID_BUILTIN_MIN..MBEDTLS_PSA_KEY_ID_BUILTIN_MAX. Add code to call mbedtls_psa_platform_get_builtin_key() when opening a key. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

It requires the platform integrator to provide an extra function. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Allow opaque drivers to expose keys that were not created through the PSA API. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Work in progress on test code for MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS. Show how mbedtls_psa_platform_get_builtin_key() would typically call a driver's "get_builtin_key" entry point. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg

This looks generally good to me, I've just left a few suggestions for improving the documentation.

include/psa/crypto_extra.h

library/psa_crypto_slot_management.c

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg

Thanks for improving the documentation. Just spotted one incomplete sentence this time, other than that it all looks good to me.

library/psa_crypto_slot_management.c

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2020-11-17T08:56:18Z

@mpg If you're happy with the design, can you please mark #3878 as approved? It has just the PSA specification part, and we'd like to merge that ASAP, whereas the implementation will wait until #3547 is merged (which is why I'm not bothering to make it pass CI).

mpg

Approving for design. CI does not pass yet but this is expected.

ronald-cron-arm · 2020-11-17T13:25:52Z

include/psa/crypto_extra.h

+ *         - #PSA_ERROR_NOT_PERMITTED: the key exists but the requested owner
+ *           is not allowed to access it.
+ */
+psa_status_t mbedtls_psa_platform_get_builtin_key(


I am not very enthousiastic with this platform function that

translate the key identifier to a (location, slot number) pair

call the driver corresponding to location to get the description of the built-in key identified by the slot number and return it.

What about for the platform function to do just 1. Then the core can call the drivers directly to get the key attributes and context.

The signature would be something like:

psa_status_t mbedtls_psa_platform_get_builtin_key( mbedtls_svc_key_id_t id, psa_key_location_t *location, psa_drv_slot_number_t *slot_number);

How does this work when deploying the same code image to multiple device models, some of which have a secure element and some not, so it isn't possible to tell at compile time whether the built-in key is in a secure element or not?

Could you elaborate on the use case here to be sure we agree on what we are talking about?

A device vendor wants to ship the same code image on two device models, one with a secure element and one without. (This is a real use case.) mbedtls_psa_platform_get_builtin_key then needs to have logic like this:

const root_key_slot_number[] = {42}; if driver.query_hardware_availability(): attributes = driver.get_builtin_key_attributes(root_key_slot_number) key_material = &root_key_slot_number; else: attributes = ... key_material = its_read(ROOT_KEY)

ok I understand that. In the case without SE, which entity operates the key? I guess it could be the software implementation or any accelerator.

The key is typically created during manufacturing using proprietary APIs, and the chip vendor writes the support code.

Not sure to understand here, my question is what is the returned location for the key (returned as part of the key attributes) ?

The location would be 0 (local storage).

Arguably we could force “local” built-in keys (such as keys derived from a HUK) to use a separate location and be accessed via the driver interface. But that seems to me like it would impose a significant complexity overhead for a very common feature that doesn't need this kind of complexity.

I think that is a good idea and this does not seem a significant complexity overhead to me.

The device vendor would have to provide two opaque drivers: one for the actual SE and one for the "storage only SE". In both cases, it has to write a get_builtin_key entry point thus the "storage onlyl" case doesn't bring anything new here and thinking about the "local" case as a "storage only SE" looks quite natural to me.

Then the platform specific API in question here would contain only routing code thus would be simpler.

Another point is the handling of buffers. It seems that the current proposal relies on dynamic allocations while if the key attributes and data are retrieved by the core through driver calls we can probably also support the case without dynamic allocations along the lines of what is defined in the "Key format for opaque drivers".

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

daverodgman · 2021-02-22T13:16:09Z

This is superseded by #4160 - closing

gilles-peskine-arm added enhancement needs-design-approval DO-NOT-MERGE component-crypto Crypto primitives and low-level interfaces HwDrivers labels Oct 26, 2020

stevew817 reviewed Oct 26, 2020

View reviewed changes

ronald-cron-arm self-requested a review November 5, 2020 12:36

gilles-peskine-arm added 3 commits November 12, 2020 22:08

Turn MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS off by default

ec1a467

It requires the platform integrator to provide an extra function. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the psa-builtin-keys-via-slot-number branch from 84ae1be to afc67fc Compare November 12, 2020 21:16

gilles-peskine-arm added 2 commits November 12, 2020 22:27

New entry point get_builtin_key for opaque drivers

8f0558e

Allow opaque drivers to expose keys that were not created through the PSA API. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added approved for design needs-preceding-pr Requires another PR to be merged first needs-work and removed DO-NOT-MERGE Product Backlog needs-design-approval labels Nov 12, 2020

gilles-peskine-arm mentioned this pull request Nov 12, 2020

New entry point get_builtin_key for opaque drivers (PSA spec) #3878

Merged

mpg self-requested a review November 16, 2020 09:34

mpg reviewed Nov 16, 2020

View reviewed changes

Documentation clarifications

fd9be02

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the psa-builtin-keys-via-slot-number branch from afc67fc to 062922e Compare November 16, 2020 21:25

mpg requested changes Nov 17, 2020

View reviewed changes

library/psa_crypto_slot_management.c Outdated Show resolved Hide resolved

Internal documentation clarifications

23bda29

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the psa-builtin-keys-via-slot-number branch from 062922e to 23bda29 Compare November 17, 2020 08:54

mpg previously approved these changes Nov 17, 2020

View reviewed changes

ronald-cron-arm requested changes Nov 17, 2020

View reviewed changes

Minor clarifications

7149362

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm dismissed mpg’s stale review via 7149362 November 19, 2020 14:38

mpg approved these changes Nov 20, 2020

View reviewed changes

bensze01 added the Product Backlog label Nov 23, 2020

stevew817 mentioned this pull request Feb 22, 2021

Add implementation for MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS #4160

Merged

3 tasks

daverodgman closed this Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform hook for built-in keys with slot numbers in drivers #3822

Platform hook for built-in keys with slot numbers in drivers #3822

gilles-peskine-arm commented Oct 26, 2020 •

edited

Loading

stevew817 Oct 26, 2020

gilles-peskine-arm Oct 26, 2020

stevew817 Oct 26, 2020

gilles-peskine-arm Oct 26, 2020

stevew817 Oct 26, 2020

gilles-peskine-arm Oct 26, 2020

stevew817 Oct 27, 2020

stevew817 Oct 26, 2020

stevew817 Oct 26, 2020

gilles-peskine-arm Oct 26, 2020

mpg left a comment

mpg left a comment

gilles-peskine-arm commented Nov 17, 2020

mpg left a comment

ronald-cron-arm Nov 17, 2020

gilles-peskine-arm Nov 18, 2020

ronald-cron-arm Nov 18, 2020

gilles-peskine-arm Nov 18, 2020

ronald-cron-arm Nov 18, 2020

gilles-peskine-arm Nov 18, 2020

ronald-cron-arm Nov 18, 2020

gilles-peskine-arm Nov 18, 2020

gilles-peskine-arm Nov 18, 2020

ronald-cron-arm Nov 24, 2020

daverodgman commented Feb 22, 2021

Platform hook for built-in keys with slot numbers in drivers #3822

Platform hook for built-in keys with slot numbers in drivers #3822

Conversation

gilles-peskine-arm commented Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 17, 2020

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daverodgman commented Feb 22, 2021

gilles-peskine-arm commented Oct 26, 2020 •

edited

Loading