secure_storage: add a ZMS-based implementation of the ITS store module

[tomi-font]

See the commit messages.

A follow-up PR will introduce the (static) allocation of psa_storage_uid_t values for end users of the PSA Secure Storage API.

[Laczen]

@tomi-font,
a. do not use storage_partition, this is already used for other purposes and cannot be used by two systems at the same time,
b. zms, nvs, or settings are not really a good fit for secure storage as they use more flash pages (at least 2) and ensure persistence, something that might not be required for secure storage,
c. the construction of the zms uid is fragile and might break as a result of firmware updates,

[Laczen]

@tomi-font I would advise on the following alternative approach:

1. A config variable CONFIG_ITS_NUM_ASSETS is defined,
2. A config variable CONFIG_ITS_FIRST_ASSET_ID is defined,

The id's CONFIG_ITS_FIRST_ASSET_ID until CONFIG_ITS_FIRST_ASSET_ID + CONFIG_ITS_NUM_ASSETS - 1 are used to store metadata (psa_its_id + uid.... + flags + nonce + tag),
The id's CONFIG_ITS_FIRST_ASSET_ID + CONFIG_ITS_NUM_ASSETS until CONFIG_ITS_FIRST_ASSET_ID + 2 * CONFIG_ITS_NUM_ASSETS -1 are used to store the data,

In the saving process the data is first written, then the metadata. In the reading process the metadata id's are searched for the correct psa_its_id, ... , a "reread" can be performed if needed (depending on the flags), followed by the read/validation of the data. At start any data that does not have a corresponding (or invalid) metadata can be removed.

This approach is also applicable to nvs, allows simultaneous use of nvs/... for secure storage, settings and other data storage (e.g when CONFIG_SETTINGS_NVS is defined the CONFIG_ITS_FIRST_ASSET_ID could be calculated as 0x8000 - 2 * CONFIG_ITS_NUM_ASSETS).

[tomi-font]

a. do not use storage_partition, this is already used for other purposes and cannot be used by two systems at the same time

Good point. I was thinking about how to handle potential conflicts, but I think that I will just remove the possibility to use storage_partition.

b. zms, nvs, or settings are not really a good fit for secure storage as they use more flash pages (at least 2) and ensure persistence, something that might not be required for secure storage

This makes no sense.

c. the construction of the zms uid is fragile and might break as a result of firmware updates

No it's not. And related to that, I will address the topic of UID assignment (for secure storage) in a follow-up PR.

I would advise on the following alternative approach:
[...]

I don't want to reinvent the storage wheel, plus I don't see the advantage in what you're proposing. Might as well just use the settings-based implementation.
For what you're proposing, there would need to be a mechanism to allocate ID ranges to different users of a same partition, which we don't currently have.

[Laczen]

b. zms, nvs, or settings are not really a good fit for secure storage as they use more flash pages (at least 2) and ensure persistence, something that might not be required for secure storage

This makes no sense.

Ok if you think they are a good fit.

c. the construction of the zms uid is fragile and might break as a result of firmware updates

No it's not. And related to that, I will address the topic of UID assignment (for secure storage) in a follow-up PR.

No need to address this in a follow-up PR, do it now before it gets used.

I would advise on the following alternative approach:
[...]

I don't want to reinvent the storage wheel, plus I don't see the advantage in what you're proposing. Might as well just use the settings-based implementation. For what you're proposing, there would need to be a mechanism to allocate ID ranges to different users of a same partition, which we don't currently have.

There are already users that are using the NVS backend for both settings and data storage, the allocation of ID ranges could be formalized.

If on the other hand it is needed that the secure storage has its own backend (as implied by the PR), a slightly modified copy of the flash backend used in tf-m would be more appropriate.

If the settings-based implementation fits the need, why propose a different alternative?

[tomi-font]

No need to address this in a follow-up PR, do it now before it gets used.

What I am talking about is not specific to what is added in this PR. It's about the PSA Secure Storage API overall. And the rationale for not including it as part of this PR is to keep the scope of a PR focused on one topic.

There are already users that are using the NVS backend for both settings and data storage, the allocation of ID ranges could be formalized.

