Default random no trng #8742

netanelgonen · 2018-11-14T14:29:41Z

Adding default implementation to seed read/write from secure storage
to active this need to define the following macros:

MBEDTLS_PLATFORM_NV_SEED_READ_MACRO mbed_defualt_seed_read
MBEDTLS_PLATFORM_NV_SEED_WRITE_MACRO mbed_defualt_seed_write

depends on #8730

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

adbridge · 2018-11-14T14:35:10Z

@netanelgonen Could you please squash the commits in this PR to a more manageable size. Could you also please rebase and ensure that your commit messages comply with the guidelines in our documentation...

alekshex · 2018-11-14T14:52:21Z

Hi,
This PR is totally dependent on PR
#8730

Once PR#8730 is in the diff in changes will be minor

netanelgonen · 2018-11-14T14:55:21Z

This is still in internal review therefor I cannot squash this for now.
I'll squash this after internal approve

amiraloosh · 2018-11-15T12:45:35Z

@dannybenor , please review

Patater · 2018-11-15T15:09:00Z

features/mbedtls/platform/COMPONENT_PSA_SRV_IMPL/inc/default_random_seed.h

+
+
+#ifndef MBED_OS_RTOS_COMPONENT_PSA_SRV_IMPL_DEFAULTRANDOMSEED_H_
+#define MBED_OS_RTOS_COMPONENT_PSA_SRV_IMPL_DEFAULTRANDOMSEED_H_


This define name is really weird and doesn't match here the file is. Please revise. Perhaps "DEFAULT_RANDOM_SEED_H" would be fine. Look at other files for an example.

Patater · 2018-11-15T15:09:24Z

features/mbedtls/platform/COMPONENT_PSA_SRV_IMPL/inc/default_random_seed.h

+#endif
+
+#ifndef RANDOM_SEED_ITS_UID
+ #define MBED_RANDOM_SEED_ITS_UID 0xBAADA555


This doesn't look random. Non-random values have a higher chance of collisions...

Patater · 2018-11-15T15:10:39Z

features/mbedtls/platform/COMPONENT_PSA_SRV_IMPL/inc/default_random_seed.h

+extern "C" {
+#endif
+
+#ifndef RANDOM_SEED_ITS_UID


Should this be MBED_RANDOM_SEED_ITS_UID and not RANDOM_SEED_ITS_UID?

Patater · 2018-11-15T15:12:09Z

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

@@ -0,0 +1,19 @@
+


Style: Remove extra newline here

Patater · 2018-11-15T15:12:19Z

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

+}
+
+
+


Style: remove extra newlines here

Patater · 2018-11-15T15:12:36Z

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

+#include "default_random_seed.h"
+#include "psa_prot_internal_storage.h"
+
+int mbed_default_seed_read( unsigned char *buf, size_t buf_len )


Style: Use Mbed OS style consistently. We seem to be mixing Mbed TLS and Mbed OS style in this file.

Patater · 2018-11-15T15:12:59Z

features/mbedtls/platform/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(MBED_RANDOM_SEED_ITS_UID, 0, buf_len, buf);
+ return rc;


Simply returning rc isn't going to work when there is an error. PSA ITS errors are positive integers. The code checking for errors from mbedtls_nv_seed_read() interprets values less than zero as errors. This will mean that no entropy was provided (due to the error), but the system will think entropy was provided (because a non-negative value was returned): catastrophic for security!

Patater · 2018-11-15T15:13:13Z

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

