Implement non-PSA pk_sign_ext()

[tomi-font]

Description

This resolves #7583.
Details in commit messages.

PR checklist

• changelog provided
• backport not required
• tests provided

[mpg]

This is the implementation of #7583.

Minor: please use github keywords to link this PR to that issue.

[mpg]

I've assigned myself as a reviewer, but will wait until CI passes before I do a full review - for now I just had a quick look, and this looks like it's going in the right direction. Please ping me the CI is green!

If you have trouble reproducing or understanding certain CI failures, feel free to ask questions!

@valeriosetti Can I assume you'll also review this (when you're back)?

[tomi-font]

@mpg After some trial and error I somewhat narrowed down and reduced the failures, but still don't have them quite figured out.

I think that some were due to me having been too keen on removing references to MBEDTLS_PSA_CRYPTO_C in the PK module.
It seems like they have further reaching consequences than I anticipated.
The fix commit with the culprit changes undone is test fix for CI failures 4.

But then, the remaining issue is that some CI tests are failing when using the PSA version of mbedtls_pk_sign_ext() only if MBEDTLS_USE_PSA_CRYPTO is defined (which is what should be done).
If #ifdefing based on MBEDTLS_PSA_CRYPTO_C instead (as is done in test fix for CI failures 5) everything passes.
So to me it seems that something is wrong either in the non-PSA implementation (that only shows up when MBEDTLS_PSA_CRYPTO_C && !MBEDTLS_USE_PSA_CRYPTO), or in the dependencies of tests that rely on this function. Based on my iterations at attempting to fix this, I am leaning towards the latter.

My latest commit, test fix for CI failures 7, is an attempt at tackling some of these test failures by tightening their dependencies.
e.g. in the PK tests failing case in a particular configuration MBEDTLS_SHA*_C were not defined in spite of having their respective MBEDTLS_MD_CAN_SHA* defined. The hashing algorithms were not available in the non-PSA version and made mbedtls_pk_sign_ext() return an MD error code.

Then I think that the bigger (and actually maybe only) issue is between TLS 1.3 and PSA. Kind of the same story there I believe.
The latest CI results don't look as bad anymore, but I don't quite know how to go about getting over with this.

I would appreciate some guidance or even fixes for these failures.

[gilles-peskine-arm]

b460e02 "test fix for CI failures" passed. Why are you not satisfied with it? (I haven't really looked at the code, I just came by to see if I could help with the CI results.)

51ee7de seems to be going in the wrong direction: it's making some tests depend on MBEDTLS_SHAxxx_C (the built-in implementation of SHAxxx) instead of MBEDTLS_MD_CAN_DO_SHAxxx (an implementation of SHAxxx which may either be the built-in one or a PSA accelerator), and it's making some TLS 1.3 tests depend on MBEDTLS_USE_PSA_CRYPTO whereas the TLS 1.3 code always uses PSA APIs (regardless of MBEDTLS_USE_PSA_CRYPTO).

cbd3d3a doesn't look right either: if MBEDTLS_USE_PSA_CRYPTO is off, then pk.c should not use any PSA APIs except to support opaque keys (MBEDTLS_PK_OPAQUE). The code for MBEDTLS_PK_RSA should not use PSA APIs when MBEDTLS_USE_PSA_CRYPTO is disabled.

[mpg]

b460e02 "test fix for CI failures" passed. Why are you not satisfied with it?

Because it dispatches based on PSA_CRYPTO_C instead of USE_PSA_CRYPTO. So, the fact that this commit passes while the other fail is useful information, but we don't want to keep the code like this.

I'll take a closer look.

[mpg]

Minor: I think the comment was correct. Version 2.1 of the standard still defines the functions from the 1.5 standard, sometimes in a more precise way. The function has V1_5 in the name used by the standard, so it's unmabiguous.

[tomi-font]

Oh okay. So I assume it's the same for the others I changed?

[mpg]

if MBEDTLS_USE_PSA_CRYPTO is off, then pk.c should not use any PSA APIs except to support opaque keys (MBEDTLS_PK_OPAQUE)

I think the "except" clause is not correct: opaque keys are only supported when USE_PSA is enabled. In current development, mbedtls_pk_setup_opaque() is guarded by MBEDTLS_USE_PSA_CRYPTO and I don't think that should change. That leaves us with a simpler rule: when USE_PSA is off, PK does not use PSA Crypto APIs, period.

[mpg]

I think we want MD_OR_USE_PSA_INIT() here: PSA needs to be initialized if USE_PSA is on, but also, if MD is going to do any hashing via PSA.

[mpg]

@tomi-font Thanks for the summary! Form looking at the code, everything you did in the first three commits looks very reasonable and absolutely going in the right direction. A shame the CI doesn't agree :)

Some of the later changes aimed at pacifying the CI are going in the wrong direction as you pointed out (moving back from USE_PSA to PSA_CRYPTO_C for dispatch decisions) and as Gilles pointed out (moving from MD_CAN dependencies to SHAxxx_C dependencies, or adding USE_PSA dependencies to TLS 1.3 tests), but I understand that's mostly exploratory, and it's useful information to know that the CI passed fully on b460e02.

Unless I'm missing something, I can't easily access the CI results for cead0db, so I'm not sure exactly what was failing before the attempts to fix the failures. I'll push a temporary branch with that commit in order to get that info.

[mpg]

the TLS 1.3 code always uses PSA APIs (regardless of MBEDTLS_USE_PSA_CRYPTO).

That's not quite the whole story. The TLS 1.3 code requires PSA Crypto APIs to be available, and always uses them for some operations. However, for other operations, it will use either PSA or legacy APIs depending on whether USE_PSA is enabled. See "Scope" in the detailed USE_PSA documentation.

However, your point on testing stands: TLS 1.3 should work as soon as PSA_CRYPTO_C is enabled (and psa_crypto_init() has been called), and TLS 1.3 test should not need to depend on USE_PSA except of course if they're using something special like opaque keys.

[mpg]

I've been looking into the TLS 1.3 failures. Perhaps there are multiple ones with different root causes, but the first I ran into is caused by rsa_rsassa_pss_sign() being called on a context (key) that has ctx->padding != MBEDTLS_RSA_PKCS_V21 and returning MBEDTLS_ERR_RSA_BAD_INPUT_DATA which ultimately makes the handshake fail.

When USE_PSA if on, the problem goes away because the key is exported before being re-imported in PSA which in effect loses that padding setting. The unit tests with pk_sign_ext in test_suite_pk pass because the test function sets the padding.

However, with TLS 1.3 we can't add a new requirement on users that "when USE_PSA is off, you need to set padding to PKCS_V21 on your RSA keys" because that would be a compatibility break: things used to work (as evidenced by the tests).

I can see three possible ways out of this:

1. TLS 1.3 makes a (long-term) copy of the key with padding set to V21;
2. PK makes a temporary copy of the key with padding set to V21 just before calling mbedtls_rsa_rsassa_pss_sign();
3. we modify rsa.c to remove the offending check, if we can agree that it's superfluous (not sure).

Rejected ideas:

• we modify the padding bit permanently in TLS 1.3: doesn't really work as the key could be shared with 1.2
• we modify the padding bit temporarily in PK: not thread-safe

[gilles-peskine-arm]

with TLS 1.3 we can't add a new requirement on users that "when USE_PSA is off, you need to set padding to PKCS_V21 on your RSA keys" because that would be a compatibility break: things used to work (as evidenced by the tests).

Arguably the fact that PSS was possible with a key that wasn't explicitly configured for PSS (hence, by default, was configured for v1.5) was a policy violation, hence a bug which we should fix.

But I'm not sure, because the legacy API isn't clear on what is normal use of the API and what is a policy guarantee.

Does TLS 1.3 need RSA keys to work with both v1.5 and PSS?

[mpg]

Arguably the fact that PSS was possible with a key that wasn't explicitly configured for PSS (hence, by default, was configured for v1.5) was a policy violation, hence a bug which we should fix.

But I'm not sure, because the legacy API isn't clear on what is normal use of the API and what is a policy guarantee.

I wasn't sure either, for the same reason, so I checked if rsa.h says anything about it and to my surprise it does:

The choice of padding mode is strictly enforced for private key operations, since there might be security concerns in mixing padding modes. For public key operations it is a default value, which can be overridden by calling specific mbedtls_rsa_rsaes_xxx or mbedtls_rsa_rsassa_xxx functions.

So, that doesn't support the kind of solution I was suggesting, and instead rather supports your position that the fact that it worked at all may be considered a bug.

Does TLS 1.3 need RSA keys to work with both v1.5 and PSS?

No, 1.3 only uses PSS for handshake signatures (v1.5 is still listed but can only be used inside cert chains).

But I guess there is a natural (though arguably misguided on security grounds) desire to share RSA keys between 1.2 (which uses 1.5 only) and 1.3 (which uses PSA only) - at least, I think our tests heavily rely on that.

Also, PK currently doesn't have a very good API for changing the padding mode on RSA keys (you need to use mbedtls_pk_rsa() and call an mbedtls_rsa_ function).

So, I think changing the behaviour of mbedtls_pk_sign_ext() (hence TLS 1.3) so that it respects ctx->padding would quite a lot of work, that we shouldn't add to the scope of this PR. I think as a first step, the !USE_PSA behaviour in this PR should mimic the current behaviour (I'd say by making a temporary copy of the key with the padding changed) and then if we want to change the behaviour for stricter policy compliance, that should be a follow-up (series of) PR(s).

