sys/psa_crypto: Ed25519 (EdDSA) support #19954

mguetschow · 2023-09-28T12:03:37Z

Contribution description

implement psa_sign_message() and psa_verify_message() for the two already supported PSA_ALG_ECDSA algorithms, together with the CryptoCell and micro-ecc backends (not for the SE backend)
add support for PSA_ALG_PURE_EDDSA, together with the CryptoCell hardware and c25519 software backend (not for the SE backend)
wipe private key data from stack for both ECDSA and EdDSA algorithms using explicit_bzero() (opinions from experienced Riot maintainers about usage of goto to avoid duplicating that function call before every return?)

Testing procedure

examples/psa_crypto has been updated to include EdDSA
successfully tested configurations:
- nrf52840dk with cryptocell (hardware) and c25519 (software) backend
- native with software backend

Issues/PRs references

Thanks @Einhornhool for the PSA Crypto framework implementation #18547 which is great to work with!

Einhornhool · 2023-09-28T12:44:41Z

To me this looks good.
I've read the code and I can build and successfully run the application on the nrf52840dk and native.

I'm also not so sure about using gotos. I learned to avoid them, but in this case it reduces redundancy without seeming too confusing. How is the community's general opinion on those?

miri64 · 2023-09-28T13:03:17Z

@mguetschow can you isolate a67890f into its own PR as a bug fix, in case the review of the rest takes longer? Since we discussed this part, and I know a little bit about the package Makefiles in general, I would instantly ACK this, but I don't feel knowledgeable about the rest. This way the bug fix gets into master faster.

benpicco · 2023-09-28T14:33:55Z

I'm also not so sure about using gotos. I learned to avoid them, but in this case it reduces redundancy without seeming too confusing. How is the community's general opinion on those?

using goto for error return is pretty idiomatic and you will find it in many places. goto is only bad if you use it to jump backwards.

19959: pkg/driver_cryptocell_310: Fix Makefile r=miri64 a=mguetschow ### Contribution description - make sure to download/extract during prepare, instead of build - this fixes the issue of missing include dependencies for other pkgs at build time ### Testing procedure - for an app Makefile, include both `c25519` and `driver_cryptocell_310` - on `master`, `make all` fails with a missing include path from the `driver_cryptocell_310` package - with this change, it works ### Issues/PRs references - isolated from #19954 as suggested by `@miri64` Co-authored-by: Mikolai Gütschow <[email protected]>

19959: pkg/driver_cryptocell_310: Fix Makefile r=benpicco a=mguetschow ### Contribution description - make sure to download/extract during prepare, instead of build - this fixes the issue of missing include dependencies for other pkgs at build time ### Testing procedure - for an app Makefile, include both `c25519` and `driver_cryptocell_310` - on `master`, `make all` fails with a missing include path from the `driver_cryptocell_310` package - with this change, it works ### Issues/PRs references - isolated from #19954 as suggested by `@miri64` 19960: dist/tools/jlink: fix DBG_PID assignment r=benpicco a=LP-HAW 19961: ztimer_periodic: fix example in documentation r=benpicco a=benpicco Co-authored-by: Mikolai Gütschow <[email protected]> Co-authored-by: LP-HAW <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]>

19959: pkg/driver_cryptocell_310: Fix Makefile r=benpicco a=mguetschow ### Contribution description - make sure to download/extract during prepare, instead of build - this fixes the issue of missing include dependencies for other pkgs at build time ### Testing procedure - for an app Makefile, include both `c25519` and `driver_cryptocell_310` - on `master`, `make all` fails with a missing include path from the `driver_cryptocell_310` package - with this change, it works ### Issues/PRs references - isolated from #19954 as suggested by `@miri64` Co-authored-by: Mikolai Gütschow <[email protected]>

MrKevinWeiss · 2023-10-06T07:44:34Z

pkg/driver_cryptocell_310/Makefile


 ifneq ($(RIOTBASE),)
 include $(RIOTBASE)/Makefile.base
 endif

-.PHONY: all clean distclean prepare
+.PHONY: all clean distcleanq


Is distcleanq supposed to be distclean?

Yes, obviously, good catch. Unfortunately this got already merged with #19959, but I've added another commit fixing the typo here.

guarding {sign,verify}_{hash,message} functions see https://github.com/Mbed-TLS/mbedtls/blob/72718dd87e087215ce9155a826ee5a66cfbe9631/include/psa/crypto_values.h\#L1678

Einhornhool · 2023-10-09T08:50:54Z

pkg/driver_cryptocell_310/psa_cryptocell_310/ecc_ed25519.c

+    CRYSError_t ret;
+
+    /* contains seed (private key), concatenated with public key */
+    uint8_t secret_key[CRYS_ECEDW_ORD_SIZE_IN_BYTES + CRYS_ECEDW_MOD_SIZE_IN_BYTES] = { 0x0 };


