Rework PSA Crypto core to store keys in export representation #3492

stevew817 · 2020-07-13T08:45:20Z

Description

This PR changes how the PSA Crypto core stores keys internally. It does not affect API behaviour, as demonstrated by no changes to test suites.

With these changes, PSA Crypto core keeps keys in 'export' representation (i.e. the format as defined by psa_export_key()), instead of mbedTLS 'internal' representation. This has multiple benefits over the pre-existing behaviour:

No conversion step on load/store to persistent key database
No conversion step when doing operations through the accelerator interface defined in PSA unified driver prototype #3313 and PSA unified driver specification #3493
Keeps API consistent: when both incoming and outgoing APIs define keys in 'export' format, it doesn't make a lot of sense to have the intermediary store them in a different format.
Provides separation of concerns: makes it easier to call different crypto providers (hardware/software) for the same key type, and paves the way for splitting up the current mbed TLS software crypto in key-validation and key-usage components.

Immediate drawbacks:

A just-in-time conversion step is currently taken on each key usage call, when the operation is backed by the mbed TLS software algorithms. This can be mitigated by adding a caching mechanism, but after discussions with @gilles-peskine-arm it was decided against implementing such a mechanism from the get-go.
A key conversion to and from mbed TLS 'internal' representation is still taken on key import, due to missing the aforementioned split between key validation and key usage in the mbed TLS libraries. This is future improvement work.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

Tests
Documentation

Steps to test or reproduce

Tested by test suite

gilles-peskine-arm

I've done a first pass of architectural review, not a thorough code review.

gilles-peskine-arm · 2020-07-13T16:56:16Z