Wdyt?

[tomi-font]

Thanks for your insights and reactivity!
Especially since being new to all this, I lack some understanding and some of what I did (particularly when attempting to fix the tests) is more akin to workarounds than proper fixes.

cbd3d3a doesn't look right either: if MBEDTLS_USE_PSA_CRYPTO is off, then pk.c should not use any PSA APIs except to support opaque keys (MBEDTLS_PK_OPAQUE). The code for MBEDTLS_PK_RSA should not use PSA APIs when MBEDTLS_USE_PSA_CRYPTO is disabled.

I made the change from PSA_CRYPTO_C to USE_PSA_CRYPTO in my implementation commit in an attempt to completely eliminate the PK module's dependencies on PSA_CRYPTO_C.
But the reversing commit you are refering to was to get rid of the following compilation failures in the CI results of b24bfd372:

psa_crypto_rsa.c: In function 'mbedtls_psa_rsa_load_representation':
psa_crypto_rsa.c:83:13: error: implicit declaration of function 'mbedtls_pk_parse_key' [-Werror=implicit-function-declaration]
mbedtls_pk_parse_key(&ctx, data, data_length, NULL, 0,
^
psa_crypto_rsa.c:87:13: error: implicit declaration of function 'mbedtls_pk_parse_public_key' [-Werror=implicit-function-declaration]
mbedtls_pk_parse_public_key(&ctx, data, data_length));
^
psa_crypto_rsa.c: In function 'mbedtls_psa_rsa_export_key':
psa_crypto_rsa.c:187:15: error: implicit declaration of function 'mbedtls_pk_write_key_der' [-Werror=implicit-function-declaration]
ret = mbedtls_pk_write_key_der(&pk, data, data_size);
^
psa_crypto_rsa.c:189:15: error: implicit declaration of function 'mbedtls_pk_write_pubkey' [-Werror=implicit-function-declaration]
ret = mbedtls_pk_write_pubkey(&pos, data, &pk);
^

I have some doubts on how doable this change actually is as the dependencies come from elsewhere than just the PK module. At least it seems that the original comment would need to be fixed.

Unless I'm missing something, I can't easily access the CI results for cead0db, so I'm not sure exactly what was failing before the attempts to fix the failures. I'll push a temporary branch with that commit in order to get that info.

At some point I rebased my branch to make sure I wouldn't fall too out of sync and in case some magic change would have fixed the issues. I think that's what caused the CI results to be wiped.

I've been looking into the TLS 1.3 failures. Perhaps there are multiple ones with different root causes, but the first I ran into is caused by rsa_rsassa_pss_sign() being called on a context (key) that has ctx->padding != MBEDTLS_RSA_PKCS_V21 and returning MBEDTLS_ERR_RSA_BAD_INPUT_DATA which ultimately makes the handshake fail.