One thing I just noticed: is there a reason why you write the key into this buffer first and then later memcpy it into priv_key_buffer, instead of directly writing it there?

Yes, the reason is that the cryptocell API returns the seed (private key) concatenated with the public key in a single buffer, see here. secret_key therefore needs to be (at least) 64B while priv_key_buffer only needs to hold the seed (private key) which is 32B.

But looking at it, I notice that the second memcpy is redundant, since it should already be written by the cryptocell API. Will remove that.

But looking at it, I notice that the second memcpy is redundant, since it should already be written by the cryptocell API. Will remove that.

Done with b2349f4.

Einhornhool

Looks good to me, then!

riot-ci · 2023-10-09T11:39:06Z

Murdock results

✔️ PASSED

b2262ae Makefile: mark periph_init_ecc_ed25519 as non-existant

Success	Failures	Total	Runtime
7937	0	7937	14m:02s

Artifacts

Documentation preview

mguetschow · 2023-10-09T11:55:21Z

Sorry, my bad, I committed an unrelated change which increased the stack size for gcoap by mistake 🙈

Should work now.

mguetschow · 2023-10-09T12:24:24Z

Does someone have a clue why the CI is failing for nrf52840dk? The example can be built and run without problem here.

The build output shows little information:

42d41
< periph_init_ecc_ed25519
{"build/": 128}

I've not written any periph_init_ecc_ed25519 function or makefile target.

mguetschow · 2023-10-09T12:29:38Z

Could it be linked to the file makefiles/feature_modules.inc.mk? I don't know what this is for, but just noticed I haven't added the new periph_ecc_ed25519 feature there.

MrKevinWeiss · 2023-10-09T14:43:22Z

Off the top of my head:

If you add a feature starting with periph_ into the make, it will automatically assume there is a periph_init_ for that feature. Not everything needs a periph_init feature though, thus the PERIPH_IGNORE_MODULES list is used, more like opt out. In Kconfig everything is explicit, so these mismatches may occur when a periph_init_ feature that should be there, ends up there.

TLDR; add periph_ecc_ed25519 to the PERIPH_IGNORE_MODULES in makefiles/feature_modules.inc.mk if you don't actually need it.

benpicco

bors merge

19954: sys/psa_crypto: Ed25519 (EdDSA) support r=benpicco a=mguetschow ### Contribution description - implement [`psa_sign_message()`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_sign_message) and [`psa_verify_message()`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_verify_message) for the two already supported [`PSA_ALG_ECDSA`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.PSA_ALG_ECDSA) algorithms, together with the CryptoCell and `micro-ecc` backends (*not* for the SE backend) - add support for [`PSA_ALG_PURE_EDDSA`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.PSA_ALG_PURE_EDDSA), together with the CryptoCell hardware and `c25519` software backend (*not* for the SE backend) - wipe private key data from stack for both ECDSA and EdDSA algorithms using `explicit_bzero()` (opinions from experienced Riot maintainers about usage of `goto` to avoid duplicating that function call before every `return`?) ### Testing procedure - `examples/psa_crypto` has been updated to include EdDSA - successfully tested configurations: - `nrf52840dk` with cryptocell (hardware) and `c25519` (software) backend - `native` with software backend ### Issues/PRs references Thanks `@Einhornhool` for the PSA Crypto framework implementation #18547 which is great to work with! 19966: sys/event: add event_is_queued() r=benpicco a=fabian18 Co-authored-by: Mikolai Gütschow <[email protected]> Co-authored-by: Fabian Hüßler <[email protected]>

benpicco · 2023-10-10T09:45:28Z

bors merge

miri64 · 2023-10-11T08:29:04Z

bors cancel
bors merge

bors · 2023-10-11T12:18:39Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