library/psa_crypto_core.h

 /** The data structure representing a key slot, containing key material
 * and metadata for one key.
 */
 typedef struct
 {
    psa_core_key_attributes_t attr;
-    union
+    struct


No, this should be a union: if the key is represented by a slot number, that's all the data there is.

Or alternatively always have data and bytes, and allocate slot numbers dynamically. But even in the interest of simplicity, I'd rather not change to allocating 8 bytes dynamically.

The change from union to struct is a leftover from implementing the caching logic that was stripped out. I'll change back.

gilles-peskine-arm · 2020-07-13T16:57:40Z

library/psa_crypto.c

 {
    /* Check that the bit size is acceptable for the key type */
    switch( type )
    {
        case PSA_KEY_TYPE_RAW_DATA:
-#if defined(MBEDTLS_MD_C)


No, the compilation guards should stay, to avoid compiling code about algorithms or key types that are not included in the build.

Conceptually:
a: I agree that code about algorithms or key types that are not supported downstream should be removed, but
b: how is this going to work for the accelerator case, which this PR is building towards?

The MBEDTLS_XXX_C defines are used to compile in/out support for a certain cryptographic algorithm, and will eat code space. For the accelerator case, you want input validation to happen in PSA Core, but the option to not have a software algorithm to back it, instead delegating to an accelerator. That means either validation and operations need to be split in two compilation units (with each their _C define?), or validation needs to happen inside PSA Core code.

For symmetric ciphers, validation is so very basic that I don't see why we'd require compiling in software support for the entire algorithm just to validate input. What if the core is being used for key storage at-rest only (e.g. in a TF-M model)? Are you going to require compiling in a full software ChaCha20 implementation just because the app storing the key is indicating it's a ChaCha20 key when storing it for later retrieval?

MBEDTLS_xxx_C indicates that the feature is available to the application, regardless of how it's implemented (built-in code, MBEDTLS_xxx_ALT, or PSA driver).

Though currently, the way you turn on/off features is by defining/undefining MBEDTLS_xxx_C macros, which means they're used to compile in software support as much as to indicate capabilities. You can't have MBEDTLS_XXX_ALT without also having MBEDTLS_XXX_C.

How do you see this getting resolved? If MBEDTLS_XXX_C takes on the meaning of "support available", then it should be defined through a combination of driver capability and software capability, and the software capability should be compiled in based on a different macro (to-be-introduced).
If MBEDTLS_XXX_C takes on the meaning of "software implementation compiled in" (which I think is the most common interpretation amongst users, given the prefix), then we should probably start defining PSA_XXX_SUPPORTED based on a combination of MBEDTLS_XXX_C macros and to-be-defined driver-provided macros.

Currently, if you turn on MBEDTLS_xxx_C and MBEDTLS_xxx_ALT, the built-in implementation is not included in the build. Turning on MBEDTLS_xxx_C turns on the feature, and another option controls what implements the feature. It's important to preserve this property, both for backward compatibility and because it provides some independence between the application configuration (what cryptographic mechanisms do I need?) and the platform configuration (what cryptographic hardware is available?).

We need to gate the software implementation with some kind of !MBEDTLS_PSA_xxx_DRIVER in addition to !MBEDTLS_xxx_ALT. Names and granularity to be defined.

Reverted through history rewrite.

library/psa_crypto.c

stevew817 · 2020-07-20T13:48:34Z

@gilles-peskine-arm merge conflict resolved.

stevew817 · 2020-07-20T15:53:00Z

This dance again... It fails ARM internal CI but passes all public checks. Please advise.

gilles-peskine-arm · 2020-07-20T16:36:18Z

Visual Studio is unhappy:

         C:\builds\ws\3492-merge-MOGE37PWQMQRGNEROW56T\worktrees\tmp81j2z3aw\library\psa_crypto.c(842): warning C4267: '=' : conversion from 'size_t' to 'psa_key_bits_t', possible loss of data [C:\builds\ws\3492-merge-MOGE37PWQMQRGNEROW56T\worktrees\tmp81j2z3aw\cmake_solution\library\mbedcrypto.vcxproj]
         C:\builds\ws\3492-merge-MOGE37PWQMQRGNEROW56T\worktrees\tmp81j2z3aw\library\psa_crypto.c(844): warning C4267: '=' : conversion from 'size_t' to 'psa_key_bits_t', possible loss of data [C:\builds\ws\3492-merge-MOGE37PWQMQRGNEROW56T\worktrees\tmp81j2z3aw\cmake_solution\library\mbedcrypto.vcxproj]
         C:\builds\ws\3492-merge-MOGE37PWQMQRGNEROW56T\worktrees\tmp81j2z3aw\library\psa_crypto.c(5822): warning C4267: '=' : conversion from 'size_t' to 'psa_key_bits_t', possible loss of data [C:\builds\ws\3492-merge-MOGE37PWQMQRGNEROW56T\worktrees\tmp81j2z3aw\cmake_solution\library\mbedcrypto.vcxproj]

Full log:
3492-merge-7-windows.log.gz

The Windows build succeeded on Travis. I think that's because Travis runs a Windows build in the default config, but Jenkins runs a build with config.pl full. Or maybe it's because Travis runs VS 2017 but Jenkins runs VS 2013. Both are 64-bit builds, and the 32-bit Windows build on Jenkins passes.

ronald-cron-arm

I started to review but the history is quite hard to follow.

Some functions that are significantly changed in one commit and again significantly changed in a following commit (like psa_is_key_supported() in "Always store the raw input key data in psa_key_slot_t" and then in "For the time being, always convert to internal representation on import"). In case you just move code, please dedicate a commit to that with an explicit commit message.

An union is changed to a struct and changed back to an union later on.

Otherwise, in PRs there is I think a common agreement that we don't merge back the development branch in case of conflicts but rather rebase the PR on top of the development branch.

Thus all together, could you please rebase the PR on top of development and rewrite the history of the commits to avoid back and forth changes.

library/psa_crypto_core.h

library/psa_crypto.c

stevew817 · 2020-07-24T21:40:42Z

@ronald-cron-arm and @gilles-peskine-arm, history is now rewritten and the non-relevant change (adding knowledge about DH keys) backed out. Please re-review.

In preparation for the implementation of the accelerator APIs. This is ramping up to the goal of only storing the export representation in the key slot, and not keeping the crypto implementation-specific representations around. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Now that both ECP and RSA keys are represented in export representation, they can be treated more uniformly. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

ronald-cron-arm

Thanks for the rebase, it was much easier to review. I've reviewed up to the commit removing the RSA internal representation. I started to look at the ECP one and saw some things similar as in the RSA commit and I prefer to clarify things on RSA before to go further.

library/psa_crypto.c

* Updated wording * Split out buffer allocation to a convenience function * Moved variable declarations to beginning of their code block Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 · 2020-08-03T12:47:27Z

@ronald-cron-arm I applied your feedback

ronald-cron-arm

Thanks again for all the changes. I've reviewed all the conversations I've opened for this PR and there are only two not resolved (apart from the one I've created as part of the present review): https://github.com/ARMmbed/mbedtls/pull/3492/files#r461690619 and https://github.com/ARMmbed/mbedtls/pull/3492/files#r460697161. Gilles has been involved in the two of them thus I guess better to wait for Gilles's review before to address them.

library/psa_crypto.c

gilles-peskine-arm

There are two blockers, both easy to fix (uninitialized variable and leak of partially-written private key). I'll approve if these are fixed. Fixing the style issues would be nice but given that we're pressed for time we can do that in a follow-up. Fixing the secp224k1 issue is out of scope.

library/psa_crypto.c

gilles-peskine-arm · 2020-08-04T11:51:41Z

library/psa_crypto.c

         * - The byte 0x04;
         * - `x_P` as a `ceiling(m/8)`-byte string, big-endian;
         * - `y_P` as a `ceiling(m/8)`-byte string, big-endian.
         * So its data length is 2m+1 where n is the key size in bits.
         */
        if( ( data_length & 1 ) == 0 )
            return( PSA_ERROR_INVALID_ARGUMENT );
-        data_length = data_length / 2;
+        curve_size = data_length / 2;


Annoyingly this isn't true for secp224k1 which has a 225-bit private key but 224-bit public key coordinates. This may be a preexisting issue and it's certainly a preexisting test gap (I only realized the issue after writing the code you're modifying). Furthermore this curve is used very little in practice (I think it's only there to have a size that matches SHA-224, which in turn only exists to have a theoretical strength that matches 3DES). So I'm happy to defer it to a follow-up.

Filed as #3541

Sounds like that's as much of a preexisting issue as the Curvexxx public key handling was...

gilles-peskine-arm · 2020-08-04T15:59:06Z

library/psa_crypto.c

@@ -961,27 +1215,15 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot )
    {
        /* No key material to clean. */
    }
-    else if( key_type_is_raw_bytes( slot->attr.type ) )
+    else if( key_type_is_raw_bytes( slot->attr.type ) ||


Now that all keys are represented by a malloc'ed buffer, the data field of the union is always valid if psa_key_slot_is_external is false, so the “shoulnd't happen” case is no longer needed.

gilles-peskine-arm · 2020-08-04T18:16:16Z

library/psa_crypto.c

    }

+    /* Allocate and initialize a key representation. */
+    ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair));


Style (here and elsewhere):

Suggested change

ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair));

ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) );

