PSA Crypto SPM #8804

mohammad1603 · 2018-11-19T17:50:28Z

Description

Integrate PSA Crypto code SPM integration code with mbed-os
This PR has few dependencies:

It also contains customized mbedtls importer, this branch needs to be rebased over mbed-os master branch once #8723 is merged, please ignore the following commits:

It also waits for the new version of mbedtls which contains mbed-crypto as a submodule (relevant PR exist in mbedtls repo).

Commits included in existent PRs (no need to review in this PR):

Commit	Parent PR
`304e0ca`	#8744 based on #8667
`a665743`	#8730
`34eef5f`	#8730
`f625181`	#8730
`e5ae50a`	#8730
`6c7c68c`	#8744
`4f3c050`	#8744
`331c0d5`	#8744
`19a5091`	#8744
`ad32209`	#8745
`f64a5fc`	#8745
`6822ba9`	#8745

Commits that MUST be reviewed as part of this PR are:

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[X]  Test update
[ ] Breaking change

NirSonnenschein · 2018-11-19T18:25:31Z

@mohammad1603 , please clearly indicate which commits belong to this PR (so that only relevant commits can be reviewed). This will also need a rebase.

Patater · 2018-11-19T19:12:43Z

@mohammad1603 Shouldn't the entropy injection IPC implementation get added together with other PSA Crypto IPC additions? I believe that @alzix already has a PR to add PSA Crypto IPC implementation that these change should go into. In which case, we can close this PR in favor of that other one.

mohammad1603 · 2018-11-20T08:40:06Z

@Patater this branch is forked from @alzix branch you mentioned, once they raise their PR and it merged, the changed files will be minimal and only related to the entropy injection

netanelgonen · 2018-11-20T09:37:30Z

@netanelgonen CC to future me

Patater · 2018-11-20T10:39:23Z

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c

-    {
-        SPM_PANIC("Failed to initiate Crypto partition!!");
-    }
-
    while (1) {
        signals = psa_wait_any( PSA_BLOCK );


If the library hasn't been initialized yet, only the entropy injection call should be allowed through. If the library has had entropy injected, initialization should be allowed, but not entropy injection anymore. If the library has been initialized, all other calls can be allowed.

I think it's ok, the generic code will do the right thing anyway. (Not that I'd object to an additional layer of safety.) Or do you see a case where an additional check is needed here?

alzix

I do not think we should enable the callbacks for cm4 core.
Cm4 mbedtls should call to PSA crypto random API thus it does not need its own seed and callbacks implementation

alekshex · 2018-11-21T08:37:28Z

@alzix yes, but without callbacks it won't compile by default.
since it doesn't have trng nor callbacks

Patater

OK

Will need me to look at again after rebase on top of merged preceeding PR

alekshex · 2018-11-22T17:12:49Z

TESTS/psa/entropy_inject/main.cpp

+#include "entropy_poll.h"
+#include "crypto.h"
+
+#if (!defined(TARGET_PSA) || (!defined(COMPONENT_PSA_SRV_IPC) && !defined(MBEDTLS_ENTROPY_NV_SEED)))


Suggested change

#if (!defined(TARGET_PSA) || (!defined(COMPONENT_PSA_SRV_IPC) && !defined(MBEDTLS_ENTROPY_NV_SEED)))

#if ((!defined(TARGET_PSA) || !defined(COMPONENT_PSA_SRV_IPC)) && (!defined(TARGET_PSA) || !defined(MBEDTLS_ENTROPY_NV_SEED)))

bulislaw · 2018-11-22T19:20:35Z

@ARMmbed/mbed-os-maintainers I believe this one needs work not review

gilles-peskine-arm

I have classified my comments as follows:

Critical: breaks something other than the use of PSA crypto APIs, or badly breaks the use of PSA crypto APIs by well-meaning, bug-free clients.
Major: breaks the use of the PSA crypto APIs by well-meaning clients but in a recoverable way, or breaks the security or stability of the system if a client misuses the API. Note that if the PSA crypto SPM integration was more than a demo, each of these security and stability issues would be blockers. Since we're only releasing them as a demo, that's ok, but this must be very clearly documented.
Minor: the more are fixed the better, but we can release with these issues.
For later: things I noticed while reading the code that can't be fixed without doing some work in Mbed Crypto.

