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

[sezrab]

Introduced max size/bytes macros in psa_util.c. Prevents 0-size array compiler warning in psa-util.c when MBEDTLS_ECDSA_C is enabled but MBEDTLS_PSA_CRYPTO_C is disabled. Fixes #9311

PR checklist

• changelog provided
• 3.6 backport provided, see [Backport 3.6] psa_util.c included in builds without PSA, which can break the build #9463
• 2.28 backport not required, feature doesn't exist
• tests not required because we removed buggy functions in configurations that made them buggy. See also Default to stricter warning flags in CMakeLists.txt and all.sh #9317

[gilles-peskine-arm]

This is correct but incomplete.

Although it's correct, I'm not sure this is the right approach, between restricting the function definition and fixing the array sizes. Let's discuss this further with Manuel in the issue.

[gilles-peskine-arm]

It doesn't really matter since these are just macros, but the new macros are only used in code guarded by #if defined(MBEDTLS_PSA_UTIL_HAVE_ECDSA), so the macro definitions should be after the #if defined(MBEDTLS_PSA_UTIL_HAVE_ECDSA) line.

[gilles-peskine-arm]

When MBEDTLS_PSA_CRYPTO_CLIENT is defined, ECDSA can be implemented either through mbedtls_ecdsa_xxx or through psa_xxx. So naively, one might expect that ECDSA_SIGNATURE_MAX_SIZE should be the max of PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE and MBEDTLS_ECDSA_MAX_LEN, and similarly for MAX_COORDINATE_BYTES.

It is in fact guaranteed that PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE >= MBEDTLS_ECDSA_MAX_LEN and so on, because it is guaranteed that all curves supported through the legacy API are also supported through PSA. We should document that we rely on this guarantee here.

We should have added a comment when we wrote the original code here, to justify why PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE was enough. But we didn't. So please write this comment now.

[gilles-peskine-arm]

Since this is a user-impacting bug, we need a changelog entry.

A changelog entry goes into a file in the ChangeLog.d directory. See https://github.com/Mbed-TLS/mbedtls/blob/development/ChangeLog.d/00README.md for explanations and existing entries in ChangeLog.d/*.txt and ChangeLog for examples.

[gilles-peskine-arm]

Note that the bug isn't only “Prevents 0-size array compiler warning”: in configurations with this warning, calling this function would never work since it would try to store data in the 0-size array.

[gilles-peskine-arm]

We need either a non-regression test (i.e. a test that fails before applying the fix and passes after), or a good reason not to have one. Sometimes, “it's too much effort to write a test” can be a good reason.

Here I see two ways in which a test could catch the bug.

One way is at compile time: either we are never compiling in an affected configuration, or we are compiling in some affected configurations but only with permissive compiler flags (GCC and Clang allow 0-size array by defaults). According to the outcome file from #9301 (I'm looking at the 3.6 outcomes, because in the development we're moving to PSA-only configurations where this bug can't happen), and with the help of search_outcomes.py from Mbed-TLS/mbedtls-framework#20, I can see:

$ framework/scripts/search_outcomes_config.py -f outcomes-3.6.csv MBEDTLS_ECDSA_C \!MBEDTLS_PSA_CRYPTO_C
component_test_default_psa_crypto_client_without_crypto_provider
component_test_full_no_cipher_no_psa_crypto
component_test_no_psa_crypto_full_cmake_asan
config-no-entropy.h
config-suite-b.h

The first one has MBEDTLS_PSA_CRYPTO_C disabled but MBEDTLS_PSA_CRYPTO_CLIENT enabled, so it's actually not relevant here. The other 4 are relevant. The two configuration names ending with .h are handled by tests/scripts/test-ref-configs.pl, and they're just using the config files in the configs subdirectory. All of these builds use Clang (or maybe GCC) without -std=c99 -pedantic, so they don't complain about 0-length arrays. Ideally we should pass -std=c99 -pedantic, but I haven't checked if it's feasible without too much effort: it might cause a bunch of other errors that we don't want to deal with.

The second way to catch the bug would be through tests. After all, if we run a test that tries to use a 0-size array, the test would surely overflow that array. We do most test builds with Asan, which detects buffer overflows. Why wasn't there a test failure? We do have tests for these functions. They're in test_suite_psa_crypto_util. The test function is only guarded by MBEDTLS_PSA_UTIL_HAVE_ECDSA, but the test data is guarded by a minimum value for PSA_VENDOR_ECC_MAX_CURVE_BITS for all test cases. If the functions are available in configurations where PSA is disabled, we need to change the guards on these tests, since PSA_VENDOR_ECC_MAX_CURVE_BITS is 0 when PSA is disabled.

So we now understand that we didn't catch this bug due to a test gap. And we need to fix the test gap here.

[gilles-peskine-arm]

Note that in builds with PSA disabled, PSA_WANT_xxx symbols might still be set, so PSA_VENDOR_ECC_MAX_CURVE_BITS might still end up being nonzero. I haven't figured out exactly when it ends up being 0.

[mpg]

Ideally we should pass -std=c99 -pedantic, but I haven't checked if it's feasible without too much effort: it might cause a bunch of other errors that we don't want to deal with.

In the past we there were indeed some portions of our code that threw a lot of errors with those flags, but looking at all.sh it looks like nowadays we can use them with config.py full and build everything. So, I'm tempted to suggest we use -std=c99 -pedantic as our new default in all.sh (like, add it to $ASAN_CFLAG and cmake's ASan flags), and only disable it locally if there are a few components that don't work with it. Wdyt?

(@sezrab That would of course be out of scope for this PR and be a separate piece of work.)

[gilles-peskine-arm]

I'm tempted to suggest we use -std=c99 -pedantic as our new default in all.sh (like, add it to $ASAN_CFLAG and cmake's ASan flags), and only disable it locally if there are a few components that don't work with it

That's absolutely a good idea. The thing is, I have no idea how far we are from it, so I can't estimate how long it'll take, which means it's currently not feasible.

[mpg]

Unfortunately this grew a conflict. @sezrab Can you resolve the conflict, then ping Gilles and me, and we'll re-review shortly?

[mpg]

Aw, very unlucky, got a new conflict right after your rebased. Can you rebase again? Thanks!

[mpg]

I think this fixes the issue, but also does extra work that's no longer needed as the chosen fix was to remove those functions from non-PSA builds.

[mpg]

@sezrab Now that this is approved, can you create the 3.6 backport? Thanks!

I am dismissing my approval because I think something's wrong due to a change in development that happened after the work here started.

[gilles-peskine-arm]

This pull request has a semantic conflict with changes in the development branch: it changes the definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA, but MBEDTLS_PSA_UTIL_HAVE_ECDSA is no longer used in development.

In develompent now, we're removing support for builds without PSA. The library functions are now guarded by PSA_HAVE_ALG_SOME_ECDSA, no longer by MBEDTLS_PSA_UTIL_HAVE_ECDSA. The test functions are also guarded by PSA_HAVE_ALG_SOME_ECDSA. So the bug is resolved in development, although we hadn't noticed that there was a bug so we should still announce it in a changelog entry. (This changelog entry will need to change because it's referring to non-PSA builds, which will no longer be a thing. But I expect that we'll handle these changes to changelog entries in bulk at the end of the de-non-PSA-ification work.)

In addition to the changelog entry, we should remove the unused definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA. Having this unused definition created confusion. For example, if the definition had been removed, there would have been a git conflict with this pull request, which would have alerted us that there was a conflict.

[gilles-peskine-arm]

In development today, MBEDTLS_PSA_UTIL_HAVE_ECDSA is still defined but never used. So there's a semantic conflict between what this pull request is doing and what has changed in development.

[mpg]

should still announce it in a changelog entry.

As a general principle I would agree. But in this case, since the bug was only when PSA is disabled, and in 4.0 PSA will always be on, does it really make sense to have a ChangeLog entry for this in 4.0? The following would seem weird to me:

Changes
    * PSA Crypto is always enabled.
Bugfix
    * Fix compile error when PSA is disabled by removing the offending functions when PSA is disabled.

we should remove the unused definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA. Having this unused definition created confusion. For example, if the definition had been removed, there would have been a git conflict with this pull request, which would have alerted us that there was a conflict.

Agreed, though I think that's a job for 9384's author.

So overall, I'd be inclined to just close this development PR, and keep the "backport" as a 3.6-only PR, changing the description to add "PR development: not required because: bug defined out of existence in 4.0".

Wdyt?

[gilles-peskine-arm]

As a general principle I would agree. But in this case, since the bug was only when PSA is disabled, and in 4.0 PSA will always be on, does it really make sense to have a ChangeLog entry for this in 4.0? The following would seem weird to me:

I expect that many changelog entries for bug fixes will not look right for the 4.0 release because they've been overridden. Sometimes they'll be overridden in ways that we don't anticipate yet because we don't know all the details of 4.0. We'll need to go over changelog entries and reword the ones that need rewording, including but not limited to removing the ones that are completely obsolete. That's a lot easier than going through the whole lot of commits and figuring out everything we need to announce.

So no, please don't skip changelog entries for bug fixes just because you think they'll go away later.

Agreed, though I think that's a job for 9384's author.

I don't care who does it. We're a team. If there's a reason for it to stay for now, then of course it should stay. But then we should add a comment to explain why it's staying!

[mpg]

So no, please don't skip changelog entries for bug fixes just because you think they'll go away later.

Ok then. In that specific instance I think we don't just think it'll go away, we know it, because PSA always on is one of the defining features of 4.0. OTOH, you're right that in other cases things will be less clear, and it's better to have a uniform process with no exception, so let's add the ChangeLog now even though it's extremely likely to be removed later. (Btw, do we have an issue to track that step when preparing the 4.0 release?)

I don't care who does it. We're a team.

Sure. I meant for another PR. My line of thinking was whether we still needed this PR at all. Since we're doing the ChangeLog entry anyway, then the question is moot.

[gilles-peskine-arm]

(Btw, do we have an issue to track that step when preparing the 4.0 release?)

Not yet. So far I've avoided creating 4.0 issues that aren't ready for work yet, to avoid noise during planning. I've only created a few because they were direct follow-ups to tasks that are started or ready to start.

[mpg]

#9384 has been replacing and we shouldn't be undoing that here. Please remove the changes to the test files (which were correct at the time you made them, btw, but development is moving fast these days - now the functions are guarded by PSA_HAVE_ALG_SOME_ECDSA so that magically became the right macros for the tests too).

[mpg]

Just to summarize, I think this PR should be ChangeLog only:

• no need to change the definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA as it's no longer used and will be removed by Remove definitions of some legacy symbols #9396 (soon, hopefully);
• no need to change the macro used in the tests as it's now correct thanks to changes that have happened in development in the meantime.

(For the avoidance of doubt: that's just for the development PR. The 3.6 PR is fine as it is and keeps the fix to the definition in addition to the ChangeLog.)

[gilles-peskine-arm]

Approved with just the changelog because we need the 3.6 PR in the upcoming release, but I would strongly prefer to either remove the definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA or edit its comment to state that it shouldn't be used anymore and explain why it's still there.