Remove all non ext apis

[bjwtaylor]

Description

Remove all non ext apis, resolves #8135 depends Mbed-TLS/TF-PSA-Crypto#499. Currently not including mbedtls_x509_get_subject_alt_name removal, as this is under discussion.

This PR is part of a multistage PR to be merged in the following order:

1. Remove all non ext apis #10439
2. Remove all non ext apis TF-PSA-Crypto#499

PR checklist

• changelog not required because: No public changes
• development PR provided #HERE
• TF-PSA-Crypto PR provided Remove all non ext apis TF-PSA-Crypto#499
• framework PR not required
• 3.6 PR not required because: No backports
• tests not required because: No changes

[valeriosetti]

The first 2 commits' titles say that they are replacing mbedtls_pk_sign with mbedtls_pk_sign_ext, but in facts they are replacing with mbedtls_pk_sign_restartable.
Looking also at the description of the original issue wasn't _ext the expected replacement?

[bjwtaylor]

@valeriosetti, this is a good spot. I originally looked at the non ext functions as saw they were just calling the restartable versions, so converted them as I suspect there was in issue converting them to ext versions. Let me investigate further though and come back to you.

[valeriosetti]

In case you need to find all non-replaced occurrences of _ext() functions I made this bash script:

#!/bin/bash

MATCHES=$(rg --no-filename --no-line-number -g '*.c' -oe 'mbedtls_\w+_ext\(' | sort -u | sed 's/_ext(//')
IGNORE=

for match in $MATCHES; do
    echo "=== Checking $match === "
    rg -wo $match library/ include/
done

It looks for all functions that have an _ext() alternative and for each of them it searches for all non-_ext() occurrences in library/ (I guess something similar should then be done for tf-psa-crypto in Mbed-TLS/TF-PSA-Crypto#499.

For the records here a list of the non-replaced functions found by this script:

• mbedtls_pk_can_do: ideally there is mbedtls_pk_can_do_psa. Replacement is possible even though it make take a bit to do it properly. I don't know if the scope of this PR is large enough to try this here or post-pone it to a different PR...
• mbedtls_pk_sign/mbedtls_pk_verify: these still appears in a couple of log messages;
• mbedtls_ssl_write_handshake_msg: should we replace it?
• mbedtls_x509_get_subject_alt_name: intentionally excluded according to the description of this PR, so this is fine.

[bjwtaylor]

@valeriosetti So for mbedtls_pk_can_do, we've agreed to postpone it as I believe we maybe need to change it to the psa form instead of mbedtls_pk_can_do_ext.

The reason that I used the restartable version of mbedtls_pk_sign/mbedtls_pk_verify is that the original function was a passthrough for these. The ext version requires that you pass in mbedtls_pk_sigalg_t pk_type and as far as I can tell this is only stored in the private struct mbedtls_pk_info_t, or at least the mbedtls_pk_type_t version is. Which is buried in the mbedtls_pk_context. We have an access function pk_get_type_ext(const mbedtls_pk_context *pk). However this is also private as it is not declared outside of pkwrite.c. Do you know the plan around these functions? Is the API for example expecting you to pass it down through the stack? Or are these ones best left as they are for now?

Thanks for the script, I'll go through and take a look at the other ext functions.

[valeriosetti]

The ext version requires that you pass in mbedtls_pk_sigalg_t pk_type and as far as I can tell this is only stored in the private struct mbedtls_pk_info_t, or at least the mbedtls_pk_type_t version is. Which is buried in the mbedtls_pk_context

Since mbedtls_pk_sign_ext() is a public function it should be possible to call it without accessing any PK internal. In fact the 1st parameter, i.e. mbedtls_pk_sigalg_t sig_type, is an entry of enum mbedtls_pk_sigalg_t which is public as well. But I agree that the problem is that there is no way to tell which enum mbedtls_pk_sigalg_t should be used in the call to mbedtls_pk_sign_ext() (unless this can be determined by some other TLS protocol detail) and brute forcing all options doesn't sound like an elegant idea.
TBH I don't know if replacing mbedtls_pk_sign() with the restartable counterpart is a good idea since then you are not implementing restartable management on the caller code. I think that leaving the calls as they were is the best option in this case, but let's wait for the 2nd reviewer's opinion.

[gilles-peskine-arm]

Please don't replace mbedtls_pk_{sign,verify} with their restartable variants: that doesn't actually improve anything. The reason the non-ext functions are deprecated is that they use whatever algorithm they think the key is for, instead of using the algorithm that the caller intends. This corresponds to how PK contexts worked in earlier library versions, but not how it works in TF-PSA-Crypto 1.0, and even before it lead to bugs where the wrong algorithm was used.

To improve things, we need to replace sign/verify with the ext variant, where the caller specifies which algorithm it wants. The caller should always know.

the problem is that there is no way to tell which enum mbedtls_pk_sigalg_t should be used in the call to mbedtls_pk_sign_ext() (unless this can be determined by some other TLS protocol detail)

Yes, exactly: the X.509 or TLS protocol says which algorithm to use. When our code says “use whatever algorithm the key was set up for”, that's potentially wrong.

Practically, things are different for RSA and ECC. For RSA, there are two completely different signature/verification algorithms (PKCS#1v1.5 and PSS), and the caller always knows which one to use because it's dictated by the protocol. For ECC, there are two variants of ECDSA (deterministic and randomized) which are functionally equivalent, but the signature operation has different security properties, and the caller needs to use whichever variant the key allows. Generally that will be MBEDTLS_PK_ALG_ECDSA, but it's also ok to try both (because they're functionally equivalent).