I agree that it could and even should be formalized.

If on the other hand it is needed that the secure storage has its own backend (as implied by the PR)

No, it's not needed. This PR is just to allow direct use of a partition for more control and efficiency of the storage. For the majority of cases it will actually still default to using settings because storage_partition will be defined but not secure_storage_partition.

[Laczen]

No, it's not needed. This PR is just to allow direct use of a partition for more control and efficiency of the storage. For the majority of cases it will actually still default to using settings because storage_partition will be defined but not secure_storage_partition.

If it is worth using a partition for more control and efficiency of storage it would be good to target as much users as possible. NVS is still much broader used (it is more efficient for most users) and has a large install base. It doesn't seem a good choice to leave these users without a possible more efficient storage solution.

Splitting up the storage into metadata (uid, caller uid, flags, (optional) nonce, (optional) tag) and the data itself would give the following advantages:

1. No limitation is build into the uid,
2. No limitation is build into the caller uid,
3. The stack usage would be reduced as it is no longer required to create the combination of metadata and data before writing to storage,
4. It can be used by both nvs and zms,

[rghaddab]

Thank you for adding ZMS backend to secure_storage.
Some few comments but all minors.
Also maybe add a separate commit where you clang-format old code to get rid of github warnings

[tomi-font]

Also maybe add a separate commit where you clang-format old code to get rid of github warnings

I'm not a fan of the suggestions. Plus they're just notices. (And have sparked some debate on Discord.) They can be hidden with the a key. 🙂

[rghaddab]

Only some minor comments that you are free to take into account or not

[rghaddab]

only for a certain type of flash (that requires erase)

[de-nordic]

only for a certain type of flash (that requires erase)

I would rather say "for devices that require erase". There are devices that have erase and have paging (only for that purpose) but you can just write them without explicitly erasing first and without loosing written data in process.

[Laczen]

The proposed solution does not comply with the requirements of the psa-its standards. Why introduce the psa-its if we don't follow or adhere to the requirements ?

My change request: modify the solution to follow the psa-its standard.

[tomi-font]

The proposed solution does not comply with the requirements of the psa-its standards. Why introduce the psa-its if we don't follow or adhere to the requirements ?

My change request: modify the solution to follow the psa-its standard.

It has been extensively discussed and documented why we want an implementation that is not fully compliant with the official specification.
In:

• Discussions of the security working group.
• The RFC.
• The subsystem's documentation.

This is something that relevant stakeholders have approved.

[Laczen]

@ceolin I am directing my concerns to you as the chairperson of the security group.

By bringing in the psa-its standard into zephyr there are IMHO 2 reasons:

1. Allow standardising on the psa-its API,
2. Allow interoperability between different systems using a shared API,

The limitation of the psa-its API entries to zephyr specifications breaks reason number 2 and might result in incompatibilities between systems using a shared API. This is dangerous and might lead to not operational systems that are using a shared API. It is easy to imagine a solution where all systems would be instructed to load the key stored at 0xffffffff and the zephyr solution would fail. Cutting corners like is proposed in the solution has lead to problems in the past (e.g. the Mars Climate Orbiter incident) and should be avoided at all cost.

[tomi-font]

This is dangerous and might lead to not operational systems that are using a shared API

Exactly. Currently it's not safe for different users of the API that are not aware of each other. And this is is true whether the UID range is limited or not. Thus my mention of a follow-up PR that will address this issue which has always been present, since even before the introduction of the secure storage subsystem.

It is easy to imagine a solution where all systems would be instructed to load the key stored at 0xffffffff and the zephyr solution would fail.

You have a fundamental misunderstanding here. UID values are not chosen randomly/dynamically.
It seems you need to learn about PSA Crypto, PSA Secure Storage and maybe even Mbed TLS/TF-M to get a proper understanding of the topic.

But most importantly, you need to stop trying to block legitimate contributions with blanket concerns. This is even more ridiculous when it's clear that you don't know what you're talking about.

[frkv]