The marshalling code and especially the server-side validation code are fundamentally insecure and incomplete: the server trusts its clients, and it does not separate clients' assets. This cannot be fixed without extensive rework. Therefore we must document the following limitations:

The use of a cryptography service with the SPM is provided as a functional demonstration only, with no expectation of added security compared to running it as a library in Mbed OS.
All the users of PSA crypto APIs on the device share a single namespace for key slots. Therefore, when integrating a platform, you must make sure that each piece of the system (PRoT parts, any additional secure service, and the Mbed OS application) use disjoint sets of slot numbers.

Because the server unmarshalling code is fundamentally insecure, I have not reviewed the marshalling and unmarshalling code in detail for potential issues such as insufficient parameter validation, buffer overflows, etc.

I'm not very familiar with Mbed OS so I only reviewed from the Mbed Crypto point of view. I haven't reviewed Mbed OS integration aspects or tried to make a build. I think that the use of the PSA crypto API on non-SPM builds is broken and that some non-default configurations of mbedtls are broken, but I'm not completely sure.

gilles-peskine-arm · 2018-11-23T12:13:35Z

components/TARGET_PSA/services/crypto/psa_psa_f_ifs.h

+
+/***********************************************************************************************************************
+ * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ * THIS FILE IS AN AUTO-GENERATED FILE - DO NOT MODIFY IT.


Critical: How was this generated? Please explain in the commit message that introduces it.

This applies to all the auto-generated files in this PR.

@gilles-peskine-arm we'll add an explanation. but lack of explanation is not classified as a critical issue

@gilles-peskine-arm,
this is a great input! Fixed b6c86e6 here.
once rebased - execute $python tools/spm/generate_partition_code.py to regenerate these files

@avolinski It's a critical issue because this code is going into production and we need to know how to re-generate it. We can't add the explanation after shipping because then there would be no way to go from the shipped version to the explanation. @alzix Thanks!

tools/spm/generate_partition_code.py doesn't take parameters and runs on the whole repository, right? Then everything is set to keep track of this information, and this issue is resolved.

gilles-peskine-arm · 2018-11-23T12:16:11Z

components/TARGET_PSA/services/crypto/crypto_partition_psa.json

@@ -0,0 +1,108 @@
+{
+  "name": "PSA_F",


Major: Why is the crypto partition called PSA_F? If you really need to use such a non-intuitive name, please justify it in a comment.

will find a better name for next release.

will find a better name for next release.

@ARMmbed/mbed-os-maintainers Not sure how strict we need to be from a backwards-compatability PoV.

gilles-peskine-arm · 2018-11-23T12:28:55Z

components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/crypto_platform_spe.h

@@ -0,0 +1,174 @@
+/**
+ * \file psa/crypto_platform_spe.h


Critical: I know this file has existed for months and I highly doubt that it was written in a single commit. If this file was developed in Mbed OS, please make sure to preserve its history when committing it to the main branch. If this file was developed outside Mbed OS, please point back to wherever you're importing it from in the commit message, make sure to indicate exactly what version you're importing, and give any information you can to trace its development and review history.

This applies to crypto_spe.h, psa_crypto_struct.h, psa_crypto_struct_ipc.h, psa_crypto_spm.c, psa_crypto_partition.c, and any other file with a similar history.

Also, please avoid combining the import of external files and the creation of new content in the same commit. In a commit that imports external files, do only the strict minimum that's needed to meet the project's per-commit rules (I don't know what the rules are for Mbed OS).

Hi @gilles-peskine-arm since the files existed outside of mbedOS we will continue with flat history here

It's unfortunate that the history of the code doesn't appear in Mbed OS, but I'm aware of this limitation. However the commit that imports external code must give a precise reference of what it's importing.

Resolved in the amended commit "Create a new partition for the crypto service"

gilles-peskine-arm · 2018-11-23T12:34:44Z

components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/crypto_platform_spe.h

+typedef enum psa_sec_function_s
+{
+    PSA_CRYPTO_INVALID,
+    PSA_CRYPTO_INIT,


For later: I think this list should be auto-generated, but that requires some refactoring in PSA Crypto first, to have a systematic way of detecting the points where IPC happens. It may also be possible to auto-generate the IPC argument structures but this requires further investigation.

gilles-peskine-arm · 2018-11-23T12:43:16Z

components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/crypto_platform_spe.h

+ */
+
+// Max length supported for nonce is 16 bytes.
+#define PSA_MAX_NONCE_SIZE 16


