mbedtls: Move local mbedtls to v3.6.0

[d3zd3z]

The in-tree mbedtls (used for the simulator and some targets) is a few years old, and currently is unable to pass the rsa tests when built with clang. Update this mbed TLS to the v3.5.2 release. This fixes clang support in the simulator.

Fixes #1986

[d3zd3z]

Some of the code depends on internal fields in Mbed TLS which have been made inaccessible. There will need to be some code rewrites to make this work.

[utzig]

Try updating: https://github.com/mcu-tools/mcuboot/blob/main/boot/bootutil/include/bootutil/crypto/common.h#L14

To: #if (MBEDTLS_VERSION_NUMBER >= 0x03000000) && (MBEDTLS_VERSION_NUMBER <= 0x03010000)

And leave the MBEDTLS_CONTEXT_MEMBER usage there; this way it might work for all Mbed-TLS versions.

[utzig]

Otherwise you could also update ext/mbedtls-asn1 to 3.5.2, which seems the reason for the Mynewt failure, at least...

[d3zd3z]

I've tried updating mbedtls-asn1 to 3.6.0. However, mynewt fails to build with this. There are two issues. One is that CONFIG_MBEDTLS_HAVE_TIME is being defined, even though it shouldn't be. I'm not sure where the definition is coming from. The other is that the mynewt upstream reference to mbedtls doesn't quite work. I'm not sure how to fix that, since it might have to be coordinated with a fix here. Maybe it is easiest to make the context thing conditional, but these changes are only going to get worse as we move to newer and newer versions of Mbed TLS. In the next release, the private fields will be unavailable entirely, and we will have to rewrite the remaining code that uses private fields.

[d3zd3z]

@almir-okato I'm trying to figure out the best way to get espressif working with the newest Mbed TLS. It is arguable that this is actually broken in upstream, but it will take a while to get a fix in. It might make sense to just remove CONFIG_MBEDTLS_HAVE_TIME from all of the espressif builds. I don't think it is needed, as we aren't using TLS.

Thoughts?

[utzig]

Mynewt does not define the date macros in tree; this config is coming from ext/mbedtls-asn1 itself. Also seems like you're adding a whole MbedTLS to ext/mbedtls-asn1 instead of just updating the required files (at least that's what I see on the diff). Wouldn't it be better to leave as is, only update the #if I mentioned above, and leave the mbedtls-asn1 update for another PR?

[utzig]

LOL, one possible hack! Change https://github.com/mcu-tools/mcuboot/blob/main/ext/mbedtls-asn1/include/mbedtls/private_access.h from:

#ifndef MBEDTLS_ALLOW_PRIVATE_ACCESS
#define MBEDTLS_PRIVATE(member) private_##member
#else
#define MBEDTLS_PRIVATE(member) member
#endif

to:

#define MBEDTLS_PRIVATE(member) member

[d3zd3z]

Mynewt does not define the date macros in tree; this config is coming from ext/mbedtls-asn1 itself. Also seems like you're adding a whole MbedTLS to ext/mbedtls-asn1 instead of just updating the required files (at least that's what I see on the diff). Wouldn't it be better to leave as is, only update the #if I mentioned above, and leave the mbedtls-asn1 update for another PR?

Those are the minimal files I was able to bring in to get it to build. There is just a lot of include promiscuity in their header files, and a lot gets pulled in. Honestly, I'd like to get rid of the directory entirely, and just depend on the mbed TLS tree.

[d3zd3z]

LOL, one possible hack! Change https://github.com/mcu-tools/mcuboot/blob/main/ext/mbedtls-asn1/include/mbedtls/private_access.h from:

#ifndef MBEDTLS_ALLOW_PRIVATE_ACCESS
#define MBEDTLS_PRIVATE(member) private_##member
#else
#define MBEDTLS_PRIVATE(member) member
#endif

to:

#define MBEDTLS_PRIVATE(member) member

Unfortunately, that doesn't fix the things that rely on the full upstream Mbed TLS, where we can't change the definition.

It also doesn't work, because the removal of private is only for the ASN.1 headers, and the structs used for some of the keys still have private fields. That is going to have to get fixed soon, anyway, because they are removing the ability to access private fields at all in the next release. But, they know are needs, so hopefully there will be accessors for what is needed. Mostly, it involves constructing the keys with import functions instead of just assembling them manually.

[utzig]

Mynewt does not define the date macros in tree; this config is coming from ext/mbedtls-asn1 itself. Also seems like you're adding a whole MbedTLS to ext/mbedtls-asn1 instead of just updating the required files (at least that's what I see on the diff). Wouldn't it be better to leave as is, only update the #if I mentioned above, and leave the mbedtls-asn1 update for another PR?

