Open ceolin opened 2 years ago
d3zd3z
commented
2 years ago Given https://www.wolfssl.com/platform-security-architecture-psa-crypto-api-support-wolfssl/, I think we are past the point of really deciding if PSA is the correct API to implement, and just planning on how we are going to get there.
carlescufi
commented
2 years ago Note: In the context of Bluetooth, there have been attempts to:
joerchan
commented
2 years ago Snapshot of the current features of TinyCrypt that we would required replacements for:
zephyr/drivers/crypto/Kconfig:
28: select TINYCRYPT_AES
29: select TINYCRYPT_AES_CBC
30: select TINYCRYPT_AES_CTR
31: select TINYCRYPT_AES_CCM
32: select TINYCRYPT_AES_CMAC
zephyr/subsys/bluetooth/common/Kconfig:
239: select TINYCRYPT_AES
zephyr/subsys/bluetooth/host/Kconfig:
118: select TINYCRYPT_AES
119: select TINYCRYPT_SHA256
120: select TINYCRYPT_SHA256_HMAC
121: select TINYCRYPT_SHA256_HMAC_PRNG
256: select TINYCRYPT_AES
257: select TINYCRYPT_AES_CMAC
633: select TINYCRYPT_ECC_DH
zephyr/subsys/bluetooth/host/Kconfig.gatt:
94: select TINYCRYPT_AES
95: select TINYCRYPT_AES_CMAC
zephyr/subsys/bluetooth/mesh/Kconfig:
10: select TINYCRYPT_AES
11: select TINYCRYPT_AES_CMAC
34: select TINYCRYPT_ECC_DH
zephyr/subsys/jwt/Kconfig:
25: select TINYCRYPT_SHA256
26: select TINYCRYPT_ECC_DSA
27: select TINYCRYPT_CTR_PRNG
28: select TINYCRYPT_AES
zephyr/subsys/mgmt/updatehub/Kconfig:
21: select TINYCRYPT_SHA256
zephyr/subsys/random/Kconfig:
94: select TINYCRYPT_CTR_PRNG if TINYCRYPT
95: select TINYCRYPT_AES if TINYCRYPT
zephyr/subsys/storage/flash_map/Kconfig:
45: select TINYCRYPT_SHA256
Vge0rge
commented
2 years ago In my opinion moving towards the PSA crypto APIs as a universal solution does make a lot of sense. There are two notes that I would like to mention here.
I still believe that moving to the PSA APIs is the way to go. I am just noting some possible challenges that will need to be handled.
microbuilder
commented
2 years ago @Vge0rge There is an effort to enable support for crypto accelerators in the PSA APIs, which I think is worth highlighting and solves a real problem: https://github.com/ARMmbed/mbedtls/blob/development/docs/proposed/psa-driver-interface.md
EDIT: You link to that proposal in point two, sorry. 🥸
microbuilder
commented
2 years ago I'd second that moving towards the PSA Crypto API make a lot of sense to me, even if there are clearly some issues to resolve around their use, and valid considerations of code size with the MbedTLS implementation. But resolving those issues will be less work than designing something else from scratch.
frkv
commented
2 years ago We are very happy with an intention to gravitate to PSA Crypto APIs!
The latest (released) version of nRF Connect SDK has an additional file describing Kconfig options for all PSA_WANT_ALG_XXX configurations found in Mbed TLS
https://github.com/nrfconnect/sdk-zephyr/blob/main/modules/mbedtls/Kconfig.psa
We have plans to upstream this, but we were waiting for the final decision on standardization
We have added two new configurations for the (for now) missing feature of configuring CTR_DRBG or HMAC_DRBG PRNG support:
We have internal logic in our SDK to handle configuring NSPE and SPE in case TF-M is used to make sure that the configurations correctly transition into the TF-M build and is exposed in the secure services.
The logic includes:
Our intention is to support both with and without TF-M and to make sure that there is only "one unit implementing PSA crypto" in the system.
With regards to out-of-box experience, it wouldn't take much to get the Zephyr-module of Mbed TLS to make use of our proposed Kconfig logic. It is also possible to consider making µECC into a PSA driver to decrease the flash-size (if need be).
HMAC DRBG is not a big item to fix in a downstream adaptation if we make use of our proposed PSA configuration PSA_WANT_ALG_HMAC_DRBG
ceolin
commented
2 years ago We are very happy with an intention to gravitate to PSA Crypto APIs!
The latest (released) version of nRF Connect SDK has an additional file describing Kconfig options for all PSA_WANT_ALG_XXX configurations found in Mbed TLS
https://github.com/nrfconnect/sdk-zephyr/blob/main/modules/mbedtls/Kconfig.psa
We have plans to upstream this, but we were waiting for the final decision on standardization
We have added two new configurations for the (for now) missing feature of configuring CTR_DRBG or HMAC_DRBG PRNG support:
We have internal logic in our SDK to handle configuring NSPE and SPE in case TF-M is used to make sure that the configurations correctly transition into the TF-M build and is exposed in the secure services.
The logic includes:
- Converting CONFIG_PSA_WANT_ALG_XXXX into PSA_WANT_ALG_XXXX in generated configuration files (for NSPE and SPE)
- Passing SPE configurations to TF-M build
- Adding Nordic specific PSA driver configurations for NSPE or SPE builds (for building and linking)
- Adding necessary C-files in NSPE or SPE for building PSA Crypto APIs
- Enabling TF-M secure services based on enabled crypto features
Our intention is to support both with and without TF-M and to make sure that there is only "one unit implementing PSA crypto" in the system.
Glad to hear it, that is one of my main concerns, we should not make Zephyr dependent of TF-M in order to have cryptography support.
With regards to out-of-box experience, it wouldn't take much to get the Zephyr-module of Mbed TLS to make use of our proposed Kconfig logic. It is also possible to consider making µECC into a PSA driver to decrease the flash-size (if need be).
Nice, we have to find a solution for the TinyCrypt EOL, uECC sounds like the direct alternative for it, but we have to have only one API to not have the same problem in future. We need to understand exactly what is the minimum requirements regarding rom/ram usage.
HMAC DRBG is not a big item to fix in a downstream adaptation if we make use of our proposed PSA configuration PSA_WANT_ALG_HMAC_DRBG
gregshue
commented
2 years ago From a cursory look it seems PSA Crypto API and Zephyr system interface have a common omission: calls that support a controlled de-initialization of the system. This matters when handing off the hardware from the boot image to the application image. And it affects not only the state of crypto hardware, but also the state of persistent storage, external chips, and even native UI.
To check my assumptions and foster discussion, here are the needs and requirements I have come up with so far:
Stakeholder needs:
System Requirements: A. When the device transitions from the boot image to the application image, the device shall have cleanly completed all updates to persistent storage. B. When the device transitions from the boot image to the application image, the device shall leave communications with external chips in a quiescent state. C. When the device transitions from the boot image to the applicaiton image, the device shall have DMAs disabled. D. When the device transitions from the boot image to the application image, the device shall have interrupts disabled. E. When the device transitions from the boot image to the application image, the device shall have the caches disabled.
Analysis: ⦁ Algorithmically cleaning up hardware agents activated in the boot image requires a de-initialization of the system (reverse dependency order). The proposed API does not provide a de-initialization call, and so does not support an extensible boot architecture implied by the Zephyr ecosystem. This precludes boot code using MCU-external cryptographic hardware agents.
I expect I am pretty naïve here. What have I missed?
gregshue
commented
2 years ago Here are several more generally related to APIs:
Stakeholder needs:
d3zd3z
commented
2 years ago Let me try to distill this down a bit, since most of these requirements are a bit higher level than what would be affected by the crypto API.
The first point I want to make is that asking this as a question "Should Zephyr Adopt the PSA Crypto API" is kind of a moot point. Mbed TLS has been a component of Zephyr for some time, and the Mbed TLS project is in transition to the PSA crypto API. The question here isn't about a general crypto API, which is already the PSA Crypto API within Zephyr, but about a small number of cryptographic primitives that are used by other functionality built into Zephyr. Right now, these use some other ad-hoc libraries (e.g. tinycrypt) that have to be replaced due to lack of upstream support. We're proposing moving these to use the PSA crypto API, and in some sense, the requirements we need here are in regards to what these subsystems need.
But, back to the above lists from @gregshue:
Again, to summarize. There are numerous points raised here. Many of them aren't really related to the use of a crypto API, and should be raised as their own issues.
As far as using the PSA Crypto API within Zephyr, that's already the case. By officially adopting it, we're discussing migrating existing users of other crypto libraries (notably tinycrypt) to use the PSA API, and what requirements there are behind that.
gregshue
commented
2 years ago Let me try to distill this down a bit, since most of these requirements are a bit higher level than what would be affected by the crypto API.
The crypto API would be affected by the system requirements, not the other way around.
Given that the current implementation is entirely software, there really isn't anything to deinitialize,
We are evaluating an API to algorithms that could be implemented in hardware and may already be for downstream users, so "current" is irrelevant. For hardware implementations, those that interact with persistent storage, or even those that use generic DMA there may be something to de-initialize, so the API needs to provide the hook.
Related, as far as priority of work, currently for a software solution, work is done in the caller's thread, for basic cryptographic operations. For features such as TLS, this is generally done by networking worker threads.
These need to be documented in the API and applied as constraints to all implementations. Now, what about for non-basic cryptographic operations? What about for the cases not done in by the networking worker threads? The downstream integrator must know this information to figure out if any implementation of this API is even an option.
this is pretty hard to enforce within Zephyr, as none of our current toolchains have any way of enforcing this kind of limit.
Actually, it is pretty easy to enforce. Choose a relatively large number for N (e.g., 255); in a kernel file define two different types of symbols that are that differ only in the last character and the type associated with them (e.g., int8_t, uint8_t). If the linker doesn't support the necessary resolution you will get a build failure. If any symbol file includes a non-mangled symbol of >N characters generate a build failure.
This is worth raising an issue to track, as it has nothing to do with the crypto API
We are gathering requirements related to APIs and related to system so we can evaluate whether or not the PSA crypto API meets them. It happens to meet this requirement, but not others already identified.
As far as symbol conflicts go, this is a general issue, and should be raised as such.
At the last TSC meeting they decided to kick off a new Project in the organization for looking in to Global Namespace Mgmt. It was created 5 days ago.
The only issue I see possibly affecting the crypto api would be the ability to cancel an operation. For a software implementation, this would be no different than canceling a running thread. For something with hardware crypto, this is challenging to address in a device-independent way, and until we have a better idea of how hardware addresses these issues, across diverse devices, it is difficult to know what could even be done that is general.
Zephyr does not support restarting threads, so canceling/killing a running thread is not an option.
In general, the driver must solve the problem. If the problem requires a hardware change to support the capability, then that particular hardware does not meet all the system requirements and has a more limited application space.
Now, what about supporting multiple API instances in the same executable (alternate implementations)?
d3zd3z
commented
2 years ago this is pretty hard to enforce within Zephyr, as none of our current toolchains have any way of enforcing this kind of limit. Actually, it is pretty easy to enforce. Choose a relatively large number for N (e.g., 255); in a kernel file define two different types of symbols that are that differ only in the last character and the type associated with them (e.g., int8_t, uint8_t). If the linker doesn't support the necessary resolution you will get a build failure. If any symbol file includes a non-mangled symbol of >N characters generate a build failure.
This is actually the opposite of what I mean by enforcing. In order for Zephyr to be able to say that it doesn't have any symbols that differ after N characters, we need a tool that can evaluate the entire Zephyr tree and determine if there are any conflicts. I don't believe this tool currently exists (although it wouldn't be difficult to add something to do this based on the output of 'nm' for a given build. You're approach really only would tell you if a given tool doesn't support N distinct characters, which isn't actually something Zephyr needs to know (it might be useful to someone).
I do believe this is a fairly minor issue. It probably should be raised (completely independently of this issue), since it is real. But, I would like to see some evidence of toolchains that have small limits on distinct characters. Practically, I think this is much easier to solve by just fixing the issues that are found (since C99 requires 31 characters, and conflicts past that are likely to be uncommon).
d3zd3z
commented
2 years ago These issues aside, we are getting very distracted from the question being raised by this issue, which is whether or not Zephyr should consider the PSA crypto API to replace some of the other APIs that are used within the system. This issue is not about whether Zephyr will use or provide the PSA crypto API. That is already the case. What we are wanting to do here is replace the usage of two existing crypto APIs (both limited to a fairly small scope), and change the in-Zephyr code that uses these APIs to use the PSA API instead. In addition, this will involve deprecating these APIs, and giving an opportunity for any clients that are currently using them to migrate to the PSA API.
In this light, I'd like to ask if we can try to keep this issue focused on this specific question. There are a lot of other concerns raised, and these should be raised in other issues, if necessary. Notably, whether we provide the PSA crypto API is not a question here, as this API is already available in Zephyr. This issue should focus on code, both within Zephyr, and external code that makes use of https://github.com/zephyrproject-rtos/zephyr/blob/main/include/crypto/crypto.h, as well as uses the tinycrypt library, as we want to deprecate these interfaces, and migrate this code to use the PSA crypto APIs.
In light of this, the main constraint (brought up in the description of this issue) has to do with the compact implementation of elliptic curve crypto in the tiny crypt library. The Mbed TLS ECC implementation is both larger and slower, and also requires heap allocation, which tinycrypt does not. It is worth investigating whether implementing a PSA wrapper for the ECC operations in uECC (the upstream code used in Tinycrypt) will be sufficient for the needs of current code that makes use of the API.
d3zd3z
commented
2 years ago I'd also like to know that if anyone is aware of a crypto implementation that uses a different API, it would be good to hear about it. We would have to replace a lot of code to move to something that isn't Mbed TLS (at least that has a different API). I'm aware of Wolf-SSL, which, although would work with Zephyr, the free version is likely to not be license compatible with Zephyr code. Wolf SSL has also implemented the PSA Crypto API. Replacing the entire crypto API with a different one would be a lot of work, and probably of limited benefit, especially as I'm not aware of anything comparable. The Zephyr project is certainly not in a position to implement and maintain its own body of code comparable to Mbed TLS.
microbuilder
commented
2 years ago I'm a bit confused by the overall conversation, particularly after the last security call.
PSA doesn't cover every possible edge case. Clearly. It's also good to identify the use cases it doesn't give us 100% coverage of and figure out what we can or should do to address those. But does not having every use case covered exclude the choice to migrate to the PSA API, and if PSA is deemed 'insufficient', what's the alternative here? I don't really see anything else that would give us something better. PSA isn't perfect ... but what's the net gain alternative?
This make me think of DeviceTree a bit. Zephyr makes use of DT as a useful abstraction for variations in HW, but the way we do DT today certainly doesn't cover every possible use case. It's imperfect. But that's how engineering works ... you have a goal in mind, and you take the tools that you do have today, you make compromises that at least move in the desired direction, and invest in tooling making things better as you go. We're constantly improving Zephyr's DT implementation and tooling to enable various edge cases as we go. It seems to me the PSA API is similar, and it brings clear value to the project as a common starting point, and we're certainly free to iterate on it, engage with the PSA authors upstream, and adapt it to our needs, and as with DT it brings more benefit to Zephyr than not having anything at all.
I understand the valid concerns being made about things the PSA and Zephyr can't do today. But to me none of that precludes officially adopting PSA as a common API for crypto etc. Those feel like very different issues, and not being able to do X/Y/Z with PSA today doesn't, in my mind, exclude a migration to PSA, anymore than not being able to model X/Y/Z in DT means we should reject DT at a project level either.
d3zd3z
commented
2 years ago @carlescufi wrote:
Note: In the context of Bluetooth, there have been attempts to:
- Replace TinyCrypt with mbedTLS: Rejected because of higher ROM footprint: https://github.com/zephyrproject-rtos/zephyr/issues/8081
Unfortunately, we do have to figure something out, because tinycrypt has become unmaintained. However, the ECC code within it isn't originally from tinycrypt, but comes from a project uECC (https://github.com/kmackay/micro-ecc, which although not highly active, does seem to have an active maintainer). It would be possible to replace hash and cipher options with Mbed TLS, and if configured properly, shouldn't be that large of a code-size increase, and either 1. continue to use the uECC code for the ECC operations within Bluetooth, or implement uECC as an ECC backend for PSA.
Whether this solution is viable for Bluetooth is really the main point of this issue and discussion.
microbuilder
commented
2 years ago @gregshue It seems like some of the concerns you're raising should in fact be individual requirements in the larger security planning of Zephyr, and they should be filed individually and tracked and addressed (via the security project page in Zephyr, etc.) based on current needs, resources and priorities.
This issue specifically is trying to address one such problem: TinyCrypt being unmaintained, and Zephyr likely having to move away from it, and what we do at an API level to harmonize our general approach to crypto to resolve this issue today and in the future.
PSA is the one viable API solution I'm aware of myself, imperfect as it may be, meaning that we need to figure out how to address the performance issues of MbedTLS as one implementation of the PSA API with the requirements of the BLE and other security-depedent subsystems. That can mean several things:
Relying on an unmaintained crypto library doesn't seem like a valid long term solution here, though, and PSA gives us a neutral, somewhat mature and vetted translation layer that allows us to solve this problem (with additional engineering effort), and adds flexibility as other crypto issues bubble to the surface in the future. It doesn't solve every problem, or consider every use case, but we can address those requirements case by case unless a better intial proposition is put on the table. The problem with TinyCrypt isn't going away, so we need to propose something.
I'd encourage you to raise individual issues with the larger security planning/goals of the project (controlled de-initialization, etc.), tracking those goals here: https://github.com/zephyrproject-rtos/zephyr/projects/24
But I'm not persuaded those concerns preclude adopting PSA as a first large steps towards making crypto more maintainable longer term for the Zephyr project simply because PSA won't solve every problem out of the box. Of course it won't, but as a community we can still work towards addressing needs case by case, and again ... what's a better alternative if PSA is insufficient, as were early attempts at adopting DT in Zephyr?
frkv
commented
2 years ago The statement that the PSA Crypto APIs doesn't do deinitialization is only partially true. I want to split my answer into two parts:
I'm going to use the standardized terms in PSA for crypto drivers. Crypto-functionality executed on an external chip is interfaced through an opaque PSA driver. The "normal" PSA driver is called a transparent PSA driver and it gets its name from the fact that memory of keys is accessible from the CPU.
Transient states
There are in fact standard features in Mbed TLS to do deinitialization of context information for individual crypto operations. These are used for error-scenarios and for aborted or finalized operations. A similar functionality is extended to PSA Crypto APIs run in environments with TrustZone features is used (e.g. in TF-M). In this case the functionality is also coupled with bookkeeping of a finite number of "accessible slots" to do cryptographic operations inside the secure image. Anything that fails or is finalized must be deinitialized by the system to make room for the next operation.
The same functionality also exists for cryptography executed using oqaue drivers. It is completely up to the implementer of an opaque driver type to do bookeeping at failure or finalization or an operation to set the external chip in a quiescent state (including optionally handling DMA, interrupts etc.)
Global states
The Mbed TLS implementation keeps some global information after psa_crypto_init has been called, mainly keeping track of the RNG state. There exists a function to deinitialize this global state, but it may be a bit broad for the use-case as it clears all data associated with the PSA layer, including the whole key store. (volatile key store, not persistent key store)
I would suggest proposing an addition to the PSA Crypto API spec for deinitialization. We can propose psa_crypto_deinit, a function that clears out the RNG state and any additional held states in a user function (e.g. in HW accelerators or external security chips)
When it comes to PSA driver layer (transparent and opaque) the documentation in Mbed TLS project states that the drivers can have an init-function (if need be). We would need to propose an addition to do PSA driver deinitialization to handle clearing out global states.
With a few additions, then there will be no problem to do cleanups of DMA, interrupts and caches and reaching quiescent state of any external chips. PSA Crypto APIs (and drivers) may be a bit simplistic on global state management right now, but it wouldn't take a lot to rectify this....
frkv
commented
2 years ago There are multiple ways persistent storage work in the PSA crypto APIs so a bit of explanation is needed to ensure a full understanding of the options (and the impacts).
Simple usecase: Keys stored in regular flash
The PSA Crypto APIs have been developed with the Arm PSA (e.g. via TF-M) as the backdrop. There is a distinguishing between crypto operations and storage operations inside the Arm PSA specification. The key storage for crypto operations is placed in a module called the internal trusted storage or ITS for short. This exists as a separated module in TF-M implementation, living outside of the PSA Crypto module.
Every API that handles persistent key storage and retrieval in PSA Crypto APIs in Mbed TLS will sooner or later call an ITS API under the hood.
On devices that can use TF-M: Implement a flash driver for your device and everything is provided out-of-the-box
On devices that can't use TF-M: If you are using Mbed TLS as the defacto implementation of PSA APIs, then you would need to enable native PSA ITS support and implement 4 APIs to give the ability to store/retrieve data from regular FLASH. Our company has done this using the Settings subsystem in Zephyr as a "backend" for PSA ITS (pure C)
Efforts on our part: The decision will be whether or not to use Mbed TLS implementation for key handling (with addition of native ITS support) or to build this functionality from scratch
Complex usecase: Keys stored in external security chip
The PSA Crypto implementation in Mbed TLS describes the term opaque PSA drivers. Opaque PSA drivers enables key usage that happens outside of the chip, meaning the CPU can only refer to the key material but never actually hold it.
The opaque PSA drivers is supported in such a way that you can use pre-provisioned keys or import keys on your own (if the external security chip supports it). The way to distinguish between these keys and volatile key or keys stored in regular flash (e.g. using PSA ITS) is the identifier for the key.
Efforts on our part: Limited. We just need to support opaque PSA drivers to ensure that any customer that has a specialized way of storing keys off-chip still get the features they need
gregshue
commented
2 years ago @d3zd3z wrote:
This issue is not about whether Zephyr will use or provide the PSA crypto API. That is already the case. What we are wanting to do here is replace the usage of two existing crypto APIs (both limited to a fairly small scope), and change the in-Zephyr code that uses these APIs to use the PSA API instead.
In that case this issue is poorly named and described. It sounds like you simply want to deprecate the existing non-PSA crypto APIs. There is no need to "adopt" or evaluate anything, the listing of a proprietary api as an alternative is really out-of-scope, and the problem of not being able to "take full advantage of HW accelerators" is irrelevant to this issue. Maintainers would automatically migrate to the existing non-deprecated API.
@microbuilder wrote:
But that's how engineering works ... you have a goal in mind, and you take the tools that you do have today, you make compromises that at least move in the desired direction, and invest in tooling making things better as you go.
Safety systems, and now secure IoT systems (due to regulations), need to be engineered to meet requirements imposed on the system by the customers (end users)/stakeholders (Linux Foundation, etc.). We get to compromise on design decisions, but compromises on requirements must be negotiated with the stakeholders.
@frkv wrote:
With a few additions, then there will be no problem to do cleanups of DMA, interrupts and caches and reaching quiescent state of any external chips. PSA Crypto APIs (and drivers) may be a bit simplistic on global state management right now, but it wouldn't take a lot to rectify this....
I completely agree that it wouldn't take a lot to rectify this. I just was raising awareness that it currently doesn't solve the stated problem and we have needs that they don't seem to be aware of or concerned with.
gregshue
commented
2 years ago @microbuilder Do you see de-initialization as an enhancement request (to SYS_INIT()), as a new feature, or as an RFC/proposal?
d3zd3z
commented
2 years ago In that case this issue is poorly named and described. It sounds like you simply want to deprecate the existing non-PSA crypto APIs. There is no need to "adopt" or evaluate anything, the listing of a proprietary api as an alternative is really out-of-scope, and the problem of not being able to "take full advantage of HW accelerators" is irrelevant to this issue. Maintainers would automatically migrate to the existing non-deprecated API.
Indeed, most of this probably is out of scope for this issue. But, that doesn't mean the discussion is unwanted or unwarranted. Bringing the issues up is how we learn about them.
Keep in mind, also, that bringing up requirements, even creating issues to track them, doesn't necessarily mean they are going to be addressed. Zephyr is a collaborative effort. For the most part, there isn't anybody sitting around waiting to work on requested features, and for the most part, features will have to be implemented by those that need them.
The advantage we have, is that a lot of these issues, esp things like deinitialization, are likely to be shared needs, and the implementation effort can also be shared.
microbuilder
commented
2 years ago @microbuilder Do you see de-initialization as an enhancement request (to SYS_INIT()), as a new feature, or as an RFC/proposal?
Ideally, an RFC/proposal in my opinion stating clearly how you think this could or should work.
This brings up an interesting detail for the PSA API, though. PSA is written in isolation from the implementation(s) itself. As you alluded to in an earlier comment, it defines requirements, codifying those in an API and reference documentation, and those are then implemented in something like TF-M (as a reference PSA implementation). The requirement comes first.
In order to influence the PSA API, we need to define new requirements for consideration in the published standards, and make a specific API suggestions to them. I'm not 100% sure what the ideal workflow for that is. We can do that in Zephyr as an RFC/proposal, and once consensus is reached that it solves our needs, escalate that for review to the PSA authors. But we probably do need to determine how this works to have it officially considered for adoption into the PSA, or if we're OK with Zephyr-specific additions on top of the published standards.
gregshue
commented
2 years ago I'll create the initial RFC for clean handoff of HW state from boot to app image. This will provide one user need for general de-initialization, leading to the PSA proposal.
gregshue
commented
2 years ago #44564 Security: Clean disable of boot image and hardware before handoff to app image has been created.
gilles-peskine-arm
commented
2 years ago Hi! I'm an Mbed TLS developer and architect of the PSA crypto API. I'm happy that you're considering our work and always open to feedback. I'll address some of the points raised here. Please don't hesitate to ask further questions and let me know if I've missed something (this is a long thread!).
The major reason to use TinyCrypt is due constraint resources, will PSA crypto be able to achieve similar requirements.
The PSA API is pretty abstract because it caters for many different scenarios. The more of this flexibility an implementation of this API supports, the more RAM and code size it's likely to require. Additionally, an implementation that's structured as the PSA API on top of another API will have some overhead due to the API translation layer.
It would certainly be possible to build a PSA API layer on top of (e.g.) tinycrypt. You'd get the performance of tinycrypt with some RAM and code overhead. You could reduce this overhead by forking tinycrypt and changing data representations and dispatch, but then you'd have to become maintainers of a crypto implementation. The PSA API is not just a crypto API, but simultaneously also a key store API: keys are always accessed via handles. (The reason for this design choice is to allow applications to transparently access opaque keys, e.g. keys in a secure element/enclave/world/service/...) This is likely to incur a small inescapable overhead compared to an implementation where keys are accessed as pointers to the key material.
Mbed TLS has quite a bit of flexibility (support for acceleration drivers, support for opaque keys, support for multiple curves, ...) and security (defense against local timing side channels) which have a cost, mainly in code size. In addition, it's structured as PSA over the legacy mbedtls_ API, so there's overhead that we haven't optimized yet. But even an ideally optimized Mbed TLS can't beat a specialized implementation for a single curve on a specific platform.
To keep the code size down with Mbed TLS, please make sure to use link-time optimization. This has a significant impact when optimizing for code size (but negligible when optimizing for performance).
As of Mbed TLS 3.1, drivers are supported for most algorithms, but you have to write your own psa_crypto_driver_wrappers.c and psa/crypto_driver_contexts_*.h. The generation of these files from a JSON description is ongoing work. Make sure to enable MBEDTLS_PSA_CRYPTO_CONFIG in addition to MBEDTLS_PSA_CRYPTO_DRIVERS to disable the software implementation. Unfortunately, this is not yet fully supported for X.509/TLS (once again, it's ongoing work).
You can follow our major work items on the Mbed TLS epic board. If you'd like to influence our schedule, let us know what your priorities are, but please be aware that our schedule is already pretty full for 2022 and beyond. Pull requests are welcome, but our main bottleneck is review — if you'd like Mbed TLS to progress faster, the best way you can help us is by onboarding for the long haul as a reviewer.
The PSA APIs do not seem to provide flexible (enough) configuration option support for crypto accelerators yet. To reduce the code size it will be important to enable/disable specific functionalities in the PSA driver level. For example it should be possible for crypto accelerators to enable SHA256 and nothing else. Even though in the current state there are configuration options for this (PSA_WANT_ALG_SHA_256), I could not find any finalized configuration option for accelerators. There are some accelerator configurations (MBEDTLS_PSA_ACCEL_ALG_SHA_256) which does not seem to be used much in the code and they don't have a finalized structure.
#define MBEDTLS_PSA_CRYPTO_CONFIG
#define MBEDTLS_PSA_CRYPTO_DRIVERS
#define PSA_WANT_ALG_SHA_256
#define MBEDTLS_PSA_ACCEL_ALG_SHA_256library/psa_crypto_driver_wrappers.c to add your dispatch code and include/psa/crypto_driver_contexts_primitives.h to add your operation representation.The CSRNG support from PSA APIs is lacking at the moment. There is a psa_generate_random function available but the entropy interface as specified in the driver proposal here is not fully implemented.
The only thing we support for now is to completely replace the PSA RNG subsystem with MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG. We aim to fully implement the PSA API in the future.
And additionally, configuration options for enabling CTR_DRBG or HMAC_DRBG using purely PSA APIs are lacking.
This is also on our backlog.
From a cursory look it seems PSA Crypto API and Zephyr system interface have a common omission: calls that support a controlled de-initialization of the system.
I would suggest proposing an addition to the PSA Crypto API spec for deinitialization. We can propose psa_crypto_deinit, a function that clears out the RNG state and any additional held states in a user function (e.g. in HW accelerators or external security chips)
We've considered having deinitialization in the API, but currently it's left out. Should init(); init(); deinit(); guarantee deinitialization when deinit() returns, or should it take as many deinit() calls as init() calls? (In practice the init() calls would come from separate libraries that aren't aware of each other.) I think the best answer heavily depends on the system architecture, which is why it was left out even as an optional feature (some platforms just won't support deinit).
Mbed TLS has mbedtls_psa_crypto_free() which deinitializes the whole PSA subsystem immediately (moslty intended for use in tests).
We do plan to add partial initialization as an optional, but standardized feature (e.g. initialize the key store without initializing the RNG).
I think it would make sense for bootloader handover to use a Zephyr-specific partial deinitialization feature.
As a developer of resource constrained embedded systems that need to support controlled disablement and cancel functionality, I need to use asynchronous API operations so that I can minimize RAM usage (fewer threads/stacks)
We have no plans for an asynchronous crypto API in PSA. The PSA firmware framework API is entirely synchronous. The PSA crypto API is independent of the firmware framework API, but it's designed to be compatible. An asynchronous API is very difficult to implement and is very sensitive to the precise way communication works, so I don't think Arm is likely to design an asynchronous crypto API.
For highly constrained systems that need to perform long interruptible operations without relying on multiple threads, Mbed TLS has a feature we call restartable ECC which allows ECC operations to be broken down into short-duration blocks that the application loops over. We plan to standardize this for at least signature and key agreement in the PSA API.
As a developer of mechatronic devices built on Zephyr, I need to cancel arbitrary operations so that I can support user-initiated Cancel functionality.
We are unlikely to ever support cancellation as such inside Mbed TLS functions. However, you can implement this on top of restartable operations, or (with a bit less RAM overhead) we could add cancellation points as a build-time alternative to restartable operations.
As a developer of software models of hardware, I need to have a single executable support both hardware and software implementations of a specific functionality so that I can run predictive maintenance models.
The PSA API allows fallback from accelerator drivers: you can specify at build time that a driver requires a fallback, and then if the driver returns PSA_ERROR_NOT_SUPPORTED the software implementation (or the next driver) will take over. While this was primarily intended for partial hardware support (e.g. hardware that only supports certain key sizes), a driver could choose to request fallback for other reasons.
These issues aside, we are getting very distracted from the question being raised by this issue, which is whether or not Zephyr should consider the PSA crypto API to replace some of the other APIs that are used within the system.
Sorry...
PSA doesn't cover every possible edge case. Clearly. It's also good to identify the use cases it doesn't give us 100% coverage of and figure out what we can or should do to address those. But does not having every use case covered exclude the choice to migrate to the PSA API, and if PSA is deemed 'insufficient', what's the alternative here? I don't really see anything else that would give us something better. PSA isn't perfect ... but what's the net gain alternative?
In order to influence the PSA API, we need to define new requirements for consideration in the published standards, and make a specific API suggestions to them. I'm not 100% sure what the ideal workflow for that is.
If there are things you think we should add to PSA crypto API, please let us know. The best place would be the psa-crypto mailing list. If you'd like to contact Arm confidentially about the PSA crypto API, you can email us at mbed-crypto@arm.com. We'll also take into accounts things requested via Mbed TLS issues (which are of course the best place to request for something to be implemented).
We can do that in Zephyr as an RFC/proposal, and once consensus is reached that it solves our needs, escalate that for review to the PSA authors. But we probably do need to determine how this works to have it officially considered for adoption into the PSA, or if we're OK with Zephyr-specific additions on top of the published standards.
Although we do require evidence of feasibility before finalizing an API, it's perfectly fine (and, I think, better) to start the discussion early, to avoid committing resources into something that we know won't be acceptable for some reason.
gregshue
commented
2 years ago @gilles-peskine-arm Thank you for the answers. Here are my thoughts:
I think the best answer heavily depends on the system architecture, which is why it was left out even as an optional feature (some platforms just won't support deinit).
I understand the struggle, but leaving it out entirely is the least desirable option for those of us that are reusing (not tweaking) the API or code.
I think you identified the most practical use cases: require a deinit for every init, executing the functionality on the last call; execute deinit on the first call. This begs for a build configuration. The default configuration probably doesn't matter since somebody won't be happy either way.
I struggle trying to recommend how to handle excessive deinit calls. It seems like it indicates a poorly-controlled API usage, but it should be an idempotent operation.
I think it would make sense for bootloader handover to use a Zephyr-specific partial deinitialization feature.
For Zephyr-based systems, I don't expect there to be more than one init and one deinit call. The MCUboot image executes the multi-threaded RTOS kernel and can contain whatever other drivers/subsystems/persistent storage and UI is needed by the product developer. So our "Zephyr-specific partial deinitialization" needs to be composable, extensible, and in reverse-dependency order. Hence, the need for deinit (essentially a static object destructor).
I think we ought to request support for deinit - after we align on our use case(s). :-)
I expect we can build a sufficient Cancel solution around restartable ECC. Thank you for pointing this option out.
The fallback mechanism covers the practical use cases that come to mind. At some point we just have to use multiple builds and inject data for verification. Thank you for pointing this option out.
ruuddw
commented
2 years ago As discussed and agreed in last Security WG, here's my list of some requirements that come to mind when selecting/defining a Crypto API for Zephyr. The discussion here seems to mix few things, let's separate those:
Common API related requirements
Alternative APIs that could be considered:
ceolin
commented
2 years ago Alternative APIs that could be considered: - PKCS#11, OpenSSL, WolfCrypt, (and many more ...) - Zephyr device driver style API in stead of crypto lib API (like current crypto.h, but extend with PK crypto) - no preferred Zephyr API, simply support multiple/different ones and accept that applications are not portable
Not having a preferred Zephyr API is not problem IMHO, the problem is having multiple API / implementations being used internally. It doesn't make a sense for me having to have TinyCrypt and mbedTLS in the same binary just because different modules decided to use different APIs / implementation.
dleach02
commented
2 years ago coming late to this. Does PSA crypto APIs have the ability to switch between HW accelerated crypto and SW only crypto? I'm aware of a use case where that would be needed.
dleach02
commented
2 years ago coming late to this. Does PSA crypto APIs have the ability to switch between HW accelerated crypto and SW only crypto? I'm aware of a use case where that would be needed.
As I read through above I see the this which implies PSA API meets this need:
The PSA API allows fallback from accelerator drivers: you can specify at build time that a driver requires a fallback, and then if the driver returns PSA_ERROR_NOT_SUPPORTED the software implementation (or the next driver) will take over. While this was primarily intended for partial hardware support (e.g. hardware that only supports certain key sizes), a driver could choose to request fallback for other reasons.
ceolin
commented
2 years ago coming late to this. Does PSA crypto APIs have the ability to switch between HW accelerated crypto and SW only crypto? I'm aware of a use case where that would be needed.
yes, it is possible AFAIK. You can have multiple drivers for the same operation.
dpkristensen
commented
1 year ago Seems like there's a lot of the security folks in here, so I wanted to ask about this in relation to #56154
I think having a better API would help facilitate the transition to newer algorithms like this; thoughts?
nordicjm
commented
7 months ago This is the first I have heard about this (for tinycrypt to be removed) from this pr: https://github.com/zephyrproject-rtos/zephyr/pull/71947
I absolutely object to this, tinycrypt has an important use for MCUmgr which is for generating sha256 hashes of files and sending them to clients, the proposed PR removes tinycrypt and only allows using mbedtls which has a 10.6KiB bump in flash requirements, this is a flat out no and I will never ack that PR. So some compromise has to be made for the many devices in zephyr that have lower amounts of flash, I don't really see C code generating sha256 hashes as needing to be maintained - it's a simple enough feature, it's worked for years and it works fine now so I do not see the merit in forcing the removal without any sort of usable replacement. It generates a hash for a file, I really don't care less about PSA crypto for that, it doesn't interest me in the slightest, it needs to be a small code size, that's what is important (if someone wants to use PSA crypto for it instead, at their choice then that's fine, but the minimal size option should still exist and be the default). And not withstanding you would be silently breaking many devices of users and prevent them from evening being able to use zephyr 3.7, I can count 4 zephyr-based devices I developed in my home that would just completely break from this change due to insufficient flash (2xstm32f301, 1xnrf52832, 1xnrf52810)
dpkristensen
commented
7 months ago @nordicjm PSA is a relatively new thing; they have an API which looks like it's meant to serve as a HAL. PSA as an API surface could be hooked into TC just as well as other libraries like Oberon, but it would have to be a first-class API provided by Zephyr. Right now, PSA only exists as a backend to mbed TLS, and probably only because mbed TLS started using it. The down-side to such a solution is that if TC doesn't support some operation, it will just generate a linker error when the function isn't implemented.
I had proposed another solution for a flexible API surface as well on #56154 which is not based on PSA.
ceolin
commented
7 months ago This is the first I have heard about this (for tinycrypt to be removed) from this pr: #71947
That's is not fair, this discussion is happening for a long time, not only in this issue but also in security working group and it was presented in TSC (long time ago) as a goal.
I absolutely object to this, tinycrypt has an important use for MCUmgr which is for generating sha256 hashes of files and sending them to clients, the proposed PR removes tinycrypt and only allows using mbedtls which has a 10.6KiB bump in flash requirements, this is a flat out no and I will never ack that PR. So some compromise has to be made for
I commented in https://github.com/zephyrproject-rtos/zephyr/pull/71947#issuecomment-2080139050 These additional 10k is bringing much more than just sha256 (md5, sha512, sha1, dtls ....) . Disabling these things the difference is ~500 bytes (and consumes less ram)
the many devices in zephyr that have lower amounts of flash, I don't really see C code generating sha256 hashes as needing to be maintained - it's a simple enough feature, it's worked for years and it works fine now so I do not see the merit in forcing the removal without any sort of usable replacement. It generates a hash for a file, I really don't care less about PSA crypto for that, it doesn't interest me in the slightest, it needs to be a small code size, that's what is important (if someone wants to use PSA crypto for it instead, at their choice then that's fine, but the minimal size option should still exist and be the default). And not withstanding you would be silently breaking many devices of users and prevent them from evening being able to use zephyr 3.7, I can count 4 zephyr-based devices I developed in my home that would just completely break from this change due to insufficient flash (2xstm32f301, 1xnrf52832, 1xnrf52810)
We are not removing TinyCrypt for the sake of remove. TinyCrypt is no longer maintained, which means no security fixes. i understand that this is working but it we have a serious dependability issue in continue supporting this library. .
JordanYates
commented
4 months ago Not part of core Zephyr but it would be nice to have a PSA backend for the hostap repository:
https://github.com/zephyrproject-rtos/hostap/tree/main/src/crypto
dpkristensen
commented
4 months ago Hi all. Earlier I had proposed a different type of crypto API based on device tree that would support multiple instances (#56154) before I was aware of this effort. I think there are niche cases where multiple Crypto API instances could be desired (e.g., if someone wants to have hardware acceleration for a particular data stream and use software implementation for lower priority data), but I'd say it's probably fairly low priority unless someone has a real need.
Should I close my original proposal? It would create a secondary API surface, and may lead to additional maintenance effort.
tomi-font
commented
4 months ago Hi all. Earlier I had proposed a different type of crypto API based on device tree that would support multiple instances (https://github.com/zephyrproject-rtos/zephyr/issues/56154) before I was aware of this effort. I think there are niche cases where multiple Crypto API instances could be desired (e.g., if someone wants to have hardware acceleration for a particular data stream and use software implementation for lower priority data), but I'd say it's probably fairly low priority unless someone has a real need.
I am not very familiar with the PSA Crypto driver layer, but I think this is something that it allows. Different (e.g., software and hardware) implementations can be enabled and used. Maybe someone that knows more about this can chime in to give more details. I encourage you to have a detailed look at PSA Crypto and what it can allow.
Should I close my original proposal? It would create a secondary API surface, and may lead to additional maintenance effort.
I think so, unless it solves a specific problem that PSA Crypto cannot.
gilles-peskine-arm
commented
4 months ago Hi all. Earlier I had proposed a different type of crypto API based on device tree that would support multiple instances (#56154) before I was aware of this effort. I think there are niche cases where multiple Crypto API instances could be desired (e.g., if someone wants to have hardware acceleration for a particular data stream and use software implementation for lower priority data), but I'd say it's probably fairly low priority unless someone has a real need.
The PSA API (or more precisely its implementation in Mbed TLS) supports multiple instances. We intended that so that you can deploy the same images to multiple devices that may or may not have an accelerator, or so that you can benefit from an accelerator with partial support but fall back to software (e.g. you need AES-192 for compliance but your hardware only does AES-128 and AES-256). However, in those cases, it's up to the driver to decide, not up to the caller of the API.
It's also possible to set up drivers for opaque keys, i.e. where the software normally doesn't see the key material. With this functionality, you decide where the key is located when you create the key. You could use this functionality to have some keys' operations be accelerated and others not be accelerated, but it's awkward because you'd get no acceleration by default.
Distinguishing by priority is a use case I haven't heard of before. (Priority concerns, yet, but not to the extent that you'd want to not use an accelerator for low-priority data, just concerns about maintaining operation priority throughout the dispatch chain while not causing priority inversion.) Is that a theoretical concern or do you have a real-world use case that really needs to avoid clogging the accelerator with low-priority requests?
dpkristensen
commented
4 months ago Is that a theoretical concern or do you have a real-world use case that really needs to avoid clogging the accelerator with low-priority requests?
The example I was thinking of is DRM (Widevine, HDCP, etc...). In a typical high-performance MCU implementation where a trusted application is doing DRM operations, the TA may have access to a hardware accelerator that the application software does not (e.g., some Qualcomm MCUs) and its output region may be in an area of RAM that is not accessible at the user application layer.
In that case it's all built as separate images and has appropriate environment boundaries for secure vs non-secure and user vs kernel execution. The reservation of the accelerator for DRM content is desired/necessary to achieve the high throughput required for video playback. That particular crypto instance can't be used for non-DRM operations because the output region is not directly accessible (there's a separate physical bus from that region to the video encoder hardware).
A very small system might build everything as a single secure process, though; in such a case the accelerator could be used for other things, but it may still be undesirable to let it be so.
gilles-peskine-arm
commented
4 months ago Ok, thanks. For DRM video, I'd actually expect to bypass standard APIs and call some low-level platform-interface directly. But if you want to go through the PSA API then I think an opaque driver would make sense, since it essentially has its own key hierarchy.
dpkristensen
commented
4 months ago Ok so for this case, the PSA driver would have to know whether to use software or hardware acceleration, and that would have to be specified in psa_cipher_operation_s somewhere. The generic PSA API for struct psa_cipher_operation_s psa_cipher_operation_init(void) does not have any input parameters, so this would have to be assumed to be software and another specialized version of it would have to be for the hardware.
It seems to me that this sort of prioritization is outside the scope of PSA API. My proposal for the Crypto API would allow such things. But as you mentioned, there is likely a non-standard interface involved and the new Crypto API would just be "there" without any guarantee anyone will use it (kinda like the current implementation).
I think in the interest of keeping the code unified and not having to maintain multiple interfaces to the same thing, it's probably best to just have PSA be the main go-to implementation for generic crypto operations and have the crypto subsystem API removed. Special-purpose operations require special-purpose drivers.
Introduction
Zephyr has currently, at least, three different cryptography available and been used. They are:
The lack of an unified API to cryptography leads to a situation where we have different components using the one that best attend there needs and when an application needs those components we end up having multiple cryptography implementations in a single target. This means resources being waste.
To make things worse, one of these implementations (TinyCrypt) is no longer supported, which put us in a position of maintain it by ourselves or replace it with something else.
PSA Crypto implementation provides a portable interface to cryptographic operations on a wide range of hardware and software. Which means we may have Zephyr's using a single API that can have different implementations depending in build options. More information about it can be found in https://armmbed.github.io/mbed-crypto/html/overview/intro.html
Problem description
The lack of ONE API may cause waste of resources (multiple implementations needed at same time), make products more vulnerable (multiple implementations increases the code surface and consequently the chance of bugs), and it does help us take full advantage of HW accelerators.
Proposed change
The proposal is to adopt psa crypto API on Zephyr to replace the direct usage of other cryptography implementations.
Detailed RFC
The initial idea is:
Concerns and Unresolved Questions
Alternatives