Will fix, though it'd be nice if someone could point me to the actual style guide for mbedTLS, and maybe even a delinter?

For that kind of things it is there: https://tls.mbed.org/kb/development/mbedtls-coding-standards.

The last time we looked for a formatting tool, we couldn't find one that supported Mbed TLS's peculiar coding rules, in particular spaces inside parentheses for function calls but not for defined. But that was a couple of years ago and clang-format has improved since then. We haven't found the time to check again.

I think the majority of the team doesn't like the current whitespace style, but that doesn't mean we'd agree on a replacement, and it would be disruptive (it would have to be applied to every PR in flight and other working branch), so for now we aren't touching it.

Thanks. Still, would be nice to have it automatic. mbed TLS' style is different enough from the other styles I have to deal with, that it's non-trivial for me to always remember which style I'm supposed to write in.

gilles-peskine-arm · 2020-08-04T18:16:31Z

library/psa_crypto.c

+    /* Allocate and initialize a key representation. */
+    ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair));
+    if( ecp == NULL )
+        return PSA_ERROR_INSUFFICIENT_MEMORY;


Style (here and elsewhere):

Suggested change

return PSA_ERROR_INSUFFICIENT_MEMORY;

return( PSA_ERROR_INSUFFICIENT_MEMORY );

Will fix, though again, would be nice to have a delinter.

