Skip to content

Prepare support for 64 bit key ids under SPM #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 20, 2019

Conversation

itayzafrir
Copy link
Contributor

@itayzafrir itayzafrir commented Jan 29, 2019

Preparation for type separation between SPE and NSPE.

This PR is the setup required for moving from 32 bit key ids to 64 bit key ids in Mbed OS in a system where SPM is present. When compiling as a library (in a system where there's no SPM) the changes in this PR are irrelevant and should not affect anything.

The file crypto_types.h will be handled differently in Mbed OS when SPM is present. There will be 2 different implementation of the file, one for the client and one for the server. Much like crypto_structs.h is separated between client and server. Specifically, the type psa_key_id_t will be 32 bits on the client and 64 bits on the server. This work is being done as part of ARMmbed/mbed-os#9575.

@itayzafrir itayzafrir assigned Patater and unassigned Patater Jan 29, 2019
@itayzafrir itayzafrir requested a review from Patater January 29, 2019 12:33
@itayzafrir itayzafrir added needs: work The pull request needs rework before it can be merged. DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. labels Jan 30, 2019
@itayzafrir
Copy link
Contributor Author

itayzafrir commented Jan 31, 2019

@Patater, @gilles-peskine-arm
ITS unique identifiers are being changed to 64 bits (as of ARMmbed/mbed-os#9529). To support this, in fe7acb4 I'v changed the value of PSA_MAX_PERSISTENT_KEY_IDENTIFIER from 0xffff0000 to 0xffff0000ffffffff - for ITS, the upper 32 bits represent the id specified by the user, the lower 32 bits represent the calling partition id (which may be -1 when a call is from NSPE).

Should the following also be updated?

#define PSA_CRYPTO_ITS_RANDOM_SEED_UID 0xFFFFFF52

If so, is there anyone external to crypto who is dependent on this value? I'v searched through Mbed OS and haven't seen anyone using this value directly outside of the crypto context. Can there be any backwards compatibility issues with changing this value?

@itayzafrir itayzafrir force-pushed the ipc-support-64-bit-key-ids branch from 3a02e53 to fe7acb4 Compare January 31, 2019 11:38
@itayzafrir itayzafrir added question Further information is requested and removed needs: work The pull request needs rework before it can be merged. labels Jan 31, 2019
@Patater
Copy link
Contributor

Patater commented Jan 31, 2019

What's the logic behind changing PSA_MAX_PERSISTENT_KEY_IDENTIFIER to 0xffff0000ffffffff? Could you put this in the commit message place?

The default random seed implementation uses the PSA_CRYPTO_ITS_RANDOM_SEED_UID define, but doesn't care about the value. The issue is with the upgrade path from a system that has an already injected random seed and we update the UID; the default random seed implementation would need to look in two places: the old and new location. A migration tool could be made for these devices to move anything present at the old UID to the new one, but there may not be a need yet.

@shelib01 Do we have any devices in the field using this entropy injection feature? Do we need to keep using the old UID to support those devices? If we haven't deployed anything yet, would you agree its okay to change the UID not before we get devices out in the field?

@Patater
Copy link
Contributor

Patater commented Jan 31, 2019

Could you also have a look at the compilation failures? It superficially looks like there are some places that mishandle 64-bit numbers, causing compilation failure.

@itayzafrir itayzafrir force-pushed the ipc-support-64-bit-key-ids branch 4 times, most recently from 237f8b7 to 919b89e Compare February 3, 2019 10:42
@itayzafrir
Copy link
Contributor Author

itayzafrir commented Feb 3, 2019

@Patater I've amended the commit and added the reasoning of changing PSA_MAX_PERSISTENT_KEY_IDENTIFIER to 0xffff0000ffffffff inside the commit message (919b89e). Old branch can be found @ https://github.com/itayzafrir/mbed-crypto/tree/ipc-support-64-bit-key-ids-1.

About changing the value of PSA_CRYPTO_ITS_RANDOM_SEED_UID, I don't think it's necessary. Since PSA_CRYPTO_ITS_RANDOM_SEED_UID is defined as 0xFFFFFF52, and the original value of PSA_MAX_PERSISTENT_KEY_IDENTIFIER was set to 0xFFFF0000, it wouldn't have passed the validation check at https://github.com/itayzafrir/mbed-crypto/blob/919b89ec656c0693631547cf635774b8cd1ac0fc/library/psa_crypto_slot_management.c#L218 anyways. A quick check revealed that the PSA_CRYPTO_ITS_RANDOM_SEED_UID doesn't go through the regular flow and is never tested against PSA_MAX_PERSISTENT_KEY_IDENTIFIER in any case. Therefore IMO we can leave it as is, unless I'm missing something?...

@itayzafrir
Copy link
Contributor Author

Build failed on test_suite_timing - known issue not related to the changes in this PR.

@itayzafrir itayzafrir added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. question Further information is requested labels Feb 4, 2019
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff0000
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change PSA_MAX_PERSISTENT_KEY_IDENTIFIER based on the presence of ITS or could we always use 0xffff0000ffffffff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only change it for ITS. If we change it always, the following check will fail when compiling in a non-SPM system:
https://github.com/itayzafrir/mbed-crypto/blob/919b89ec656c0693631547cf635774b8cd1ac0fc/library/psa_crypto_slot_management.c#L205-L219
Actually clang failed this because the condition if( id >= PSA_MAX_PERSISTENT_KEY_IDENTIFIER ) would never be true because id is 32 bits in a non-SPM system. In an SPM supported system, psa_key_id_t is 32 bits on the client and a 64 bit type on the service side.

Copy link
Contributor

@Patater Patater Feb 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to what exactly is failing and what the configuration looks like on NSPE and SPE, in both segmented and non-segmented systems.

In a segmented system:
ITS would be present on SPE, so MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C would be used so we can use it. ITS would also be present on NSPE, but we wouldn't define ITS_C for the Mbed Crypto client code?

In a non-segmented system:
The server and client Mbed Crypto are the same. ITS is available, but we'd not define MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C?

The type of the storage identifier (and whether it is 64-bits or 32-bits) and the max value is probably best handled in a header that can change between client and server implementations. The max value is a property of how big the identifier is. However, at the same time, crypto_sizes.h is perhaps not best since psa_crypto_storage.h is not part of the PSA API. Do we need to do any work in Mbed OS to change which version of psa_crypto_storage.h is used based on server or client? Should we use a different header? Perhaps crypto_types.h to contain the max?

Copy link
Contributor Author

@itayzafrir itayzafrir Feb 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can what be 64 bits? If you're asking about the psa_key_id_t then not really, it would then make ITS to change to 96 bits and we'll be in the same loop again.

The idea for this is that the client code sends a 32 bit key id, the service places these 32 bits in the upper 32 bits of the service side psa_key_id_t (64 bits), and the calling partition id in the lower 32 bits of the server representation of psa_key_id_t. This happens for psa_create_key and also for psa_open_key (in the service side) and this ensures that only the partition which created the key is allowed to open the key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Patater I see you point, I overlooked the fact that ITS is a also used in a non-segmented system.

In a segmented system:
ITS would be present on SPE, so MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C would be used so we can use it. ITS would also be present on NSPE, but we wouldn't define ITS_C for the Mbed Crypto client code?

Not exactly, as the crypto client code is implemented just as a proxy, I think it's meaningless for the crypto client side whether ITS is enabled or not. Same thing goes for PSA_MAX_PERSISTENT_KEY_IDENTIFIER, it's not used by the client code and it isn't part of the PSA Crypto API.

In a non-segmented system:
The server and client Mbed Crypto are the same. ITS is available, but we'd not define MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C?

I guess we can't really do that...


So, since PSA_MAX_PERSISTENT_KEY_IDENTIFIER is not part of the official PSA Crypto API it might be meaningless to other PSA Crypto implementations so I'm not sure if placing it in crypto_types.h makes sense...IMO it would make more sense placing it crypto_extra.h and keeping different copies of crypto_extra.h in Mbed OS, one for client and for server, same as is already being done for crypto_structs.h and is planned (ARMmbed/mbed-os#9575) to be done with crypto_types.h. However, this forces us to maintain yet another file which is different for client/server. I think that a simpler solution is to just change (919b89e):

From:

#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff0000ffffffff
#else
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff0000	
#endif

To:

#if defined(MBEDTLS_PSA_CRYPTO_SPM)
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff0000ffffffff
#else
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff0000	
#endif

MBEDTLS_PSA_CRYPTO_SPM is set only when compiling the server side, see https://github.com/ARMmbed/mbed-os/blob/57d8915a899a59999d7e2a9695e20a8a18cb54bd/targets/targets.json#L7905

Please let me know if this is an acceptable solution and I will update this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it, also changed the PSA_MAX_PERSISTENT_KEY_IDENTIFIER value from 0xffff0000ffffffff to 0xffff000000000000 when compiled for server side. I amended the commit, old branch https://github.com/itayzafrir/mbed-crypto/tree/ipc-support-64-bit-key-ids-2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user calls psa_create_key(PSA_KEY_LIFETIME_PERSISTENT, 0xFFFFFF52, ...), how do we know they won't ever get a persistent allocation with PSA_CRYPTO_ITS_RANDOM_SEED_UID as the storage UID for their key? 0xFFFFFF52 is certainly less than PSA_MAX_PERSISTENT_KEY_IDENTIFIER. Don't we need to explicitly block this UID when we select the storage ID? Previously, we depended on the PSA_MAX_PERSISTENT_KEY_IDENTIFIER checks to block allocation of this storage location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for an explicit block on PSA_CRYPTO_ITS_RANDOM_SEED_UID.

In a system where SPM is present and as of ARMmbed/mbed-os#9575, if a user were to call psa_open_key(PSA_KEY_LIFETIME_PERSISTENT, PSA_CRYPTO_ITS_RANDOM_SEED_UID, &handle) or psa_create_key(PSA_KEY_LIFETIME_PERSISTENT, PSA_CRYPTO_ITS_RANDOM_SEED_UID, &handle) the ID (psa_key_id_t) sent from the client (PSA_CRYPTO_ITS_RANDOM_SEED_UID in this case or any other ID for that matter) is translated by the server (crypto partition) into a different ID prior to calling the actual Mbed Crypto library.

The ID (psa_key_id_t) on the server side is 64 bits and it translated from what the client sent (32 bits) in the following way:
Place the 32 bit ID received from the client in the upper 32 bits of the 64 bit psa_key_id_t, and the calling partition id (32 bits) in the lower 32 bits of the 64 bit psa_key_id_t.

32 bits PSA_CRYPTO_ITS_RANDOM_SEED_UID 32 bits CALLING PARTITION ID

The table shows what the actual ID sent to ITS will looks like.

Example: If a client were to call psa_open_key or psa_create_key with PSA_CRYPTO_ITS_RANDOM_SEED_UID as the ID, the actual ID value sent to ITS will be:

  1. If calling from NSPE (partition id -1) - ID sent to ITS = 0xffffff52ffffffff
  2. If calling from different partition (for example partition id 3) - ID sent to ITS = 0xffffff5200000003

So there's no chance of overwriting the injected PSA_CRYPTO_ITS_RANDOM_SEED_UID.
Moreover, the results from the examples above would not pass the check against PSA_MAX_PERSISTENT_KEY_IDENTIFIER as both 0xffffff52ffffffff and 0xffffff5200000003 are greater than PSA_MAX_PERSISTENT_KEY_IDENTIFIER.

All this magic is planned to happen in ARMmbed/mbed-os#9575, this PR is about the required setup for 9575. You might want to take a look at it to see the complete picture.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand for client/server architectures that this would work. Thanks for your explanation.

How about for K64F or other non-segmented systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-segmented systems this has no effect and everything remains as it is now. MBEDTLS_PSA_CRYPTO_SPM is currently only defined for the server side of psoc6 (FUTURE_SEQUANA_M0_PSA). See https://github.com/ARMmbed/mbed-os/blob/57d8915a899a59999d7e2a9695e20a8a18cb54bd/targets/targets.json#L7905

alekshex
alekshex previously approved these changes Feb 5, 2019
@itayzafrir itayzafrir added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Feb 6, 2019
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to preserve backward compatibility with systems that have an injected entropy file in ITS. We need either a clear explanation in the code of how this is ensured, or a specification document that lives outside the code. Without a specification it's impossible to review this change for correctness.

@Patater
Copy link
Contributor

Patater commented Feb 7, 2019

What testing has been done so far?

Did we test that a system (e.g. K64F) with injected entropy in Mbed OS 5.11 still keeps proper state after reflashing the K64F to use the latest Mbed OS development branch? Is the saved entropy still available and usable?

@itayzafrir
Copy link
Contributor Author

What testing has been done so far?

Did we test that a system (e.g. K64F) with injected entropy in Mbed OS 5.11 still keeps proper state after reflashing the K64F to use the latest Mbed OS development branch? Is the saved entropy still available and usable?

Yes this has been tested as explained in mail.

@Patater
Copy link
Contributor

Patater commented Feb 7, 2019

On 2019-02-07 1330Z @itayzafrir writes:

Good news!
Seems like the entropy injected in 5.11 is preserved is master. Tested & debugged on K64F.

Validated using 2 modified branches on my fork:

  1. Branch "mbed-os-5.11-inject-entropy-no-delete":
    Branched out from mbed-os-5.11 and modified https://github.com/itayzafrir/mbed-os/blob/mbed-os-5.11-inject-entropy-no-delete/TESTS/psa/entropy_inject/main.cpp in a way that only the test injection_big_good is executed and this test resets ITS every time before injecting entropy. This branch leaves the injected entropy on the device after the test is done so be aware of this if you run it on your device.

  2. Branch " mbed-os-master-inject-entropy-preserved":
    Branched out from mbed-os master and modified https://github.com/itayzafrir/mbed-os/blob/mbed-os-master-inject-entropy-preserved/TESTS/psa/entropy_inject/main.cpp in a way that only the test injection_big_good is executed and removed the ITS resets from setup and teardown handlers. The test failed as expected on line 55 (mbedgt: :55::FAIL: Expected 0 Was 3) due to PSA_ERROR_NOT_PERMITTED error which is expected. I also debugged it to make sure that when mbedtls_psa_inject_entropy was called it got a PSA_ITS_SUCCESS code from psa_its_get_info( PSA_CRYPTO_ITS_RANDOM_SEED_UID, &p_info ) meaning that the entropy exists.

Patater
Patater previously approved these changes Feb 7, 2019
alekshex
alekshex previously approved these changes Feb 10, 2019
@Patater
Copy link
Contributor

Patater commented Feb 11, 2019

@itayzafrir Could you please add a specification explaining what the promise between versions is exactly, and/or provide an automated regression test?

Thanks

@Patater
Copy link
Contributor

Patater commented Feb 13, 2019

The specification is being provided through #53

This PR is OK for merge alongside #53

@Patater
Copy link
Contributor

Patater commented Feb 13, 2019

CI failure is FreeBSD timing test, a known issue.

Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Bad: code duplication:
  • Bad: the semantics of PSA_MAX_PERSISTENT_KEY_IDENTIFIER is no longer clear.
  • Incomplete: even once these issues are fixed, I don't know what the goal of this PR is.

@@ -59,7 +59,11 @@ extern "C" {
* This limitation will probably become moot when we implement client
* separation for key storage.
*/
#if defined(MBEDTLS_PSA_CRYPTO_SPM)
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff000000000000
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this value. Why is 0xffffffff00001234 ok (as the key id 0xffffffff with the owner 0x00001234)? What is this macro supposed to be used for? At the moment, it's one plus the maximum key id value that an application may use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is your macro, we're not using it externally. This has been explained in #32 (comment)
Please take a look

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But on PSA service integration, PSA_MAX_PERSISTENT_KEY_IDENTIFIER does not limit the key id values that applications can use. Applications can use any value between 0 and 0xffffffff inclusive, unless there's a check in the service code. So what's the point of setting PSA_MAX_PERSISTENT_KEY_IDENTIFIER to this particular value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The service uses the library as a black box. The service is not communicating with neither ITS (directly) nor any internal crypto APIs. So the code flow remains the same from the librarys' point of view.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, nothing is enforcing a particular range of key ids for an application. Which may be ok (it's a design decision that we're still free to take), but then PSA_MAX_PERSISTENT_KEY_IDENTIFIER is not useful in a PSA service integration.

Copy link
Contributor Author

@itayzafrir itayzafrir Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? The following code runs when crypto is used as a library but also as a PSA service:

static psa_status_t psa_internal_make_key_persistent( psa_key_handle_t handle,
psa_key_id_t id )
{
#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C)
psa_key_slot_t *slot;
psa_status_t status;
/* Reject id=0 because by general library conventions, 0 is an invalid
* value wherever possible. */
if( id == 0 )
return( PSA_ERROR_INVALID_ARGUMENT );
/* Reject high values because the file names are reserved for the
* library's internal use. */
if( id >= PSA_MAX_PERSISTENT_KEY_IDENTIFIER )
return( PSA_ERROR_INVALID_ARGUMENT );

Shouldn't the library (in both modes) reserve key ids for it's internal use as stated in documentation?

Reserve a whole range of key slots just in case something else comes up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, my bad, this does enforce an upper limit of 0xfffeffff for key ids, since the key id is in the upper 32 bits. However, the checks as described are still not correct.

On an upgrade of ITS from 32-bit identifiers to 64-bit identifiers, how are identifiers converted?

  • If the conversion is uint64_t new_key_id = old_key_id, then the entropy file is now PSA_CRYPTO_ITS_RANDOM_SEED_UID which is 0x00000000ffffff52. This is key id 0 of owner 0xffffff52. Per Prepare support for 64 bit key ids under SPM #32 (comment) (no validation of key ids in the service) these are valid values. 0xffffff52 has the most significant bit set which makes it a non-secure partition. We don't have multiple non-secure partitions on Mbed OS (yet?), but we do on Linux.
  • If the conversion is uint64_t new_key_id = (uint64_t)old_key_id << 32, then PSA_CRYPTO_ITS_RANDOM_SEED_UID needs to change to 0xffffff5200000000, or we need an upgrade path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the only key that must be preserved between Mbed OS versions 5.11 and 5.12 is the injected entropy key with id PSA_CRYPTO_ITS_RANDOM_SEED_UID.

NON-PSA Target: injected entropy preservation between versions 5.11 and 5.12:
This has been tested and validated on a K64F (non-PSA target i.e. without the crypto service), see #32 (comment).

PSA Target: injected entropy preservation between versions 5.11 and 5.12:
I don't see a reason to change the value of PSA_CRYPTO_ITS_RANDOM_SEED_UID, I'll try to explain my point of view...

The following is the entropy inject function implemented in mbed crypto:

psa_status_t mbedtls_psa_inject_entropy( const unsigned char *seed,
size_t seed_size )
{
psa_status_t status;
psa_its_status_t its_status;
struct psa_its_info_t p_info;
if( global_data.initialized )
return( PSA_ERROR_NOT_PERMITTED );
if( ( ( seed_size < MBEDTLS_ENTROPY_MIN_PLATFORM ) ||
( seed_size < MBEDTLS_ENTROPY_BLOCK_SIZE ) ) ||
( seed_size > MBEDTLS_ENTROPY_MAX_SEED_SIZE ) )
return( PSA_ERROR_INVALID_ARGUMENT );
its_status = psa_its_get_info( PSA_CRYPTO_ITS_RANDOM_SEED_UID, &p_info );
status = its_to_psa_error( its_status );
if( PSA_ITS_ERROR_UID_NOT_FOUND == its_status ) /* No seed exists */
{
its_status = psa_its_set( PSA_CRYPTO_ITS_RANDOM_SEED_UID, seed_size, seed, 0 );
status = its_to_psa_error( its_status );
}
else if( PSA_ITS_SUCCESS == its_status )
{
/* You should not be here. Seed needs to be injected only once */
status = PSA_ERROR_NOT_PERMITTED;
}
return( status );
}

As you can see entropy injection does not go through the "normal" flow other crypto keys do (i.e. psa_open_key or psa_create_key etc.), it talks to the ITS directly. There's no intervention by the crypto service (or anything else) that changes this key id into anything else. This means that we can keep using the same PSA_CRYPTO_ITS_RANDOM_SEED_UID value both in a system where ITS ids are 32 or 64 bits. The ITS does not care about the meaning of the id it recieves.

As to overwriting or obtaining a handle to a key with id PSA_CRYPTO_ITS_RANDOM_SEED_UID, the only way to access this ITS record is by talking to ITS directly, it will not be possible to do so via crypto API:

In a PSA target, if a user were to call psa_open_key or psa_create_key with id PSA_CRYPTO_ITS_RANDOM_SEED_UID the service would translate this value to:

  • If calling from NSPE (partition id -1) - ID sent to ITS = 0xffffff52ffffffff
  • If calling from different partition (for example partition id 3) - ID sent to ITS = 0xffffff5200000003

So I don't see a possible way of obtaining a handle to a key with id 0xffffff52 when going through the crypto service.

Moreover, the results from these 2 examples would not pass the check (in function psa_internal_make_key_persistent) against PSA_MAX_PERSISTENT_KEY_IDENTIFIER (when it's defined to 0xffff000000000000) as both 0xffffff52ffffffff and 0xffffff5200000003 are greater than 0xffff000000000000 and the function would return a PSA_ERROR_INVALID_ARGUMENT.

In a non-PSA system, the above is also true, if PSA_MAX_PERSISTENT_KEY_IDENTIFIER is defined as 0xffff0000 (as it is now), it will also fail the validation in psa_internal_make_key_persistent as 0xffffff52 is greater than 0xffff0000.

One more point, the crypto service does (as part of ARMmbed/mbed-os#9575) validate that key id is not 0 prior to calling the library's open/create a key, see:
https://github.com/ARMmbed/mbed-os/blob/3bc5f9086c3bbf1471906d6a4753457cf46f19cf/components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c#L1179
and
https://github.com/ARMmbed/mbed-os/blob/3bc5f9086c3bbf1471906d6a4753457cf46f19cf/components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c#L1201

Clarification on:

the only way to access this ITS record is by talking to ITS directly

It will only be possible to do so from the partition that created the ITS record. ITS has it's own validation/enforcement mechanism which prevents accessing ITS records from partitions which are not the record's owner. They do so by converting the id received into a string and prepending the calling partition id to that string. So if ITS was called directly from NSPE (0xffffffff) with id PSA_CRYPTO_ITS_RANDOM_SEED_UID (0xffffff52), the actual ITS record id would be converted (by ITS) into ffffffff_ffffff52. In our case, in a PSA system (where the crypto service id is 0x23), the injected entropy id that ITS eventually stores (as a string) is 23_ffffff52. This ensures that the entropy key will only be accessible from the crypto partition and only via direct access to ITS (i.e. without going through open/create key APIs).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a PSA target, if a user were to call psa_open_key or psa_create_key with id PSA_CRYPTO_ITS_RANDOM_SEED_UID the service would translate this value to:

If calling from NSPE (partition id -1) - ID sent to ITS = 0xffffff52ffffffff

On Linux, there are multiple partitions in the NSPE and they're numbered 0xffffffff, 0xfffffffe, etc. I don't know if it's already implemented, but if it isn't it's only a matter of weeks, and we need a solution that works in this time scale.

If partition 0xffffff52 wants to store key ID 0, this can only be blocked at the service level. The safety of the entropy seed file depends on this check in the service. I would strongly prefer a more robust solution where the entropy seed is in a domain that the PRoT already reserves for itself: owner_uid = 0.

@itayzafrir itayzafrir dismissed stale reviews from alekshex and Patater via d8a8074 February 14, 2019 09:03
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the code so far as a stepping stone, but we need to work some more to have a working design that correctly protects and upgrades the entropy file. I'm blocking this PR because I fear that it will lead to a system that works functionally, but is insecure. I'm willing to accept this PR if we have some clear follow-up steps that ensure that we will never ship a system where another partition can access the entropy file.

@itayzafrir
Copy link
Contributor Author

@gilles-peskine-arm please take a look at my answers in #32 (comment).

@itayzafrir itayzafrir force-pushed the ipc-support-64-bit-key-ids branch from abf46c2 to ff24f59 Compare February 19, 2019 12:52
@itayzafrir
Copy link
Contributor Author

@gilles-peskine-arm @Patater I have reverted commit Change PSA_MAX_PERSISTENT_KEY_IDENTIFIER to 64 bits when using ITS (43b9f2d) and also rebased to solve merge conflicts in file psa_crypto_storage_its.c.

Old branch https://github.com/itayzafrir/mbed-crypto/tree/ipc-support-64-bit-key-ids-3

@itayzafrir itayzafrir added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Feb 19, 2019
@Patater
Copy link
Contributor

Patater commented Feb 19, 2019

@itayzafrir Could you please delete the commit instead of add a revert commit?

itayzafrir added 2 commits February 19, 2019 15:08
@itayzafrir itayzafrir force-pushed the ipc-support-64-bit-key-ids branch from ff24f59 to 7723ab1 Compare February 19, 2019 13:10
@itayzafrir
Copy link
Contributor Author

@Patater, I removed the commit and also squashed some commits to make history clearer.

Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand how Mbed Crypto is being built and what parts of the code expect psa_key_id_t to mean psa_key_file_id_t. Do note that the hack whereby psa_key_id_t is aliased to psa_key_file_id_t is temporary: we will remove it at some point, and use psa_key_file_id_t in crypto.h. But if this works for you now, ok. Since this is purely an internal matter between the library and the service, we can change this without any compatibility concerns.

@gilles-peskine-arm
Copy link
Collaborator

Note that the main goal of this PR, namely support for 64-bit key file IDs encoding the key owner, is now fulfilled by #59. The goal of this PR is to integrate #59 into the PSA crypto service.

@itayzafrir
Copy link
Contributor Author

@gilles-peskine-arm

we will remove it at some point, and use psa_key_file_id_t in crypto.h...

Understood, it shouldn't make too much of a difference in the service side as long as we'll still be able to have separation for the identifier between the crypto service client the crypto service itself.

@Patater
Copy link
Contributor

Patater commented Feb 20, 2019

CI failure is FreeBSD timing test, a known-flaky test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: review The pull request is ready for review. This generally means that it has no known issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants