psa_crypto: Fix psa_key_derivation_output_key ECC without builtin keys

[joerchan]

Fix psa_key_derivation_output_key not being able to derive ECC keys without MBEDTLS_BUILTIN ECC key types enabled.
The PSA crypto drivers can generate these keys without requiring the builtin key types.

Gatekeeper checklist

• changelog not required - will be added in Driver-only ECC: EPIC wrap-up #7273
• backport not required
• tests provided

[valeriosetti]

@mpg
To me this issue sounds similar to the issue you found here and analyzed here

[mpg]

I don't think the proposed changes are correct.

Furthermore, I think the correct fix to the underlying problem that this PR is trying to address will be more complex and likely require some advance study and design work. For example, it might involve using the new/upcoming "cooked key derivation" driver interface.

[mpg]

I don't think that change is correct. This function calls a number of functions from ECP and Bignum. The way we know these are available is by guarding with MBEDTLS_PSA_BUILTIN_KEY_TYPE_ECC_xxx. In the absence of those guards, we'd need to explicitly guard with defined(MBEDTLS_ECP_C) and no actual progress would be made.

I think the current guards are correct here: they're consistent with the definition of the function.

[joerchan]

I want to call psa_key_derivation_output_key. Using ECC key types and accelerated key types, i.e MBEDTLS_PSA_ACCEL_KEY_TYPE_ECC_KEY_PAIR, so no buildin key types enabled.
And this function is preventing the calls to the PSA crypto driver calls.

There was a similar issue like this for validating tag length: 86679c7
I need both of these changes to be able to use cryptocell acceleration on Mbed TLS 3.2.1.

[mpg]

In the absence of those guards, we'd need to explicitly guard with defined(MBEDTLS_ECP_C) and no actual progress would be made.

So, I was wrong about the last part: yes, we need an explicit ECP_C guard, but progress is still being made, as it allows more interesting configurations to build.

[mpg]

@joerchan Thanks for your contribution. I don't think the changes are correct, and I'm not not sure this is the right approach, but I think we should try to clarify what is the underlying goal that you're trying to achieve here. What kind of configuration exactly would you like to start working, that currently doesn't?

[joerchan]

@joerchan Thanks for your contribution. I don't think the changes are correct, and I'm not not sure this is the right approach, but I think we should try to clarify what is the underlying goal that you're trying to achieve here. What kind of configuration exactly would you like to start working, that currently doesn't?

It fixes my issue.

[mpg]

I want to call psa_key_derivation_output_key. Using ECC key types and accelerated key types, i.e MBEDTLS_PSA_ACCEL_KEY_TYPE_ECC_KEY_PAIR, so no buildin key types enabled.
And this function is preventing the calls to the PSA crypto driver calls.

I'd like to clarify: even with this change, psa_key_derivation_output_key() will not be calling a driver, it will still use the internal software implementation (ecp.c, bignum.c). If that's OK with you, then indeed this change fixes your issue. Otherwise, what you need is basically this EPIC which is a lot more work, for which planning needs to be discussed.

It fixes my issue.

That's good to know. However it doesn't change my assessment: we want the guards to be correct not just in the configuration you're using, but all configurations. With your patch as it stands, we're calling functions from ecp.c without any guard ensuring that MBEDTLS_ECP_C is enabled; we need to fix that. For example we could, on top of your current patch, change the body of psa_generate_derived_ecc_key_weierstrass_helper() to

#if defined(MBEDTLS_ECP_C)
/* Current body */
#else
/* Cast each argument to void */
return PSA_ERROR_NOT_SUPPORTED;
#endif

I've checked and unless I've missed something psa_generate_derived_ecc_key_montgomery_helper() is OK (doesn't use any function from ecp.c).

Wdyt?

[joerchan]

So what I meant was that the following code below, like psa_driver_wrapper_get_key_buffer_size, and psa_key_derivation_output_bytes, will invoke the crypto drivers that have hardware acceleration.
I can see that you are right that ECP is dependency for the weierstrass ECC key.
We have this in our configuration so this dependency should be met:

#if defined(PSA_WANT_ALG_ECDH)
#define MBEDTLS_ECDH_C
#define MBEDTLS_ECP_C
#define MBEDTLS_BIGNUM_C
#endif

You suggestion for adding ifdef guards for ECP until the epic you pointed to has been implemented seems like a reasonable thing to do. We're currently on 3.2.1, but will probably want this change for 3.3 and 3.4.
Is there a target release for the transparent crypto key acceleration support?
Should I add that change to this PR?

[mpg]

Thanks for clarifying your needs!

You suggestion for adding ifdef guards for ECP until the epic you pointed to has been implemented seems like a reasonable thing to do. [...] Should I add that change to this PR?

Yes, please do, then I'll approve it, and we can look for a second reviewer (@valeriosetti maybe?) and get it merged. (Note: we have a code freeze for 3.4 coming very soon, so if you push today there's a chance it will get in.)

Is there a target release for the transparent crypto key acceleration support?

