sys: Add PSA Crypto module #18578

Einhornhool · 2022-09-12T09:31:00Z

Contribution description

This adds an implementation of the ARM PSA Crypto API specification to RIOT.

It is a cryptographic API that supports software and hardware backends as well as the use of multiple secure elements, which can be configured with Kconfig.
It integrates indirect, identifier based key management to support persistent storage of key material in local memory and devices with protected key storage.

A description of the implementation design and an evaluation of the processing time and memory overhead in RIOT has been published here: Usable Security for an IoT OS: Integrating the Zoo of Embedded Crypto Components Below a Common API
Implementation status

So far this implementation supports the following operations:

Volatile key storage
AES in CBC mode
Hashes (MD5, SHA1, SHA224, SHA256)
HMAC SHA256
ECDSA with NIST P192 and P256 curves

Issues/PRs references

Split up PR #18547

Related:
#18583
#18582
#18581
#18580
#18579

aabadie · 2022-09-12T10:10:56Z

Why not keep the example application from #18547 ? It was providing a way for Murdock to build (and test) the code of this PR.

Einhornhool · 2022-09-12T11:21:34Z

I removed it, because this branch only provides the PSA Module without backend support. I submitted the code for the wrappers for the RIOT cipher module, etc, in separate PRs, to reduce the size of the PR, but this will mean that the example won't build.

Einhornhool · 2022-09-13T14: 8000 24:34Z

Why not keep the example application from #18547 ? It was providing a way for Murdock to build (and test) the code of this PR.

I added a minimal example application that works on this branch. It only executes key management operations and random number generation. It can be extended by more operations when other backends are included.

kfessel

the static tests found some empty lines and some long lines https://github.com/RIOT-OS/RIOT/actions/runs/3045866373 please check if they are fixable

Ollrogge · 2022-09-16T14:12:28Z

sys/psa_crypto/psa_key_slot_mgmt/psa_crypto_slot_management.c

+        psa_key_slot_number_t slot_number;
+#if IS_ACTIVE(CONFIG_PSA_SECURE_ELEMENT_ASYMMETRIC)
+        uint8_t pubkey_data[PSA_EXPORT_PUBLIC_KEY_MAX_SIZE];
+        size_t pubkey_bytes;


Any reason you use pubkey_bytes here to refer to the size of the buffer ? This is kind of confusing to me since I relate bytes to being data. pubkey_data_len or pubkey_data_length would make more sense in my opinion.

I agree, I changed the names

Ollrogge · 2022-09-16T14:26:44Z

sys/psa_crypto/psa_crypto.c

+{
+    lib_initialized = 1;
+
+#if IS_ACTIVE(CONFIG_MODULE_PSA_KEY_SLOT_MGMT)


I learned that one should use if (IS_ACTIVE(...)) wherever possible because this will cause the code to be compiled normally even if the specific FLAGS are not enabled and therefore enable syntax checks. If the code chunks are not used they will be optimized out afterwards.

No idea how much work changing this would be but you should keep this in mind. Especially looking at e.g. psa_crypto_location_dispatch this could increase code coverage by a lot.

Thanks for telling me, I didn't know. I changed this on this branch.

Ollrogge · 2022-09-16T14:42:43Z

sys/psa_crypto/psa_crypto.c

+    }
+
+    operation->iv_set = 1;
+    *iv_length = operation->default_iv_length;


Shouldn't this be *iv_length = iv_size ?

Yup, my bad. Fixed.

Ollrogge · 2022-09-16T15:34:21Z

sys/psa_crypto/psa_crypto.c

