-
Notifications
You must be signed in to change notification settings - Fork 96
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
Prepare support for 64 bit key ids under SPM #32
Conversation
@Patater, @gilles-peskine-arm Should the following also be updated? mbed-crypto/include/psa/crypto_extra.h Line 38 in 3a02e53
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? |
3a02e53
to
fe7acb4
Compare
What's the logic behind changing The default random seed implementation uses the @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? |
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. |
237f8b7
to
919b89e
Compare
@Patater I've amended the commit and added the reasoning of changing About changing the value of |
Build failed on |
library/psa_crypto_storage.h
Outdated
#define PSA_MAX_PERSISTENT_KEY_IDENTIFIER 0xffff0000 | ||
#endif |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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.
919b89e
to
7b6f37f
Compare
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. |
On 2019-02-07 1330Z @itayzafrir writes:
|
@itayzafrir Could you please add a specification explaining what the promise between versions is exactly, and/or provide an automated regression test? Thanks |
CI failure is FreeBSD timing test, a known issue. |
There was a problem hiding this 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.
library/psa_crypto_storage.h
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
mbed-crypto/library/psa_crypto_slot_management.c
Lines 205 to 219 in 2d7e5fe
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.
There was a problem hiding this comment.
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 nowPSA_CRYPTO_ITS_RANDOM_SEED_UID
which is0x00000000ffffff52
. 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
, thenPSA_CRYPTO_ITS_RANDOM_SEED_UID
needs to change to0xffffff5200000000
, or we need an upgrade path.
There was a problem hiding this comment.
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:
mbed-crypto/library/psa_crypto.c
Lines 4427 to 4455 in 0574e6a
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).
There was a problem hiding this comment.
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
orpsa_create_key
with idPSA_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.
There was a problem hiding this 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.
@gilles-peskine-arm please take a look at my answers in #32 (comment). |
abf46c2
to
ff24f59
Compare
@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 Old branch https://github.com/itayzafrir/mbed-crypto/tree/ipc-support-64-bit-key-ids-3 |
@itayzafrir Could you please delete the commit instead of add a revert commit? |
Preparation for type separation between SPE and NSPE.
ff24f59
to
7723ab1
Compare
@Patater, I removed the commit and also squashed some commits to make history clearer. |
There was a problem hiding this 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.
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. |
CI failure is FreeBSD timing test, a known-flaky test. |
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 likecrypto_structs.h
is separated between client and server. Specifically, the typepsa_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.