19465: drivers/mtd: use XFA for pointers to defined MTDs r=benpicco a=gschorcht ### Contribution description This PR provides the support to hold pointers to defined MTDs within a XFA. The XFA allows - to access MTDs of different types (`mtd_flashpage`, `mtd_sdcard`, `mtd_emulated`, ...) by an index - to determine the number of MTDs defined in the system. ### Testing procedure To be defined once PR #19443 is merged because emulated MTDs will allow to test this PR on arbitrary boards. ### Porting Guide For external boards: - remove the `MTD_NUMOF` definition from `board.h` - add `MTD_XFA_ADD(<mtd_dev>, <idx>);` to the definition of `<mtd_dev>`. - `MTD_0`, `MTD_1`, … defines are no longer needed. ### Issues/PRs references Related to PR #19443 19981: Fletcher32: Add incremental API r=benpicco a=bergzand ### Contribution description This PR extends the current fletcher32 checksum with an incremental API mode. This way the bytes to be checksummed can be supplied via multiple successive calls and do not have to be provided in a single consecutive buffer. I've also rephrased the warning with the original function a bit as that function uses an `unaligned_get_u16` to access the data. The data thus does not require alignment, but the length does need to be supplied as number of 16 bit words. ### Testing procedure The test has been extended ### Issues/PRs references None 19995: sys/psa_crypto: Fix macro for public key max size and SE example r=benpicco a=Einhornhool ### Contribution description #### 1. Wrong public key size when using secure elements, introduced by #19954 Fixed conditions for key size macros in `crypto_sizes.h`. #### 2. EdDSA and ECDSA examples fail when using a secure element because of unsopported changes introduced by #19954 Updated `example/psa_crypto` to use only supported functions for secure elements. ### Testing procedure Build `example/psa_crypto` for secure elements and run application Output on master: ``` 2023-10-19 14:33:24,372 # main(): This is RIOT! (Version: 2019.07-devel-22378-gb6772) 2023-10-19 14:33:24,372 # HMAC SHA256 took 56393 us 2023-10-19 14:33:24,372 # Cipher AES 128 took 68826 us 2023-10-19 14:33:24,372 # *** RIOT kernel panic: 2023-10-19 14:33:24,373 # HARD FAULT HANDLER 2023-10-19 14:33:24,373 # 2023-10-19 14:33:24,373 # *** rebooting... ``` Output with fixes: ``` 2023-10-19 13:35:24,715 # main(): This is RIOT! (Version: 2019.07-devel-22384-g8ef66-dev/psa-crypto-fixes) 2023-10-19 13:35:24,715 # HMAC SHA256 took 56374 us 2023-10-19 13:35:24,715 # Cipher AES 128 took 68805 us 2023-10-19 13:35:24,715 # ECDSA took 281164 us 2023-10-19 13:35:24,715 # All Done ``` Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: Koen Zandberg <[email protected]> Co-authored-by: Lena Boeckmann <[email protected]>

20037: nib/_nib-6ln: bail out early if address is no longer assigned [backport 2023.10] r=benpicco a=MrKevinWeiss # Backport of #19999 20038: nanocoap: prevent integer underflow in coap_opt_put_uri_pathquery() [backport 2023.10] r=benpicco a=MrKevinWeiss # Backport of #19994 20039: sys/psa_crypto: Fix macro for public key max size and SE example [backport 2023.10] r=benpicco a=MrKevinWeiss # Backport of #19995 ### Contribution description #### 1. Wrong public key size when using secure elements, introduced by #19954 Fixed conditions for key size macros in `crypto_sizes.h`. #### 2. EdDSA and ECDSA examples fail when using a secure element because of unsopported changes introduced by #19954 Updated `example/psa_crypto` to use only supported functions for secure elements. ### Testing procedure Build `example/psa_crypto` for secure elements and run application Output on master: ``` 2023-10-19 14:33:24,372 # main(): This is RIOT! (Version: 2019.07-devel-22378-gb6772) 2023-10-19 14:33:24,372 # HMAC SHA256 took 56393 us 2023-10-19 14:33:24,372 # Cipher AES 128 took 68826 us 2023-10-19 14:33:24,372 # *** RIOT kernel panic: 2023-10-19 14:33:24,373 # HARD FAULT HANDLER 2023-10-19 14:33:24,373 # 2023-10-19 14:33:24,373 # *** rebooting... ``` Output with fixes: ``` 2023-10-19 13:35:24,715 # main(): This is RIOT! (Version: 2019.07-devel-22384-g8ef66-dev/psa-crypto-fixes) 2023-10-19 13:35:24,715 # HMAC SHA256 took 56374 us 2023-10-19 13:35:24,715 # Cipher AES 128 took 68805 us 2023-10-19 13:35:24,715 # ECDSA took 281164 us 2023-10-19 13:35:24,715 # All Done ``` Co-authored-by: Benjamin Valentin <[email protected]> Co-authored-by: Lena Boeckmann <[email protected]>

mguetschow requested review from leandrolanzieri, jia200x, MrKevinWeiss, aabadie and haukepetersen as code owners September 28, 2023 12:03

benpicco requested review from PeterKietzmann and Einhornhool September 28, 2023 12:10

mguetschow force-pushed the psa-crypto-ed25519 branch from 7ff1059 to b9711d4 Compare September 28, 2023 12:12

waehlisch assigned Einhornhool Sep 28, 2023

mguetschow force-pushed the psa-crypto-ed25519 branch from b9711d4 to 3627baa Compare September 28, 2023 12:19

miri64 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Sep 28, 2023

mguetschow mentioned this pull request Sep 29, 2023