+int mbed_default_seed_write( unsigned char *buf, size_t buf_len )
+{
+ psa_its_status_t rc = psa_its_set(MBED_RANDOM_SEED_ITS_UID, buf_len, buf, 0);
+ return rc;


Please check how errors are handled here as well.

Patater · 2018-11-15T15:27:11Z

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

@@ -6,14 +6,20 @@
 int mbed_default_seed_read( unsigned char *buf, size_t buf_len )
 {
 psa_its_status_t rc = psa_its_get(MBED_RANDOM_SEED_ITS_UID, 0, buf_len, buf);
- return rc;
+ /* Make sure that in case of an error the value will be negative
+ * mbedTLS errors are negative values */


Please use "Mbed TLS"

Patater · 2018-11-15T15:27:39Z

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

+ /* Make sure that in case of an error the value will be negative
+ * mbedTLS errors are negative values */
+ rc = rc < 0 ? rc : (-1 * rc);
+ return ( rc );


What if we return -rc?

This just in case that the psa_its_* will return a negative value

Patater · 2018-11-15T15:43:38Z

features/mbedtls/platform/COMPONENT_PSA_SRV_IMPL/inc/default_random_seed.h

@@ -8,7 +8,7 @@ extern "C" {
 #endif

 #ifndef RANDOM_SEED_ITS_UID
- #define MBED_RANDOM_SEED_ITS_UID 0x90210
+#define MBED_RANDOM_SEED_ITS_UID 0x90210


Could we get this define from the Mbed Crypto library instead?

This still doesn't look very random...

This feature does not enforce the use of Mbed Crypto,
There are 2 things the injection and the get/set the seed from secure storage

this definition is relevant for this file...

replacing number using this tool https://www.random.org/bytes/

If this feature does not require the use of Mbed Crypto, how does the initial random seed get populated? If there is some way other than Mbed Crypto, then we should not worry about making any PSA Entropy Injection API since we can just use that other way, right?

@Patater these 2 api callbacks currently are in no way related to any library except psa_its* secure storage, this is the only link. But psa-crypto is not part of the includes. let's not make a circular dependency where psa-crypto uses callbacks and callbacks include psa crypto

I'm suggesting we make Mbed Crypto the one source of truth for which storage slot contains the NV seed. Both have to agree. It's backwards to make Mbed Crypto depend on Mbed OS to determine this. Shouldn't be circular, but from Mbed Crypto picking the slot and Mbed OS using the same slot.

cmonr · 2018-11-15T18:14:52Z

@netanelgonen One more rebase, and this should be good.

netanelgonen · 2018-11-15T20:36:42Z

@Patater comment was treated

Patater · 2018-11-16T09:17:24Z

Looks alright. Awaiting rebase.

netanelgonen · 2018-11-18T15:57:44Z

rebased

Patater · 2018-11-19T11:42:58Z

Still too many commits in this PR. This depends on #8730 getting merged first, after which this PR needs another rebase (including commit cleanup to meet Mbed OS standards).

netanelgonen · 2018-11-19T14:24:23Z

@Patater squashed all my changes into a single commit

Patater · 2018-11-19T18:37:33Z

features/mbedtls/platform/COMPONENT_PSA_SRV_IMPL/inc/default_random_seed.h

@@ -0,0 +1,42 @@
+


Is this the final location we should place the file? @alzix is working on adding a new TARGET_PSA folder mechanism. I think we should place these files into a TARGET_PSA folder (perhaps features/mbedtls/platform/TARGET_PSA). Sorry for the late design change, but TARGET_PSA is a late design change that affects this PR.

Ensure you are aligned with #8723 so we don't have too many disparate folders for what should all be the same folder.

alzix · 2018-11-20T08:33:09Z

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

+ psa_its_get_info(MBED_RANDOM_SEED_ITS_UID, &info);
+ if (info.size < buf_len)
+ {
+ actual_size = info.size;


Please note that mbed_default_seed_read(unsigned char *buf, size_t buf_len) is not able to tell anyone it has read less then required as buf_len is an IN argument only.
Thus reading less then requested is not a solution.
Consider calling panic as it looks like missconfiguration issue. System integrator MUST know what seed size is needed for his system, and must inject appropriate size.
By gracefully treating this error we may introduce security vulnerabilities, when error code will not be treated properly.

Patater · 2018-11-20T09:03:30Z

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

@@ -5,7 +5,16 @@

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


Will mbed_default_seed_read() ever be called multiple times? Perhaps mbedtls will poll, asking for 32-byte chunks at a time. Do we need to handle that? If so, we should make sure we don't re-use bits of injected entropy for each call to mbed_default_seed_read().

Alternatively, we can set MBEDTLS_ENTROPY_BLOCK_SIZE to exactly how much will be injected and exactly how much entropy Mbed TLS will ask for (must be same number), and can return an error if the mbed_default_seed_read() is called more than once.

I believe I prefer the second approach (easier to test and verify).

What are your thoughts in this area, @gilles-peskine-arm ?

Context: what is this function supposed to do?

- add SPDX license identifier - add missing license headers - update year in license headers

* Modify linker scripts to be compatible with bootloader and PSA * Add memory protection * Modify original post-build step to allow link with PSA binaries * Config kvstore for ITS on FUTURE_SEQUANA_PSA

Implement the following: KVStore base class TDBStore class FileSystemStore class SecureStore class Global APIs Configuration framework Design documentation

* Modify linker scripts to be compatible with bootloader and PSA * Add memory protection * Modify original post-build step to allow link with PSA binaries * Config kvstore for ITS on FUTURE_SEQUANA_PSA

This is a workaround supplied by Cypress The issue will be fixed correctly with an updated PDL

Bug description - Sequential flash operations fails

If setting the MBEDTLS_PLATFORM_NV_SEED_ALT and MBEDTLS_ENTROPY_NV_SEED flags and not setting MBEDTLS_PLATFORM_NV_SEED_WRITE_MACRO and MBEDTLS_PLATFORM_NV_SEED_READ_MACRO flags mbed-os will add an entropy source to the relevent partition - SPE in case of daul core or in case of single core V7 to the main partition. The defualt behaviour will be to read or write the data from the ITS.

alekshex · 2018-11-21T15:59:24Z

internal review approved. rebase done

cmonr · 2018-11-21T16:58:41Z

@netanelgonen @avolinski Please take a look at the travis failures.

alekshex · 2018-11-21T17:41:58Z

Adding default implementation to seed read/write from secure storage
to active this need to define the following macros:

MBEDTLS_PLATFORM_NV_SEED_READ_MACRO mbed_defualt_seed_read
MBEDTLS_PLATFORM_NV_SEED_WRITE_MACRO mbed_defualt_seed_write

depends on #8730
[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

This PR also depends on the upcoming mbedTLS PR with mbed crypto, since it includes "crypto.h"

Patater

OK

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

netanelgonen · 2018-11-22T15:37:38Z

closed and replaced by #8804

cmonr requested a review from bulislaw November 14, 2018 14:33

cmonr added the needs: review label Nov 14, 2018

netanelgonen closed this Nov 14, 2018

studavekar removed the needs: review label Nov 14, 2018

cmonr removed the request for review from bulislaw November 14, 2018 14:35

netanelgonen reopened this Nov 14, 2018

NirSonnenschein added do not merge needs: work labels Nov 14, 2018

Patater suggested changes Nov 15, 2018

View reviewed changes

Patater reviewed Nov 15, 2018

View reviewed changes

NirSonnenschein added release-version: 5.11.0-rc1 risk: A labels Nov 18, 2018

netanelgonen force-pushed the default-random-no-trng branch 2 times, most recently from beac936 to 2dbcd3a Compare November 18, 2018 15:56

netanelgonen force-pushed the default-random-no-trng branch from 2dbcd3a to 65f2456 Compare November 19, 2018 14:23

mohammad1603 mentioned this pull request Nov 19, 2018

PSA Crypto SPM #8804

Merged

Patater suggested changes Nov 19, 2018

View reviewed changes

alzix reviewed Nov 20, 2018

View reviewed changes

Patater reviewed Nov 20, 2018

View reviewed changes

theotherjimmy and others added 19 commits November 21, 2018 17:54

Include deliver_into in Config mock

71e3540

astyle changes

80f0e68

Normalize license headers

3a55eaf

- add SPDX license identifier - add missing license headers - update year in license headers

Add FUTURE_SEQUANA_PSA target

180cec1

* Modify linker scripts to be compatible with bootloader and PSA * Add memory protection * Modify original post-build step to allow link with PSA binaries * Config kvstore for ITS on FUTURE_SEQUANA_PSA

Move shared memory to the end of the RAM

a8bc1f1

Reserve space for secure storage

4478e9e

Add PSA-crypto partition & IPC code

de14995

Enable PSA-Crypto on PSoC6

1057694

Update the importer to pull Jaeden's branch of mbed crypto

584204b

Update the importer to pull two repos

8ba9e81

KVStore & derived classes: design docs, implementation & configuration

9785151

Implement the following: KVStore base class TDBStore class FileSystemStore class SecureStore class Global APIs Configuration framework Design documentation

Add FUTURE_SEQUANA_PSA target

a72df8e

* Modify linker scripts to be compatible with bootloader and PSA * Add memory protection * Modify original post-build step to allow link with PSA binaries * Config kvstore for ITS on FUTURE_SEQUANA_PSA

Workaround for cypress memory protection

2613c1d

This is a workaround supplied by Cypress The issue will be fixed correctly with an updated PDL

Disable memory protection due to bug in flash

3be6499

Bug description - Sequential flash operations fails

move default random seed functions into TARGET_PSA

46f4c53

update random seed uid define name

07c4da2

update target.json

6e87bca

Change naming of MBEDTLS_RANDOM_SEED_ITS

1479dd9

alekshex force-pushed the default-random-no-trng branch from a78666d to 1479dd9 Compare November 21, 2018 15:57

alekshex approved these changes Nov 21, 2018

View reviewed changes

cmonr added the needs: preceding PR label Nov 22, 2018

Patater approved these changes Nov 22, 2018

View reviewed changes

netanelgonen closed this Nov 22, 2018

studavekar removed do not merge needs: work labels Nov 22, 2018



		#ifndef MBED_OS_RTOS_COMPONENT_PSA_SRV_IMPL_DEFAULTRANDOMSEED_H_
		#define MBED_OS_RTOS_COMPONENT_PSA_SRV_IMPL_DEFAULTRANDOMSEED_H_

Default random no trng #8742

Default random no trng #8742

Conversation

netanelgonen commented Nov 14, 2018

adbridge commented Nov 14, 2018

alekshex commented Nov 14, 2018

netanelgonen commented Nov 14, 2018

amiraloosh commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmonr commented Nov 15, 2018

netanelgonen commented Nov 15, 2018

Patater commented Nov 16, 2018

netanelgonen commented Nov 18, 2018

Patater commented Nov 19, 2018

netanelgonen commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekshex commented Nov 21, 2018

cmonr commented Nov 21, 2018

alekshex commented Nov 21, 2018

Patater left a comment

Choose a reason for hiding this comment

netanelgonen commented Nov 22, 2018

Patater Nov 20, 2018 •

edited

Loading