+
+    uint8_t actual_hash_length = PSA_HASH_LENGTH(operation->alg);
+
+    if (hash_size < actual_hash_length) {


Isn't this check a little late ? I would think that for e.g. psa_hash_compute checking if the output buffer is big enough should happen before any computation is done

We actually need the check in psa_hash_finish, in case it is called outside of psa_hash_compute.
I do agree that it would be good to abort before computing anything, but checking in both places would result in redundant code. I'm not sure what's preferable here.

MrKevinWeiss · 2022-10-04T09:16:01Z

examples/psa_crypto/tests/01-run.py

@@ -0,0 +1,13 @@
+#!/usr/bin/env python3


please change the mode

sudo chmod 0775 examples/psa_crypto/tests/01-run.py

MrKevinWeiss

I was able to get the make build and test (vs kconfig).

There is some cleanup needed in the kconfig model. Normally we don't have configuration values select modules.

diff --git a/examples/psa_crypto/Makefile b/examples/psa_crypto/Makefile
diff --git a/examples/psa_crypto/Makefile b/examples/psa_crypto/Makefile
index 7c5d438aab..791374bf9b 100644
--- a/examples/psa_crypto/Makefile
+++ b/examples/psa_crypto/Makefile
@@ -1,10 +1,12 @@
 # This has to be the absolute path to the RIOT base directory:
 RIOTBASE ?= $(CURDIR)/../..
 
+USEMODULE += psa_crypto
+USEMODULE += random
+USEMODULE += psa_key_slot_mgmt
+
 APPLICATION = example_psa_crypto
 
 BOARD ?= native
 
-export TEST_KCONFIG ?= 1
-
 include $(RIOTBASE)/Makefile.include
diff --git a/sys/psa_crypto/Kconfig b/sys/psa_crypto/Kconfig
index 4bdda0b10e..e3332e85fc 100644
--- a/sys/psa_crypto/Kconfig
+++ b/sys/psa_crypto/Kconfig
@@ -13,12 +13,13 @@ menuconfig MODULE_PSA_CRYPTO
 if MODULE_PSA_CRYPTO
 
 rsource "Kconfig.asymmetric"
-rsource "Kconfig.ciphers"
 rsource "Kconfig.hashes"
 rsource "Kconfig.mac"
-rsource "Kconfig.keys"
 
 rsource "psa_se_mgmt/Kconfig"
 rsource "psa_key_slot_mgmt/Kconfig"
 
 endif # MODULE_PSA_CRYPTO
+
+rsource "Kconfig.ciphers"
+rsource "Kconfig.keys"
diff --git a/sys/psa_crypto/Kconfig.ciphers b/sys/psa_crypto/Kconfig.ciphers
index 7cb282ff77..aac8d5570f 100644
--- a/sys/psa_crypto/Kconfig.ciphers
+++ b/sys/psa_crypto/Kconfig.ciphers
@@ -8,7 +8,7 @@
 menuconfig PSA_CIPHERS
     bool "PSA Cipher"
     select PSA_KEY_CONFIG
-    select MODULE_PSA_KEY_SLOT_MGMT
+    select MODULE_PSA_KEY_SLOT_MGMT if TEST_KCONFIG
 
 if PSA_CIPHERS
 
@@ -27,9 +27,9 @@ choice CIPHER_AES_128_ECB_IMPLEMENTATION
 
 config RIOT_CIPHER_AES_128_ECB
     bool "RIOT Cipher Module"
-    select MODULE_CRYPTO
-    select MODULE_CRYPTO_AES_128
-    select MODULE_PSA_RIOT_CIPHER_AES_ECB
+    select MODULE_CRYPTO if TEST_KCONFIG
+    select MODULE_CRYPTO_AES_128 if TEST_KCONFIG
+    select MODULE_PSA_RIOT_CIPHER_AES_ECB if TEST_KCONFIG
 
 endchoice
 
@@ -51,7 +51,7 @@ choice CIPHER_AES_128_CBC_IMPLEMENTATION
 
 config RIOT_CIPHER_AES_128_CBC
     bool "RIOT Cipher Module"
-    select MODULE_PSA_RIOT_CIPHER_AES_128_CBC
+    select MODULE_PSA_RIOT_CIPHER_AES_128_CBC if TEST_KCONFIG
 
 config HARDWARE_CIPHER_AES_128_CBC
     bool "Hardware Accelerated"
@@ -61,7 +61,7 @@ config HARDWARE_CIPHER_AES_128_CBC
 config PSA_TINYCRYPT_CIPHER_AES_128_CBC
     bool "TinyCrypt Library"
     select PACKAGE_TINYCRYPT
-    select MODULE_PSA_TINYCRYPT_CIPHER_AES_CBC
+    select MODULE_PSA_TINYCRYPT_CIPHER_AES_CBC if TEST_KCONFIG
 
 endchoice
 
@@ -82,9 +82,9 @@ choice CIPHER_AES_192_CBC_IMPLEMENTATION
 
 config RIOT_CIPHER_AES_192_CBC
     bool "RIOT Cipher Module"
-    select MODULE_CRYPTO
-    select MODULE_CRYPTO_AES_192
-    select MODULE_PSA_RIOT_CIPHER_AES_CBC
+    select MODULE_CRYPTO if TEST_KCONFIG
+    select MODULE_CRYPTO_AES_192 if TEST_KCONFIG
+    select MODULE_PSA_RIOT_CIPHER_AES_CBC if TEST_KCONFIG
 
 endchoice
 
@@ -105,9 +105,9 @@ choice CIPHER_AES_256_CBC_IMPLEMENTATION
 
 config RIOT_CIPHER_AES_256_CBC
     bool "RIOT Cipher Module"
-    select MODULE_CRYPTO
-    select MODULE_CRYPTO_AES_256
-    select MODULE_PSA_RIOT_CIPHER_AES_CBC
+    select MODULE_CRYPTO if TEST_KCONFIG
+    select MODULE_CRYPTO_AES_256 if TEST_KCONFIG
+    select MODULE_PSA_RIOT_CIPHER_AES_CBC if TEST_KCONFIG
 
 endchoice
 
@@ -143,4 +143,4 @@ config PSA_CIPHER_AES_192
 config PSA_CIPHER_AES_256
     bool
     help
-        Indicates that an 256 bit AES key is used
\ No newline at end of file
+        Indicates that an 256 bit AES key is used

MrKevinWeiss

A few meta-review things from the Kconfig/dependency modelling aspect:

Many of the symbols that are used in code (ie #if IS_ACTIVE(CONFIG_*)) should probably be changed into modules so that they can be modelled in the Makefile.deps and exposed with info-modules.
Anything that is used as values such as CONFIG_PSA_MAX_KEY_SIZE should be first modelled in the header file:

#ifndef CONFIG_PSA_MAX_KEY_SIZE
#if (IS_ACTIVE(MODULE_PSA_KEY_SIZE_256_R1) || \
     IS_ACTIVE(MODULE_PSA_CIPHER_AES_256_CBC) || \
     IS_ACTIVE(MODULE_PSA_MAC_HMAC_SHA_256) || \
     IS_ACTIVE(MODULE_PSA_SE_ATECCX08A_ECC))
#define CONFIG_PSA_MAX_KEY_SIZE 32
#elif (IS_ACTIVE(MODULE_PSA_CIPHER_AES_192_CBC) || \
       IS_ACTIVE(MODULE_PSA_ECC_P192_R1))