gilles-peskine-arm · 2020-08-04T18:32:10Z

library/psa_crypto.c

@@ -5264,16 +5602,17 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key,
                                            size_t shared_secret_size,
                                            size_t *shared_secret_length )
 {
-    mbedtls_ecp_keypair *their_key = NULL;
+    mbedtls_ecp_keypair *their_key;


Blocker: their_key must be initialized to NULL because psa_load_ecp_representation won't set it if it fails. This can be fixed either by making psa_load_xxx_representation set *p_xxx unconditionally (which has my preference) or by adding the initialization here (the only place where it's missing).

Please add a non-regression test. The test case “PSA key agreement setup: ECDH + HKDF-SHA-256: public key on different curve” should catch this, but doesn't, due to a preexisting oddity in psa_key_agreement_ecdh: the public key in that test is a valid secp384r1 public key and psa_load_ecp_representation happily loads this, and the discrepancy with the private key is only caught in the second call to mbedtls_ecdh_get_params. A similar test case where the public key isn't valid at all (e.g. empty string) does make psa_load_ecp_representation fail and then psa_key_agreement_ecdh dereferences an uninitialized pointer variable.

library/psa_crypto.c

* return is treated as a function call * space between opening and closing parentheses * remove whiteline between assignment and checking of same variable Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 · 2020-08-05T14:45:25Z

@gilles-peskine-arm and @ronald-cron-arm, latest fixes pushed.

library/psa_crypto.c

gilles-peskine-arm

LGTM apart from #3492 (comment)

And zeroize key buffer before freeing to avoid keys hanging around on the heap. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Since it is being dereferenced by free on exit it should be inited to NULL. Also added a small test that would trigger the issue. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

ronald-cron-arm

Nothing left on my side, LGTM.

gilles-peskine-arm · 2020-08-05T15:51:59Z

Thanks, looks good to me! I'll just wait for CI results and then I should be able to merge.

gilles-peskine-arm · 2020-08-05T19:16:04Z

The non-MbedOS part of the CI has passed on Jenkins. Travis failed due to a known issue with GnuTLS. Merging.

stevew817 · 2020-08-05T20:27:44Z

🥳

* Updated wording * Split out buffer allocation to a convenience function * Moved variable declarations to beginning of their code block Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

* Allocate internal representation contexts on the heap (i.e. don't change where they're being allocated) * Unify load_xxx_representation in terms of allocation and init behaviour Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

gilles-peskine-arm requested changes Jul 13, 2020

View reviewed changes

gilles-peskine-arm self-assigned this Jul 13, 2020

gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces needs-work enhancement labels Jul 13, 2020

gilles-peskine-arm added this to the July 2020 Sprint milestone Jul 13, 2020

ronald-cron-arm self-requested a review July 20, 2020 13:41

gilles-peskine-arm self-requested a review July 20, 2020 13:56

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jul 20, 2020

stevew817 mentioned this pull request Jul 20, 2020

Add partial implementation of accelerator API defined in #3493 #3501

Merged

3 tasks

ronald-cron-arm requested changes Jul 24, 2020

View reviewed changes

library/psa_crypto_core.h Outdated Show resolved Hide resolved

library/psa_crypto_core.h Outdated Show resolved Hide resolved

library/psa_crypto.c Outdated Show resolved Hide resolved