Those are the minimal files I was able to bring in to get it to build. There is just a lot of include promiscuity in their header files, and a lot gets pulled in. Honestly, I'd like to get rid of the directory entirely, and just depend on the mbed TLS tree.

Better to just use or adapt something like the hack I suggested above. This library has nothing to do with MbedTLS apart from the name, and files copied over, but it's not to be used when MbedTLS is defined, only for Tinycrypt/fiat/whatever.

[d3zd3z]

I have moved the mcuboot change to allow newer mbed TLS into #1989 so it can be merged separately, and that will fix Zephyr, for example. This looks like it will need some work with espressif and mynewt.

[utzig]

I have moved the mcuboot change to allow newer mbed TLS into #1989 so it can be merged separately, and that will fix Zephyr, for example. This looks like it will need some work with espressif and mynewt.

Mynewt passed. For Espressif try commenting out these lines:

https://github.com/mcu-tools/mcuboot/blob/main/boot/espressif/include/crypto_config/mbedtls_custom_config.h#L136
https://github.com/mcu-tools/mcuboot/blob/main/boot/espressif/include/crypto_config/mbedtls_custom_config.h#L157

I believe this should allow it to also pass the tests. I don't think those should be enabled anyway, but @almir-okato can approve later.

[d3zd3z]

Mynewt passed. For Espressif try commenting out these lines:

I tried this, and at least locally, it doesn't seem to pass. There seem to be declarations within the ESP32 boot/espressif/hal/esp-idf submodule that also enable the HAVE_TIME, that I'm not quite sure what is the right way to fix.

Mynewt works because it doesn't pull in the submodule, but pulls in the mbedtls directly. Espressif uses the submodules, and runs into the issues.

Ideally, we should possibly migrate mynewt to a newer mbed TLS at some point, but at least that can be done independent of mcuboot.

One workaround would be to just go grab the old version of mbedtls in the test.

[d3zd3z]

I have added some code to the espressif ci script to revert mbedtls to a known working version. The real fix probably needs to happen in mbed TLS, so will probably come with a newer version. The time macro check in mbed TLS assumes either POSIX or Windows, and hits an #error if neither of these are present.

[d3zd3z]

Rebased this. I think this is a good first step to migrating to newer mbed TLS. We upgrade the version in our tree, but hold back mynewt and espressif, since fixing those involves changes outside of the mcuboot tree.

• Espressif: I think this is a bug in upstream mbed TLS, where it doesn't handle systems that provide time, but aren't posix or windows.
• Mynewt: This is mostly a build issue. Need to figure out where CONFIG_MCUBOOT_HAVE_TIME is getting set (it might have been getting set before, but wasn't a problem). But, it doesn't actually have time. There also may be issues with available files and such.

[utzig]

• Mynewt: This is mostly a build issue. Need to figure out where CONFIG_MCUBOOT_HAVE_TIME is getting set (it might have been getting set before, but wasn't a problem). But, it doesn't actually have time. There also may be issues with available files and such.

Mynewt always had explicitly disabled this option, I will test locally but this should already be at the NOT set config.

[utzig]

• Mynewt: This is mostly a build issue. Need to figure out where CONFIG_MCUBOOT_HAVE_TIME is getting set (it might have been getting set before, but wasn't a problem). But, it doesn't actually have time. There also may be issues with available files and such.

Mynewt always had explicitly disabled this option, I will test locally but this should already be at the NOT set config.

Confirmed, Mynewt does not set MBEDTLS_HAVE_TIME nor MBEDTLS_HAVE_TIME_DATE configuration, tested on both RSA/Mbed-TLS and EC-256/Tinycrypt/Mbedtls-ASN1, with this branch as well.

[utzig]

I think this is fine but I am quite sure the Espressif changed will be reverted, I still believe that changing the configs I pointed above would solve the issues! :-P

[almir-okato]

I think this is fine but I am quite sure the Espressif changed will be reverted, I still believe that changing the configs I pointed above would solve the issues! :-P

Sorry I didn't notice that I was tagged here. I'll take a better look into this.

[nordicjm]

@d3zd3z can you rebase and remove the final commit? Should be fixed now

[d3zd3z]

LOL, one possible hack! Change https://github.com/mcu-tools/mcuboot/blob/main/ext/mbedtls-asn1/include/mbedtls/private_access.h from:

#ifndef MBEDTLS_ALLOW_PRIVATE_ACCESS
#define MBEDTLS_PRIVATE(member) private_##member
#else
#define MBEDTLS_PRIVATE(member) member
#endif

to:

#define MBEDTLS_PRIVATE(member) member

This will almost certainly break with future versions of MbedTLS, though. Their intent is to make these fields impossible to access, not just more difficult. There will probably always be a way to work around it, but it will likely just get messier.