#define CONFIG_PSA_MAX_KEY_SIZE 24
#elif (IS_ACTIVE(MODULE_PSA_CIPHER_AES_128_CBC)) || \
      (IS_ACTIVE(MODULE_PSA_CIPHER_AES_128_ECB))
#define CONFIG_PSA_MAX_KEY_SIZE 16
#else
#define CONFIG_PSA_MAX_KEY_SIZE 0
#endif
#endif

Same goes for things like the

#ifndef CONFIG_PSA_ASYMMETRIC_KEYPAIR_COUNT
#if IS_ACTIVE(MODULE_PSA_ASYMMETRIC)
#define CONFIG_PSA_ASYMMETRIC_KEYPAIR_COUNT  5
#else
#define CONFIG_PSA_ASYMMETRIC_KEYPAIR_COUNT  0
#endif

Generally all modules should be namespaced, many things (eg HASHES_MD5 should be namespaced with PSA_
I know that you were asked to split the PR but, at least in this state, there are a lot of things included that will be used but not here making it odd to have. There are two paths to move, either reduce and rework this PR to only have the bare minimum which you know will require more work later (eg. menuconfig with many options will only be config then in a follow-up change it back to menuconfig). Since hardware acceleration is not in this PR, all hardware based symbols should be removed from the module selection. OR go back to the initial, big PR, and split the commits to make it semi-reviewable. This would mean that not every step would need a passing test and things can be added with the future in mind.
Obviously the make resolution needs to be implemented, but it would be a lot easier if the first 2 points are followed.

Einhornhool · 2022-11-05T10:13:53Z

Thank you, Kevin.
I think from that perspective it makes more sense to keep everything in one PR with separate commits. I will apply all the changes I made on this branch and the other PSA subbranches to the original PR and reopen it, so we can work on that.
I'll also address numbers 1-3.

Einhornhool · 2022-11-05T14:18:30Z

To make this easier to test and work on are going to keep working on the original branch: #18547

Einhornhool requested review from leandrolanzieri, jia200x and MrKevinWeiss as code owners September 12, 2022 09:31

github-actions bot added Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System labels Sep 12, 2022

github-actions bot added the Area: examples Area: Example Applications label Sep 13, 2022

kfessel reviewed Sep 15, 2022

View reviewed changes

Ollrogge reviewed Sep 16, 2022

View reviewed changes

Einhornhool added 3 commits September 27, 2022 16:58

sys: Add PSA Crypto module

e2a08e6

examples: Add minimal example application for PSA Crypto

8c8bb95

sys/psa_crypto: Address reviews

f26cc27

< 8000 /div>

Einhornhool force-pushed the pr/psa-crypto-module branch from aaaf494 to f26cc27 Compare September 27, 2022 15:00

sys/psa_crypto: Address static test warnings

bdc6ceb

Einhornhool force-pushed the pr/psa-crypto-module branch from 3c71bf0 to bdc6ceb Compare September 27, 2022 15:16

MrKevinWeiss reviewed Oct 4, 2022

View reviewed changes

Einhornhool added 2 commits October 18, 2022 16:41

sys/psa_crypto: Check inputs for NULL pointers

38c1c87

examples/psa_crypto: Change mode of test script

97d42dd

MrKevinWeiss reviewed Nov 3, 2022

View reviewed changes

Einhornhool closed this Nov 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys: Add PSA Crypto module #18578

sys: Add PSA Crypto module #18578


		uint8_t actual_hash_length = PSA_HASH_LENGTH(operation->alg);

		if (hash_size < actual_hash_length) {

sys: Add PSA Crypto module #18578

sys: Add PSA Crypto module #18578

Conversation

Contribution description

Issues/PRs references

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