stevew817 force-pushed the rework/key_slot_contains_key_buffer branch from ec27292 to 0b3f4b0 Compare July 24, 2020 21:35

stevew817 added 5 commits July 24, 2020 23:44

Pull apart slot memory allocation from key validation.

81be2fa

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Remove RSA internal representation from key slot

a01795d

Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Remove ECP internal representation from key slot

acda834

Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Unify key handling logic

560c28a

Now that both ECP and RSA keys are represented in export representation, they can be treated more uniformly. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 force-pushed the rework/key_slot_contains_key_buffer branch from 0b3f4b0 to c92b0e2 Compare July 24, 2020 21:44

Disconnect knowing about a PSA key type from knowing the mbedTLS API

19fd574

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 force-pushed the rework/key_slot_contains_key_buffer branch from c92b0e2 to 19fd574 Compare July 24, 2020 21:46

stevew817 requested a review from ronald-cron-arm July 24, 2020 21:50

ronald-cron-arm reviewed Jul 27, 2020

View reviewed changes

Update after feedback on Mbed-TLS#3492

75b7436

* Updated wording * Split out buffer allocation to a convenience function * Moved variable declarations to beginning of their code block Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

ronald-cron-arm reviewed Aug 3, 2020

View reviewed changes

library/psa_crypto.c Show resolved Hide resolved

danh-arm modified the milestones: July 2020 Sprint, August 2020 Sprint Aug 3, 2020

gilles-peskine-arm requested changes Aug 4, 2020

View reviewed changes

Style fixes

2914986

* return is treated as a function call * space between opening and closing parentheses * remove whiteline between assignment and checking of same variable Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

gilles-peskine-arm reviewed Aug 5, 2020

View reviewed changes

library/psa_crypto.c Show resolved Hide resolved

gilles-peskine-arm requested changes Aug 5, 2020

View reviewed changes

stevew817 added 3 commits August 5, 2020 17:38

Simplified key slot deletion

fd4d69a

And zeroize key buffer before freeing to avoid keys hanging around on the heap. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Add buffer zeroization when ecp_write_key fails

b7f6dea

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Initialize key pointer in ecdh to NULL

d486787

Since it is being dereferenced by free on exit it should be inited to NULL. Also added a small test that would trigger the issue. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 force-pushed the rework/key_slot_contains_key_buffer branch from 870b701 to d486787 Compare August 5, 2020 15:38

ronald-cron-arm approved these changes Aug 5, 2020

View reviewed changes

gilles-peskine-arm approved these changes Aug 5, 2020

View reviewed changes

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Aug 5, 2020

gilles-peskine-arm merged commit 67a85d1 into Mbed-TLS:development Aug 5, 2020

This was referenced Aug 25, 2020

PSA: support arbitrary data storage from secure element drivers #3289

Closed

Add a validate_key entry point to the PSA driver interface #3617

Closed

	ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair));
	ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) );

	return PSA_ERROR_INSUFFICIENT_MEMORY;
	return( PSA_ERROR_INSUFFICIENT_MEMORY );

Rework PSA Crypto core to store keys in export representation #3492

Rework PSA Crypto core to store keys in export representation #3492

Conversation

stevew817 commented Jul 13, 2020 • edited Loading

Description

Status

Requires Backporting

Migrations

Todos

Steps to test or reproduce

gilles-peskine-arm 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

stevew817 commented Jul 20, 2020

stevew817 commented Jul 20, 2020

gilles-peskine-arm commented Jul 20, 2020

ronald-cron-arm left a comment • edited Loading

Choose a reason for hiding this comment

stevew817 commented Jul 24, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

stevew817 commented Aug 3, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm 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

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

stevew817 commented Aug 5, 2020

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 5, 2020

gilles-peskine-arm commented Aug 5, 2020

stevew817 commented Aug 5, 2020

stevew817 commented Jul 13, 2020 •

edited

Loading

ronald-cron-arm left a comment •

edited

Loading