When USE_PSA if on, the problem goes away because the key is exported before being re-imported in PSA which in effect loses that padding setting. The unit tests with pk_sign_ext in test_suite_pk pass because the test function sets the padding.

Indeed I stumbled upon this ctx->padding issue and worked my way around it but didn't think further about potential issues.
At least I am quite confident that all the failures only happen in configurations where MBEDTLS_PSA_CRYPTO_C && !MBEDTLS_USE_PSA_CRYPTO.

I think as a first step, the !USE_PSA behaviour in this PR should mimic the current behaviour

What current behavior? The fact that a different (and supposedly incompatible) padding mode is accepted?

Wdyt?

I agree with not making the scope of this PR bigger if possible.
If the suggested temporary copy is lightweight (just a copy of struct bytes?) then it seems like a good temporary solution to me.
Though would also have to see what the CI has to say then.

[gilles-peskine-arm]

So, I think changing the behaviour of mbedtls_pk_sign_ext() (hence TLS 1.3) so that it respects ctx->padding would quite a lot of work, that we shouldn't add to the scope of this PR. I think as a first step, the !USE_PSA behaviour in this PR should mimic the current behaviour (I'd say by making a temporary copy of the key with the padding changed) and then if we want to change the behaviour for stricter policy compliance, that should be a follow-up (series of) PR(s).

I'm still thinking about this. I'm bothered by the API, not just by the implementation. Whether we make a copy of the key, or relax the algorithm check in the low-level module, we're building an API that relies on cheating a key's policy. e6d1d82 chose to relax the algorithm check for PSS verification, which allowed 20422e9 to implement pk_verify_ext. We're now building a psa_sign_ext on the same principle, but is that API design really a good idea?

Enforcing key policies in a library isn't critical by itself since the caller can just access the content of the key. It only becomes a security restriction in the application as a whole or if there's a security boundary. The built-in RSA implementation doesn't have a security boundary. On the other hand, the PSA API can be implemented with a security boundary (and so can the legacy API but only with MBEDTLS_PK_RSA_ALT, but keys of that type declare that they can't do PSS so the issue is moot).

With respect to considering the application as a whole, I don't really agree with the implicit reasoning in e6d1d82 that it's ok to override the configuration of the key. Doing that when not intended can compromise an application if it starts accepting signatures that it didn't intend to accept. Even if it isn't opening a security hole by itself, it's at least potential for misuse. I don't think I would have agreed to the design of mbedtls_pk_verify_ext. So I'm afraid I can't agree with the proposed design of mbedtls_pk_sign_ext which this pull request would commit into the API.

I'm still thinking about this, but currently I see two solutions:

• We decide that it's not the job of the RSA module to enforce the algorithm usage after all. We add warnings to the documentation and we remove the padding mode checks from at least rsa_rsassa_pss_sign.
• We add a function to the PK API that allows users to configure an RSA key's policy (and also, possibly, to configure an ECDSA key's policy to decide between deterministic and randomized ECDSA?).

[mpg]

@tomi-font Since this PR looks like it's going to be merged soon, I wanted to let you know that I've created #8647 which might be of interest to you (together with this PR, it should fully resolve #7126) if you'd like to make further contributions to Mbed TLS - which would be very much appreciated!

[mpg]

Ok, so now we have two approvals and a green CI, but unfortunately conflicts have appeared after #8579 was merged. @tomi-font Can you fix them? Either by rebasing on top of recent development, or merging development into your branch, whatever works best for you.

[tomi-font]

Yep, I noticed. I rebased on top of the latest development and force pushed to fix the conflicts in pk_internal.h and pkparse.c.
What's gonna come up next? 😅

[mpg]

The rebase and conflict resolution look good to me.

[mpg]

What's gonna come up next? 😅

I've set up auto-merge when ready, so if Valerio is happy with the rebase too, and the CI comes back green, this will be immediately added to the merge queue, to reduce risk of further conflicts!

We'll see 🤞

[valeriosetti]

LGTM! 🤞