EC256 encryption is broken when building MCUboot for Zephyr

[davidvincze]

Original report of this issue (from #1734 (comment)):

Build me MCUboot for Zephyr with BOOT_ENCRYPT_EC256 enabled; you can use latest Zephyr with current mcutools/main, preferred platform is nrf52840dk.

[davidvincze]

@de-nordic When I'm building with west and CONFIG_BOOT_ENCRYPT_EC256=y I see a build error caused by MBEDTLS_CHECK_RETURN_TYPICAL in mbedtls/aes.h and a MBEDTLS_CONFIG_FILE redefinition warning.
Currently, I'm building MCUboot v1.10 (before all the refactoring happened).

Is it the same error that you encountered?
(Hopefully it's not my local envs fault)

[davidvincze]

I was building for the nrf52840dk_nrf52840 platform and it seems with setting both CONFIG_BOOT_SIGNATURE_TYPE_ECDSA_P256 and CONFIG_BOOT_ENCRYPT_EC256 it works fine (as docs/readme-zephyr.md says - which could also use some updating).

[de-nordic]

Regarding the Mbed TLS builds nobody probably bothers, I do not even think people try to figure out whether Mbed TLS or TinyCrypt will be used because it is hard to figure out which encryption/signature will compile with which library.
Yes it builds fine now, the build was broken till the Jamie fix done several days ago.
You can build ECB using latest Zephyr with latest MCUboot from mcu-tools/mcuboot/main, because I am not sure whether the Zephyr has the fixed version in fork now.
When you build MCUboot with:

west build -d ecb_boot -b nrf52840dk_nrf52840 -DCONFIG_BOOT_ENCRYPT_EC256=y -DCONFIG_BOOT_ENCRYPTION_KEY_FILE=\"enc-ec256-priv.pem\" -DCONFIG_BOOT_SIGNATURE_TYPE_ECDSA_P256=y

that happens with TinyCrypt not Mbed TLS, which the combination of options disable.

That was one of the issues I have mentioned - configuration requires magic skills: if you have preferred lib, or it is provided by system anyway, it is hard impossible to figure out combination of options to build. And it is not possible to have mix, maybe somebody would like for some reason, where you have ECB encryption and RSA signature because then you have lib collision.

[davidvincze]

Regarding the Mbed TLS builds nobody probably bothers, I do not even think people try to figure out whether Mbed TLS or TinyCrypt will be used because it is hard to figure out which encryption/signature will compile with which library.

That was one of the issues I have mentioned - configuration requires magic skills: if you have preferred lib, or it is provided by system anyway, it is hard impossible to figure out combination of options to build. And it is not possible to have mix, maybe somebody would like for some reason, where you have ECB encryption and RSA signature because then you have lib collision.

I already thought about a feature matrix in the documentation, but an automatic config check would be much better.
MCUboot configuration happens (officially) through the mcuboot_config.h header, but I've seen KConfig and CMake... etc. for being used as a configuration backend, making the header file secondary. The combination of a proper documentation, updated sample_config_file and a basic, centralized/decentralized config check (already present in some of the modules) could be a good start. Any ideas? But that's another topic...

When you build MCUboot with: .... that happens with TinyCrypt not Mbed TLS, which the combination of options disable.

Yes, I saw that from the CMake files... that it uses TinyCrypt for ECDSA and MbedTLS for RSA. I think it's not documented.
(I'm not using Zephyr with MCUboot)

You can build ECB using latest Zephyr with latest MCUboot from mcu-tools/mcuboot/main

Is it the 76d19b patch? To be clear on this, was it caused by some encryption rework or was it independent?
Could you give me a commit hash (MCUboot) that I could use to reproduce the error you experienced?

[de-nordic]

The problem with mcuboot_config.h is that there is separate one for every system, while there should be the main one that defines all available MCUBOOT_ type identifiers, then includes system/platform specific overrides, which redefine some of them, which is then followed by some checks.
We probably need autoconf type C file to be build to actually figure out some stuff but that is impossible wish at this time.

Yes, I saw that from the CMake files... that it uses TinyCrypt for ECDSA and MbedTLS for RSA. I think it's not documented. (I'm not using Zephyr with MCUboot)

Not only. There is ifdef party in headers like sha256.h, while we should have some plug-in architecture for crypto with MCUboot headers defining the API, we have a mess. Now if you would like to support some other lib or maybe hardware accelerated Mbed TLS there are more ifdefs going to happen in sha256.h and other headers.
Or even if you want to support some two incompatible releases of Mbed TLS, to give people grace period to switch, it is a lot of additional ifdefs.

The, lets call it, entropy of MCUboot code as a project increases , CI can not keep up, and configuration is harder and harder to follow.

Is it the 76d19b patch? To be clear on this, was it caused by some encryption rework or was it independent? Could you give me a commit hash (MCUboot) that I could use to reproduce the error you experienced?

No I was wrong, the fix was 018b770

Anyway, this issue can be closed as there is no issue with ECB, if TinyCrypt support is enough - which is default.
It seems that I can now build every encryption configuration.

[davidvincze]

It seems that I can now build every encryption configuration.

I'm glad to hear that. :)

Not only. There is ifdef party in headers like sha256.h, while we should have some plug-in architecture for crypto with MCUboot headers defining the API, we have a mess. Now if you would like to support some other lib or maybe hardware accelerated Mbed TLS there are more ifdefs going to happen in sha256.h and other headers.

Yes, lately we were trying to mitigate and reverse this a bit with the crypto module refactorings (while adding the new PSA Crypto backend) towards a more modularized code structure with a cleaner API. I know there is still work to be done but the direction is good I think.

if TinyCrypt support is enough - which is default.

If a saw it correctly Zephyr only supports TinyCrypt with ECDSA.

The, lets call it, entropy of MCUboot code as a project increases , CI can not keep up, and configuration is harder and harder to follow.

I also share this concern, I'd be happy to continue this discussion in another topic/issue. The loader.c seems like a lot of work the divide and give the core logic a much more modular structure. The crypto modules are a much easier target. I think we should identify code parts that could be rearranged thus making the modularization of the remaining parts easir.
One thing does not seem easy that we may need: moving/renaming/introducing new files.

[davidvincze]

Follow-up discussion: #1770