psa_util.c included in builds without PSA, which can break the build

[gilles-peskine-arm]

In Mbed TLS 3.6.0, when MBEDTLS_ECDSA_C is enabled but MBEDTLS_PSA_CRYPTO_C is disabled, some code from psa_util.c is included and declares a 0-size array, resulting in a compilation error (except under some non-picky compilers). In this configuration, no code from psa_util.c should be included.

The culprit seems to be the definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA in include/mbedtls/config_adjust_legacy_crypto.h:

#if defined(MBEDTLS_ECDSA_C) || (defined(MBEDTLS_PSA_CRYPTO_C) && \
    (defined(PSA_WANT_ALG_ECDSA) || defined(PSA_WANT_ALG_DETERMINISTIC_ECDSA)))
#define MBEDTLS_PSA_UTIL_HAVE_ECDSA
#endif

MBEDTLS_PSA_UTIL_xxx should be either undefined or ignored if MBEDTLS_PSA_CRYPTO_C isn't defined. (Or should this be MBEDTLS_PSA_CRYPTO_CLIENT?)

Originally reported on the mailing list

Definition of done:

• When MBEDTLS_PSA_CRYPTO_C is disabled, no code from psa_util.c should be enabled.
• This should have been caught by the CI, in component_test_no_psa_crypto_full_cmake_asan and in some of the reference configurations. Figure out why it wasn't caught and fix that.

[gilles-peskine-arm]

I'm adding this task to the 3.6.1 epic because it breaks the build.

[mpg]

In this configuration, no code from psa_util.c should be included.

IIRC (couldn't find much written discussion, there's some on #7765 (comment)), it was intentional to include the the ECDSA raw2der/der2raw functions in any build that has at least one implementation of ECDSA. They're only used by the library in builds that have USE_PSA enabled (to convert between PSA format and TLS/X509/PK format) but the reasoning was they may be useful for other purposes as well, so why not offer them so that people (for example who implement other protocols) can start using them if they need? That's also why they're in a public header not just an internal one, and were announced in the ChangeLog under "Features".

Also, now that they're part of the public API in 3.6.0, we need to be careful not to remove them in any configuration where they are working. We might want to just gate them with PSA_VENDOR_ECC_MAX_CURVE_BITS > 0? Or something similar that removes them only in configs where they were never compiling in the first place?

When MBEDTLS_PSA_CRYPTO_C is disabled, no code from psa_util.c should be enabled.

To be clear: I don't think we want that, or can do that without breaking our compatibility promises, or need to do that (see below).

[mpg]

Or we could just fix the code so that it works in all configs where the functions are currently defined.
[Edited to add: implicit context: the original [mailing-list thread(https://lists.trustedfirmware.org/archives/list/[email protected]/thread/WGMVCPKOYERXFJWHKBK3Z47ERKTQMKLJ/) says the issue is a zero-size array, so here I'm suggesting we use the correct macro so that the size of the array is non-zero.]

#if defined(MBEDTLS_PSA_CRYTPO_C) // or perhaps CLIENT?
#define MAX_COORDINATE_BYTES PSA_BITS_TO_BYTES(PSA_VENDOR_ECC_MAX_CURVE_BITS)
#else
#define MAX_COORDINATE_BYTES MBEDTLS_ECP_MAX_BYTES
#endif

I think the reasoning behind the current unconditional use of the PSA macro is that the set of curves enabled on the PSA side is a superset of those enabled on the legacy side (we make sure of that in config_adjust_psa_superset_legacy.h), so let's just use the PSA size, right? Of course the flaw is that's only true when PSA is enabled (more precisely: when that header is included). When it's not, we should use a legacy macro for the size.

[gilles-peskine-arm]

Good point: the ECDSA signature conversion functions are also useful for users who use the legacy API but also need the raw signature format. We have them in psa_util.c more for historical (although recent) reasons than for technical reasons.

[gilles-peskine-arm]

we need to be careful not to remove them in any configuration where they are working

Configurations where the functions try to store data in a 0-size array can't possibly be working. This affects both functions in the same sets of configurations, so there isn't a configuration where one function works but not the other. So we won't break backward compatibility if we gate both functions by the conditions under which they are working in 3.6.0.

[gilles-peskine-arm]

I was wondering why we didn't catch this in tests. We have tests that would overflow an array of length 0, but they're gated by PSA sizes, so they aren't running in non-PSA configurations.

I'm leaning more and more towards going back to my original plan: exclude the functions in non-PSA configurations. In non-PSA configurations, these functions were not clearly documented as available, not working, and not tested.

We should change the guard for MBEDTLS_PSA_UTIL_HAVE_ECDSA to use MBEDTLS_PSA_CRYPTO_CLIENT, though — in that case, the functions were not provided (unless built-in ECDSA was enabled), but are actually useful. That's a separate bug, but we might as well fix it at the same time. (Without a changelog entry, I think, because client-only builds are not officially supported.)

[mpg]

Ah, good point, we're not as tied by our compatibility promises as I thought we were: if the functions have never been working in configuration X, then indeed it's OK to officially remove them in configuration X. So, removing them in non-PSA configs is indeed an option.

I'm still undecided: initially we were thinking the functions could be useful in configurations with ECDSA_C && !CRYPTO_C. I don't think that has changed. Perhaps the only thing that has changed is that now we've figured our that we won't be getting that for free, but have to invest some time in (1) getting the right sizes in the code (2) testing everything properly in all configs? As usual, (2) is the most time-consuming part, so perhaps your reasoning is that the added value of having those functions in ECDSA_C && !CRYPTO_C is not worth the effort?

That's something I could understand. But otherwise, I'm still inclined to say that since our original intention was for the functions to be available in that case as well, then the obvious thing to do is to just make them work.

Wdyt?

[gilles-peskine-arm]

The functions are declared in a header called psa_util.h, so in 3.6.0, they are neither working nor discoverable by users who don't use PSA, even if they're technically documented (but we document a lot of functions in configurations where those functions don't exist — we often don't guard function declarations like we guard their implementation). They are useful if you have a protocol that requires one signature format and an API that uses the other signature format. We added those functions in 3.6.0 primarily to support people who have a protocol with the DER format, and who want to move to PSA as their crypto API.

If existing users have a protocol that needs raw signatures and are using the legacy API, they've presumably found a solution by themselves. And if they haven't, we would prefer it if they moved to the PSA API. If new users have a protocol that needs raw signatures, helping them use the legacy API is not a priority, especially now that there will no longer be a feature release where the legacy API is first-tier.

So I think that making the functions work would run counter to our product strategy, both because we want people to move to PSA and because it would technically be a new feature in an LTS. I'm not fundamentally opposed to doing it, but those considerations put it very low on my priority list.

[mpg]

Makes sense. So I'm happy going back to your original plan: remove those functions from non-PSA configs, where they never worked in the first place.