It is reasonable to ask questions and give comments in PR's, but the topics tackled here has been given ample attention both in the Zephyr Security Committee, The Zephyr Security Group in multiple meetings over a long time. The additions suggested in this PR (and related PRs on secure storage) is in my opinion well vetted and understood and accepted across the board for the people who decides what security means in Zephyr

I think we can gain much from the standardization of the API(s) in their functional form. Everything that is complicated about this in terms of security (or lack thereof) is thoroughly documented and will continue to get reasonable and rational attention in time to come

[de-nordic]

Looks OK.

[Laczen]

uid.uid is a psa_storage_uid_t which is a uint64_t, this cannot be casted to a (uint32_t) without duplicate uses of zms_id.

[tomi-font]

The result obtained from this is not used before uid is checked to not have any upper bits set (in has_forbidden_bits_set()), so here uid.uid can be considered to be (even less than) a uint32_t.

[Laczen]

1. Where is it documented that this specific implementation of the ITS store module has this limitation on bits?
2. How to handle that one implementation of the ITS store module has this limitation, while another does not?
3. Why use a 64bit uid when only 30bit are supported?

[tomi-font]

1. subsys/secure_storage/Kconfig.its_store:24
2. By letting the users of the API only use a well-defined range of UIDs. Will come in a follow-up PR as stated previously.
3. The 64-bit size originally comes from the specification. It could indeed be reduced when allowed values are officially defined.

[ceolin]

@ceolin I am directing my concerns to you as the chairperson of the security group.

By bringing in the psa-its standard into zephyr there are IMHO 2 reasons:

1. Allow standardising on the psa-its API,
2. Allow interoperability between different systems using a shared API,

The limitation of the psa-its API entries to zephyr specifications breaks reason number 2 and might result in incompatibilities between systems using a shared API. This is dangerous and might lead to not operational systems that are using a shared API. It is easy to imagine a solution where all systems would be instructed to load the key stored at 0xffffffff and the zephyr solution would fail. Cutting corners like is proposed in the solution has lead to problems in the past (e.g. the Mars Climate Orbiter incident) and should be avoided at all cost.

@Laczen these are valid questions to be asked :) It is so true that we spend a reasonable amount of time debating it in the security working group. The conclusion is that the best we can do regarding clash is what is being done in this pr. As @frkv said this is going to be extensively documented / emphasized to minimize the risk.

@tomi-font it does not hurt to mark it as experimental for now IMHO. Lets give it some time for testing before assume it is ready for production.

[tomi-font]

@tomi-font it does not hurt to mark it as experimental for now IMHO. Lets give it some time for testing before assume it is ready for production.

For sure. In fact, the whole subsystem is already marked as experimental. 🙂

[Laczen]

@ceolin I am directing my concerns to you as the chairperson of the security group.
By bringing in the psa-its standard into zephyr there are IMHO 2 reasons:

1. Allow standardising on the psa-its API,
2. Allow interoperability between different systems using a shared API,

The limitation of the psa-its API entries to zephyr specifications breaks reason number 2 and might result in incompatibilities between systems using a shared API. This is dangerous and might lead to not operational systems that are using a shared API. It is easy to imagine a solution where all systems would be instructed to load the key stored at 0xffffffff and the zephyr solution would fail. Cutting corners like is proposed in the solution has lead to problems in the past (e.g. the Mars Climate Orbiter incident) and should be avoided at all cost.

@Laczen these are valid questions to be asked :) It is so true that we spend a reasonable amount of time debating it in the security working group. The conclusion is that the best we can do regarding clash is what is being done in this pr. As @frkv said this is going to be extensively documented / emphasized to minimize the risk.

@ceolin, thank you for your reply. As an outsider to the security group I find it hard to believe that preference was given to a storage solution that is supported by bending or limiting the shared API instead of using a storage solution in a way to support the shared API.

I admit that I don't know much about the PSA Crypto, PSA Secure Storage, MBEDTLS, but even I can see directly that what is being done here will clash with using the secure storage solution for a PSA Crypto key store. As the PSA Crypto keys are defined to use the full 32 bit uid range (part for user key, part for vendor key, and the other part for future extensions).