Add vendor key support #353

michaelthomasj · 2020-01-24T03:47:20Z

No description provided.

Added macro to check AES key

Made vendor support more generic

Updated code to use lifetime value for vendor instead of vendor keytype

gilles-peskine-arm

Thank you for your contribution! I left some individual comments about specific issues.

Testing and documentation

Please note that any new feature should have adequate documentation and test coverage. In particular, any new compile-time option should be documented in include/mbedtls/config.h and tested in tests/scripts/all.sh. Once we (you and the Mbed Crypto team) have converged a bit more on architecture, we (the Mbed Crypto team) can provide more specific guidance.

For driver support code, testing would mean at least doing a build with the driver hooks enabled, and having unit tests with a mock driver or a test driver that wraps around the software implementation. We don't necessarily need full test coverage from the beginning, but at a minimum we need smoke tests to ensure that the new code builds and doesn't do anything that tools like ASan would flag.

Architecture

In general, this is not very well aligned with our medium-term architectural goals. We distinguish between two kinds of hooks for external hardware:

Accelerators work on plaintext keys. Mbed Crypto dispatches an operation to the accelerator driver if the driver implements that operation. Dispatch is based on a per-algorithm basis and is configured at compile time. psa/crypto_accel_driver.h declares types of functions that accelerator drivers can implement, and the documentation of each type explains the function names that the driver may define. We have not yet started to add support for PSA-style accelerators in Mbed Crypto. Mbed Crypto currently supports accelerators through the MBEDTLS_xxx_ALT interfaces.
Secure elements work on opaque keys. Mbed Crypto only stores a handle to the key, or a wrapped form of the key. psa/crypto_se_driver.h defines the type of structures containing function pointers that secure element drivers implement. Mbed Crypto currently contains proof-of-concept support for some secure element features (only with keys accessed through handles, not yet with keys where Mbed Crypto stores a wrapped form of the key).

We welcome contributions to improve support for cryptographic hardware in Mbed Crypto, but we strongly prefer to add features that are on the path towards our medium-term architectural goal, or at least that have a similar enough interface to facilitate later evolution.

Next steps

Since you're effectively contributing support for features that we intended to have, but earlier than we were planning to provide it, we should have some further private discussions about the architecture.

I think the plaintext-key case and the wrapped-key case should be treated separately. Even if there are many platforms where it's the same hardware under the hood, the key manipulations and the hook points in Mbed Crypto are rather different.

gilles-peskine-arm · 2020-01-27T13:34:31Z

include/psa/crypto_values.h

 #define PSA_KEY_TYPE_AES                        ((psa_key_type_t)0x40000001)

+/** Whether a key type is AES. */
+#define PSA_KEY_TYPE_IS_AES(type) (((type)&PSA_KEY_TYPE_AES) != 0)


PSA_KEY_TYPE_AES is a value, not a flag. The correct expansion of this macro would be (type) == PSA_KEY_TYPE_AES. The API generally doesn't define boolean macros with such a simple expansion.

gilles-peskine-arm · 2020-01-27T13:44:47Z

include/mbedtls/aes.h

                                     <li>Simplifying key expansion in the 256-bit
                                         case by generating an extra round key.
                                         </li></ul> */
+    void *vendor_ctx;           /*!< Vendor defined context. */


The official way to customize the AES context for an accelerator is to define MBEDTLS_AES_ALT and provide a custom version of the type mbedtls_aes_context as well as all the functions in aes.c (except selftests).

It's also possible to substitute the core AES mechanisms (key expansion, single-block encryption, single-block decryption) by defining MBEDTLS_AES_SETKEY_ENC_ALT, MBEDTLS_AES_SETKEY_DEC_ALT, MBEDTLS_AES_ENCRYPT_ALT and MBEDTLS_AES_DECRYPT_ALT and providing definitions for the corresponding functions (mbedtls_aes_setkey_enc, mbedtls_aes_setkey_dec, mbedtls_internal_aes_encrypt, mbedtls_internal_aes_decrypt). In this case, you have to use the default definition of mbedtls_aes_context. If you override all four functions, you can choose what you put in the rk field. So I believe that vendor_ctx is not necessary.

We would prefer not to change the layout of mbedtls_aes_context at this point.

gilles-peskine-arm · 2020-01-27T13:47:46Z

include/psa/crypto_extra.h

+ * \note    This function has to be defined by the vendor if MBEDTLS_PSA_CRYPTO_ACCEL_DRV_C 
+ *          is defined.
+ *          A weakly linked version is provided by default and returns
+ *          PSA_ERROR_NOT_SUPPORTED. Do not use this function directly;


Since this function is not meant to be called by applications, it should not be declared in crypto_extra.h. The correct location would be crypto_accel_driver.h. That file is still in an early draft stage and not ready for production yet. I propose that we work together on getting at least part of it production-ready.

This applies to the other functions added here.

gilles-peskine-arm · 2020-01-27T13:50:56Z

include/psa/crypto_struct.h

 {
    attributes->core.lifetime = lifetime;
-    if( lifetime == PSA_KEY_LIFETIME_VOLATILE )
+    if (PSA_KEY_LIFETIME_IS_VOLATILE(lifetime))


For Mbed Crypto specific files (as opposed to standard PSA headers), please use the Mbed TLS coding style, with its unusual whitespace placement: spaces inside parentheses, not outside.

if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) )

gilles-peskine-arm · 2020-01-27T13:52:52Z

include/psa/crypto_values.h

 #define PSA_KEY_LIFETIME_PERSISTENT             ((psa_key_lifetime_t)0x00000001)