Minor: “Nonce” has a more general meaning. This should be called PSA_AEAD_MAX_NONCE_SIZE.

Actually, the PSA crypto API should provide a suitable macro, but it currently doesn't. Tracked in https://github.com/ARMmbed/psa-crypto/issues/56 (private).

gilles-peskine-arm · 2018-11-23T16:19:38Z

features/mbedtls/importer/Makefile

@@ -29,18 +29,32 @@
 # Set the mbed TLS release to import (this can/should be edited before import)
 MBED_TLS_RELEASE ?= mbedtls-2.13.1


Remember to update this when 2.15.0 is released. And the mbed crypto release as well.

features/mbedtls/importer/Makefile

gilles-peskine-arm · 2018-11-23T16:59:00Z

TESTS/psa/entropy_inject/its_reset/COMPONENT_SPE/test_pits_impl.cpp

+    }
+
+    KVMap &kv_map = KVMap::get_instance();
+    KVStore *kvstore = kv_map.get_main_kv_instance(STR_EXPAND(MBED_CONF_STORAGE_DEFAULT_KV));


I don't understand why SPE code even has access to the Mbed OS KVstore API. Shouldn't this call psa_its_remote? And for that matter, why not use psa_its_remove when there's no SPM as well?

This is because the test is running in a different partition then the actual its_set is called through the entropy_injection API.
this is an onboard testing and this test should work for all targets.

But how can this work on targets where Mbed OS cannot access the ITS storage? Or is this just a temporary shortcut that's acceptable because there are no such targets in 5.11?

as current design entropy inject will work IFF there is ITS.

gilles-peskine-arm · 2018-11-23T17:00:34Z

TESTS/psa/entropy_inject/main.cpp