I don't think there is. There were discussions about some of your colleagues working on it once they're done with PBKDF2 PSA software implementation, but I don't think there's a firm timeline for that, and we haven't planned any work on that in the next quarter. You probably want to talk to Shebu about this.

[valeriosetti]

we can look for a second reviewer (@valeriosetti maybe?)

Adding myself as reviewer of this one. Just waiting for the last agreed changes to be added then I will start the review

[mpg]

(Note: we have a code freeze for 3.4 coming very soon, so if you push today there's a chance it will get in.)

Note: the current plan is for the code freeze to happen on Monday.

[mpg]

I've taken the liberty to apply the suggested change myself in order to improve our chances of getting this in 3.4.

[mpg]

My original plan was to wait for #7272 to test this, but @AndrzejKurek correctly points out that trying to get this into the release and postponing testing are not compatible, so I'm adding tests. Unfortunately, for this I had to use a more decent version of development (driver support for EC J-PAKE was merged in the meantime), so I'm force-pushing. @joerchan Sorry for the inconvenience.

Note: the test I'm adding is an updated version of that one which was failing, and is now passing, confirming that this patch is indeed an improvement.

Update: force-pushed a second time to resolve a conflict that appeared in the meantime.

[AndrzejKurek]

Minor: These can be probably just added to loc_accel_list, right?

[valeriosetti]

I will try to answer this one because it is something that I found also in the ECDH/ECDSA threads. @mpg please correct me if I'm wrong.

I think that the answer is "no" because otherwise you would find them also accelerated few lines below when building the mbedtls library (not the accelerated one). In other words you add them to the driver build because otherwise they would not be present there (driver use a different mbedtls_config IIRC), but at the same time those are not the thing you want to test for acceleration when building the mbedtls library (I guess for example when running other tests that do not involve EC)

[mpg]

Indeed. loc_accel_list is used for two things: (1) what we want included in libtestdriver1 (2) for what the main libraries should call drivers from libtestdriver1. Usually those are the same, but with composite algorithms (mostly the hash-and-sign algorithms), sometimes we want hashes enables in libtestdriver1, but we don't really want the main library to call drivers for these. That is when doing SHA-256 the main library will use its built-in implementation, but when doing ECDSA-SHA-256, the it will call libtestdriver1, so libtestdriver1 needs (at least minimal) support of SHA-256.

[AndrzejKurek]

Minor: should this be removed?

[mpg]

Indeed, we could remove this comment and just change default to default (no TLS 1.3 or USE_PSA) a few lines above.

[AndrzejKurek]

Looks good, only minor points raised.

[valeriosetti]

Just a couple of minor comments which however are not blocking the PR, so I'm approving it. Hopefully the CI will be OK

[valeriosetti]

This is partially pre-existent, but is there a reason for which this function has different guards in crypto_extra.h?

[mpg]

I don't think so. We should probably synchronize the guards to avoid issue in some builds.

[valeriosetti]

This is another minor thing that is not blocking PR, but I was wondering: now that we changed from MBEDTLS_PSA_BUILTIN_KEY_ to PSA_WANT_KEY_TYPE, can't we also drop the following MBEDTLS_PSA_BUILTIN_ALG guards?
To me it looks like the latter (MBEDTLS_PSA_BUILTIN_ALG) are a subgroup of the former (PSA_WANT_KEY_TYPE), but probably I'm missing something here, isn't it?

[mpg]

I have to say, I'm not entirely sure why the BUILTIN_ALG flags were included here. That's probably something we want to look at, at some point.

[valeriosetti]

@mpg it seems to me that this PR and the test you added in it are very close to what I was trying to achieve in #7103. Am I wrong? If this is the case I can try to rebase that one after this PR will be merged and see if I can change it to a full PR instead of draft

[mpg]

@mpg it seems to me that this PR and the test you added in it are very close to what I was trying to achieve in #7103. Am I wrong? If this is the case I can try to rebase that one after this PR will be merged and see if I can change it to a full PR instead of draft

Indeed, I think this might unblock testing in #7103, so it's worth reviving it once this one is merge to see how it goes.

[mpg]

@AndrzejKurek @valeriosetti Thank you for your reviews! Since you've both approved with non-blocking comments, I'm merging this now, and left a note in #7272 to address the remaining comments.

[joerchan]

@mpg No problem force-pushing. Thank you getting this change in.

[mpg]

I don't think there is. There were discussions about some of your colleagues working on it once they're done with PBKDF2 PSA software implementation, but I don't think there's a firm timeline for that, and we haven't planned any work on that in the next quarter. You probably want to talk to Shebu about this.

Ah, sorry, it's embarrassing, I got mixed up, the discussions were about another partner possibly contributing to the "cooked" key derivation (that includes ECC key derivation) work, but that's unlikely to happen before Q3 this year. The rest of the paragraph is still valid I think: we (Arm) are not planning work on this in Q2, and it you'd like us to prioritize it you should talk to Shebu (obviously, more people asking for the same thing means we're more likely to prioritize it).