+#define PSA_KEY_LIFETIME_IS_VOLATILE(lifetime) \
+    (((lifetime)&PSA_KEY_LIFETIME_VOLATILE) != 0)


PSA_KEY_LIFETIME_VOLATILE is a value, not a flag. There are currently two defined lifetime values: VOLATILE = 0 and PERSISTENT = 1. We can make 0x00000001 a PSA_KEY_LIFETIME_PERSISTENT_FLAG and thus

#define PSA_KEY_LIFETIME_PERSISTENT_FLAG ((psa_key_lifetime_t)0x00000001) #define PSA_KEY_LIFETIME_IS_VOLATILE(lifetime) (((lifetime) & PSA_KEY_LIFETIME_PERSISTENT_FLAG) == 0)

gilles-peskine-arm · 2020-01-27T13:54:25Z

include/psa/crypto_values.h

+#define PSA_KEY_LIFETIME_VOLATILE_VENDOR (PSA_KEY_LIFETIME_VENDOR_FLAG | PSA_KEY_LIFETIME_VOLATILE)
+
+#define PSA_KEY_LIFETIME_IS_VENDOR_DEFINED(lifetime) \
+    (((lifetime)&PSA_KEY_LIFETIME_VENDOR_FLAG) != 0)


Style: please put whitespace around binary operators including & ((t)&x would be the address of x cast to the type t).

This one will also change to match your proposal.

gilles-peskine-arm · 2020-01-27T13:57:20Z

library/psa_crypto.c

    else
 #endif /* MBEDTLS_PSA_CRYPTO_SE_C */
+#if defined (MBEDTLS_PSA_CRYPTO_ACCEL_DRV_C)
+    if (PSA_KEY_LIFETIME_IS_VENDOR_DEFINED(slot->attr.lifetime))


If an accelerated implementation is available, it should be called for any transparent key regardless of its lifetime. Custom lifetimes may indicate keys in vendor-specific storage, or (to be clarified) keys with vendor-specific wrapping.

gilles-peskine-arm · 2020-02-05T18:37:09Z

include/psa/crypto_values.h

 */
 #define PSA_KEY_LIFETIME_PERSISTENT             ((psa_key_lifetime_t)0x00000001)

+#define PSA_KEY_LIFETIME_IS_VOLATILE(lifetime) \


Please refer to the definition of PSA_KEY_LIFETIME_IS_VOLATILE and to the new structure for lifetime values in #358, which is slated to be included in the next release of Mbed Crypto and in an upcoming version of the PSA Cryptography specification.

I am fine with that new definition. Im not quite sure how to get it in here though.

moved accel definitions from other header files to accel header file

This reverts commit a9ba5bb.

michaelthomasj added 21 commits January 8, 2020 22:28

Initial updates to support vendor defined keys

4533607

Updated aes gen signature

01d88e9

Updated argument

c822cf9

Added vendor format key to aes context

3e88b5c

Renamed to keep naming consistent

6ef5072

Added macro to check AES key

Context setup for vendor defeind AES key

2d73c20

Wrapped key working now.

17547bc

Updates to make the AES Wrapped support generic

7de9ac5

Added ECC vendor macros

1d573ec

Made vendor support more generic

Added ECDSA sign and verify weak functions

ff0e8b9

removed unused weak functions

153cd2f

Updated code to use lifetime value for vendor instead of vendor keytype

Added macros to check lifetime

aa259cf

undid uncrustify changes

bbaaced

removed uncrustify changes

6b632ef

More uncrustify undo

cc0d0a2

Removed old vendor keytype usage

ecb7284

Undo uncrustify

0b57513

uncrustify undo

06705f9

Uncrustiry fixes

f4776ad

whitespace fix

a8ac016

Added new macro to enable crypto accelerator

4c53fc7

michaelthomasj requested review from Patater and gilles-peskine-arm January 24, 2020 03:47

gilles-peskine-arm requested changes Jan 28, 2020

View reviewed changes

michaelthomasj added 2 commits January 29, 2020 16:54

Removed aes vendor ctx field

4d6f518

Prototype update

e2e724a

gilles-peskine-arm reviewed Feb 5, 2020

View reviewed changes

michaelthomasj added 3 commits February 10, 2020 19:41

made some static functions externally visible

863b5d6

moved accel definitions from other header files to accel header file

made fns non-staic

7ab5174

Undid changes

46b8e0e

michaelthomasj added 10 commits February 17, 2020 18:08

typecast to fix warning

d10cd66

made rsa import fn non-static

1ed98bd

made ecdsa sign and verify fns non-static

257d3be

Removed unnecessary fns

420afc4

Made exit uniform

efcf4cf

Changed volatile check

58de784

Changed volatile check

bb039ba

Updated persistent check

6a632df

MR update

40ae76c

Updated Volatile check macro

82fd27c

gilles-peskine-arm changed the base branch from development to renesas-RA March 3, 2020 12:15

danh-arm force-pushed the renesas-RA branch from 1146b4e to 92348d1 Compare March 3, 2020 12:52

michaelthomasj added 3 commits March 3, 2020 13:01

Whitespace fix

a9ba5bb

Revert "Whitespace fix"

3f0fc77

This reverts commit a9ba5bb.

MR Whitespace fix

5a3f32b

danh-arm merged commit 327730c into ARMmbed:renesas-RA Mar 3, 2020

Add vendor key support #353

Add vendor key support #353

Uh oh!

Conversation

michaelthomasj commented Jan 24, 2020

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Testing and documentation

Architecture

Next steps

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!

gilles-peskine-arm Feb 5, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gilles-peskine-arm Feb 5, 2020 •

edited

Loading