pkg/driver_cryptocell_310: Fix Makefile #19959

Merged

MrKevinWeiss reviewed Oct 6, 2023

View reviewed changes

mguetschow added 4 commits October 9, 2023 10:21

sys/psa_crypto: add support for Ed25519 (EdDSA)

963775b

sys/psa_crypto: fix PSA signing macros

335c4f8

guarding {sign,verify}_{hash,message} functions see https://github.com/Mbed-TLS/mbedtls/blob/72718dd87e087215ce9155a826ee5a66cfbe9631/include/psa/crypto_values.h\#L1678

sys/psa_crypto: wipe temporary private key copies from stack

7b9c113

pkg/driver_cryptocell_310: fix typo in Makefile

d8d5478

mguetschow force-pushed the psa-crypto-ed25519 branch from 3627baa to d8d5478 Compare October 9, 2023 08:24

Einhornhool reviewed Oct 9, 2023

View reviewed changes

github-actions bot added the Area: network Area: Networking label Oct 9, 2023

Einhornhool approved these changes Oct 9, 2023

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 9, 2023

psa-crypto: remove redundant memcpy

da41dd1

mguetschow force-pushed the psa-crypto-ed25519 branch from b2349f4 to da41dd1 Compare October 9, 2023 11:53

github-actions bot removed the Area: network Area: Networking label Oct 9, 2023

Makefile: mark periph_init_ecc_ed25519 as non-existant

b2262ae

github-actions bot added the Area: build system Area: Build system label Oct 9, 2023

benpicco approved these changes Oct 10, 2023

View reviewed changes

bors bot merged commit ff71a10 into RIOT-OS:master Oct 11, 2023
25 checks passed

This was referenced Oct 19, 2023

sys/psa_crypto: Fix build problems #19992

Merged

sys/psa_crypto: Fix macro for public key max size and SE example #19995

Merged

mguetschow deleted the psa-crypto-ed25519 branch October 30, 2023 10:27

MrKevinWeiss mentioned this pull request Nov 2, 2023

sys/psa_crypto: Fix macro for public key max size and SE example [backport 2023.10] #20039

Merged

MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023

mguetschow mentioned this pull request Feb 6, 2024

sys/psa_crypto: return proper error code for psa_verify_msg #20346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/psa_crypto: Ed25519 (EdDSA) support #19954

sys/psa_crypto: Ed25519 (EdDSA) support #19954

mguetschow commented Sep 28, 2023

Einhornhool commented Sep 28, 2023 •

edited

Loading

miri64 commented Sep 28, 2023

benpicco commented Sep 28, 2023

MrKevinWeiss Oct 6, 2023

mguetschow Oct 9, 2023

Einhornhool Oct 9, 2023

mguetschow Oct 9, 2023

mguetschow Oct 9, 2023

Einhornhool left a comment

riot-ci commented Oct 9, 2023 •

edited

Loading

mguetschow commented Oct 9, 2023

mguetschow commented Oct 9, 2023 •

edited

Loading

mguetschow commented Oct 9, 2023

MrKevinWeiss commented Oct 9, 2023

benpicco left a comment

benpicco commented Oct 10, 2023

miri64 commented Oct 11, 2023

bors bot commented Oct 11, 2023

sys/psa_crypto: Ed25519 (EdDSA) support #19954

sys/psa_crypto: Ed25519 (EdDSA) support #19954

Conversation

mguetschow commented Sep 28, 2023

Contribution description

Testing procedure

Issues/PRs references

Einhornhool commented Sep 28, 2023 • edited Loading

miri64 commented Sep 28, 2023

benpicco commented Sep 28, 2023

MrKevinWeiss Oct 6, 2023

Choose a reason for hiding this comment

mguetschow Oct 9, 2023

Choose a reason for hiding this comment

Einhornhool Oct 9, 2023

Choose a reason for hiding this comment

mguetschow Oct 9, 2023

Choose a reason for hiding this comment

mguetschow Oct 9, 2023

Choose a reason for hiding this comment

Einhornhool left a comment

Choose a reason for hiding this comment

riot-ci commented Oct 9, 2023 • edited Loading

Murdock results

Artifacts

mguetschow commented Oct 9, 2023

mguetschow commented Oct 9, 2023 • edited Loading

mguetschow commented Oct 9, 2023

MrKevinWeiss commented Oct 9, 2023

benpicco left a comment

Choose a reason for hiding this comment

benpicco commented Oct 10, 2023

miri64 commented Oct 11, 2023

bors bot commented Oct 11, 2023

Einhornhool commented Sep 28, 2023 •

edited

Loading

riot-ci commented Oct 9, 2023 •

edited

Loading

mguetschow commented Oct 9, 2023 •

edited

Loading