@@ -0,0 +1,153 @@
+/*


Critical: the commits that import-and-adapt entropy injection tests from work in progress then the final version of the unit tests in Mbed Crypto should reference the precise version that they are importing.

Hi @gilles-peskine-arm the APIs should not change. so the tests remains relevant.
and api changes can always happen regardless of this fact, then the old ones become deprecated and new ones introduced

Because this is only test code, I'm willing to downgrade this to “major”. But in the future, please trace the history of code the way medical implants should be traced, not the way medical implants are traced.

gilles-peskine-arm · 2018-11-23T17:01:52Z

features/mbedtls/platform/TARGET_PSA/COMPONENT_PSA_SRV_IMPL/src/default_random_seed.cpp

+int mbed_default_seed_read(unsigned char *buf, size_t buf_len)
+{
+    psa_its_status_t rc = psa_its_get(PSA_CRYPTO_ITS_RANDOM_SEED_UID, 0, buf_len, buf);
+    return ( -1 * rc );


Minor: - rc would do as well, and a comment to explain why (which existed in a previous commit: ITS errors are positive but mbedos errors are negative) would be useful.

@gilles-peskine-arm Fixed

alekshex

Good to go for 5.11 content scope

bulislaw · 2018-11-26T14:46:56Z

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

alekshex · 2018-11-27T11:12:10Z

Hi @bulislaw ,The PR is ready from our perspective. If CI will pass it is good to go in.

0xc0170 · 2018-11-27T11:47:37Z

CI job stopped, waiting for 8744

Patater

NAK

Patater · 2018-11-27T12:51:28Z

targets/targets.json

@@ -1439,7 +1439,7 @@
            "PSA"
        ],
        "is_disk_virtual": true,
-        "macros": ["CPU_MK64FN1M0VMD12", "FSL_RTOS_MBED"],
+        "macros": ["CPU_MK64FN1M0VMD12", "FSL_RTOS_MBED", "MBEDTLS_PSA_CRYPTO_C"],


This would force all K64F users to enable PSA Crypto.

We don't want to turn on PSA for everybody's K64F. Memory usage and code size may be higher, for instance. If someone's application barely fit before, it won't fit if we force them to use PSA. Unfortunately, it looks like #8730 enables PSA by default on K64F. This is likely unintentional or a mistake.

PSA should be opt-in on K64F and come from the user's application (mbed_app.json), which means MBEDTLS_PSA_CRYPTO_C should not get turned on for a user unless they explicitly request it via their application's configuration.

I've verified code size and static RAM usage both with and without PSA for blinky. LTO seems to be working, in that what the app doesn't use won't bloat the image.

With PSA and PSA Crypto

Total Static RAM memory (data + bss): 12672(+12672) bytes Total Flash memory (text + data): 61565(+61565) bytes Image: ./BUILD/K64F/GCC_ARM/mbed-os-example-blinky.bin

Without PSA and without PSA Crypto

Total Static RAM memory (data + bss): 12672(+12672) bytes Total Flash memory (text + data): 61565(+61565) bytes Image: ./BUILD/K64F/GCC_ARM/mbed-os-example-blinky.bin

So, I'm OK with this being available by default from a technical perspective.

Patater · 2018-11-27T12:53:23Z

TESTS/psa/crypto_init/main.cpp

 #endif // TARGET_PSA

 #include "greentea-client/test_env.h"
 #include "unity/unity.h"
 #include "utest/utest.h"
 #include "crypto.h"
+#include "crypto_extra.h"


crypto_extra should not need to be included explicitly by tests or user applications. It's already included by crypto.h.

Patater · 2018-11-27T12:53:53Z

TESTS/psa/crypto_init/main.cpp

-#if ((!defined(TARGET_PSA)) || (!defined(MBEDTLS_PSA_CRYPTO_C)))
-#error [NOT_SUPPORTED] Mbed Crypto is OFF - skipping.
+#if ((!defined(TARGET_PSA)) || (!defined(MBEDTLS_PSA_CRYPTO_C)) || (!defined(MBEDTLS_PSA_CRYPTO_SPM )))
+#error [NOT_SUPPORTED] Mbed SPM Crypto is OFF - skipping.


Why can't we run tests without SPM enabled? Mbed Crypto should work fine without SPM.

There is a different behavior to init with SPM
without you can call init as much as you want and the free releases the context but with SPM in a client-server mode you can have more than 1 client, therefore we keep track on how many time you init and how many times you free.
this test is checking this feature

OK, this is just for the crypto init test.

Patater

The concerns I've raised have been addressed.

0xc0170 · 2018-11-27T14:27:53Z

#8744 was integrated. Can this be rebased now?

NirSonnenschein · 2018-11-27T14:37:13Z

@0xc0170, it doesn't seem to be required. We are pre-rebased on the 9744 branch and a local check showed a re-base does nothing. so we can proceed

cmonr · 2018-11-27T15:54:21Z

Re-review of comments looks good. Just have a follow-up question here, but not blocking: 32bc912#commitcomment-31470301

mbed-ci · 2018-11-27T17:12:12Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 5
Build artifacts
Build logs

alekshex · 2018-11-27T17:15:09Z

Hi @0xc0170 , @cmonr , @NirSonnenschein
All tests passed, but the exporter seemed to have a CI issue : Pipe is already closed
Can you please rerun the exporter only, so we'll have a true green CI on this urgent PR.
Thank you!

NirSonnenschein · 2018-11-27T17:57:35Z

@avolinski trying to re-run exporters , looks like a CI issue

alekshex · 2018-11-27T20:09:06Z

Hi @cmonr @0xc0170 @NirSonnenschein , the CI is green now.
We are good to go :)

NirSonnenschein added needs: review release-version: 5.11.0-rc1 labels Nov 19, 2018

0xc0170 added the needs: preceding PR label Nov 20, 2018

Patater reviewed Nov 20, 2018

View reviewed changes

netanelgonen force-pushed the inject_entropy_spm branch from 6e188ec to 1f29b99 Compare November 20, 2018 13:26

alzix suggested changes Nov 20, 2018

View reviewed changes

alzix approved these changes Nov 21, 2018

View reviewed changes

mohammad1603 changed the title ~~Inject entropy spm~~ PSA Crypto SPM Nov 21, 2018

mohammad1603 force-pushed the inject_entropy_spm branch 2 times, most recently from 0cc4403 to 4b58b55 Compare November 22, 2018 11:01

Patater approved these changes Nov 22, 2018

View reviewed changes

netanelgonen mentioned this pull request Nov 22, 2018

Default random no trng #8742

Closed

netanelgonen force-pushed the inject_entropy_spm branch 2 times, most recently from 29cea6a to 6765a04 Compare November 22, 2018 16:30

alekshex reviewed Nov 22, 2018

View reviewed changes

netanelgonen force-pushed the inject_entropy_spm branch from 6765a04 to fb40d2d Compare November 22, 2018 17:27

gilles-peskine-arm suggested changes Nov 23, 2018

View reviewed changes

alekshex approved these changes Nov 26, 2018

View reviewed changes

mohammad1603 force-pushed the inject_entropy_spm branch from 775186e to 15ee0db Compare November 26, 2018 15:35

0xc0170 added the priority: high label Nov 26, 2018

mohammad1603 force-pushed the inject_entropy_spm branch 2 times, most recently from 00dab25 to 4c03a0e Compare November 26, 2018 16:49

cmonr requested a review from a team November 26, 2018 17:16

add MBEDTLS_PSA_CRYPTO_C defined to K64F PSA target

f1c5aeb

netanelgonen force-pushed the inject_entropy_spm branch from 0f17ec9 to 843783a Compare November 27, 2018 12:46

Patater suggested changes Nov 27, 2018

View reviewed changes

Patater reviewed Nov 27, 2018

View reviewed changes

do not run crypto init SPM test if not SPM target

5f36447

netanelgonen force-pushed the inject_entropy_spm branch from 843783a to 5f36447 Compare November 27, 2018 13:29

Patater approved these changes Nov 27, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: preceding PR needs: review labels Nov 27, 2018

0xc0170 added needs: CI and removed needs: work labels Nov 27, 2018

cmonr added risk: A and removed risk: R labels Nov 27, 2018

NirSonnenschein added ready for merge and removed needs: CI labels Nov 27, 2018

cmonr added risk: G and removed risk: A labels Nov 27, 2018

0xc0170 merged commit e62abd8 into ARMmbed:master Nov 28, 2018

0xc0170 removed the ready for merge label Nov 28, 2018

	#if (!defined(TARGET_PSA) \|\| (!defined(COMPONENT_PSA_SRV_IPC) && !defined(MBEDTLS_ENTROPY_NV_SEED)))
	#if ((!defined(TARGET_PSA) \|\| !defined(COMPONENT_PSA_SRV_IPC)) && (!defined(TARGET_PSA) \|\| !defined(MBEDTLS_ENTROPY_NV_SEED)))

		@@ -29,18 +29,32 @@
		# Set the mbed TLS release to import (this can/should be edited before import)
		MBED_TLS_RELEASE ?= mbedtls-2.13.1

PSA Crypto SPM #8804

PSA Crypto SPM #8804

Uh oh!

Conversation

mohammad1603 commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

NirSonnenschein commented Nov 19, 2018

Uh oh!

Patater commented Nov 19, 2018

Uh oh!

mohammad1603 commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netanelgonen commented Nov 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alzix left a comment

Choose a reason for hiding this comment

Uh oh!

alekshex commented Nov 21, 2018

Uh oh!

Patater left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Nov 22, 2018

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alekshex Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

mohammad1603 commented Nov 19, 2018 •

edited

Loading

mohammad1603 commented Nov 20, 2018 •

edited

Loading

Patater left a comment •

edited

Loading

alekshex Nov 26, 2018 •

edited

Loading

bulislaw commented Nov 26, 2018 •

edited

Loading