PK: RSA-PSS verification

[mpg]

Make RSA-PSS signature verification in the PK layer use PSA transparently when MBEDTLS_USE_PSA_CRYPTO is enabled and the requested parameters match what's expected by PSA_ALG_RSA_PSS.

In mbedtls_pk_verify_ext(), create a new code path that's guarded, at compile-time by MBEDTLS_USE_PSA_CRYPTO, and at run-time by whether the options match what PSA_ALG_RSA_PSS that uses psa_verify_hash() instead of mbedtls_rsa_rsassa_pss_verify_ext(). That is, the body of the if( type == MBEDTLS_PK_RSASSA_PSS ) should look like:

/* (common checks such as options != NULL) */
#if defined(MBEDTLS_USE_PSA_CRYPTO)
if( pss_opts->mgf1_hash_id == md_alg && 
    pss_opts->expected_salt_len == hash_len ) // see note 1 below
{
    /* (path using psa_verify_hash()) */
}
else
#endif
{
    /* (path using mbedtls_rsa_rsassa_pss_verify_ext() */
}

The path using mbedtls_rsa_rsassa_pss_verify_ext() remains the same as the existing code. On the path using psa_verify_hash() we'll need to create a temporary PSA public key of type PSA_ALG_RSA_PSS(hash_alg) with an appropriate policy. An example of this can be found in the MBEDTLS_USE_PSA_CRYPTO definition of ecdsa_verify_wrap() in pk_wrap.c.

Note 1: we may also want to check that hash_len is consistent with md_alg, and take the PSA path only if that's the case.

Note 2: the expected result of this task is that the call to mbedtls_pk_verify_ext() in ssl_tls13_generic.c takes the PSA path. We probably want to verify that with a debugger, a printf or whatever. No other testing is required, except ensuring all existing test keep passing.

Note 3: it is possible to reduce the level of nesting by restructuring mbedtls_pk_verify_ext() to have the non-RSA-PSS case first, which returns early, and then the PSS case no longer needs to be wrapped in an if statement.

[AndrzejKurek]

Should we also redirect MBEDTLS_RSA_SALT_LEN_ANY via PSA_ALG_RSA_PSS_ANY_SALT to the PSA-using path?

[AndrzejKurek]

Also, there's a bit of a problem with error translation:

    /* Mbed TLS distinguishes "invalid padding" from "valid padding but
     * the rest of the signature is invalid". This has little use in
     * practice and PSA doesn't report this distinction. */
    status = ( ret == MBEDTLS_ERR_RSA_INVALID_PADDING ) ?
             PSA_ERROR_INVALID_SIGNATURE :
             mbedtls_to_psa_error( ret );

This way, there are a couple of errors that can end up as PSA_ERROR_INVALID_SIGNATURE, and I couldn't find any good Mbed TLS equivalent to that... Should I use mbedtls_psa_err_translate_pk to translate it to MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED? This seems quite broad.

[gilles-peskine-arm]

“Invalid signature” is a very specific error and we should translate it accurately. Unfortunately in mbedtls it's split between MBEDTLS_ERR_RSA_VERIFY_FAILED and MBEDTLS_ERR_ECP_VERIFY_FAILED. I don't have a good answer to that — create a MBEDTLS_ERR_PK_VERIFY_FAILED?

[gilles-peskine-arm]

Should we also redirect MBEDTLS_RSA_SALT_LEN_ANY via PSA_ALG_RSA_PSS_ANY_SALT to the PSA-using path?

Yes, especially since this is the path that the X.509 code will need since in practice the X.509 world (when it uses PSS at all) often uses a salt length which is not the one PSA_ALG_RSA_PSS requires (see https://github.com/ARMmbed/mbedtls/issues/5277).

[mpg]

“Invalid signature” is a very specific error and we should translate it accurately. Unfortunately in mbedtls it's split between MBEDTLS_ERR_RSA_VERIFY_FAILED and MBEDTLS_ERR_ECP_VERIFY_FAILED. I don't have a good answer to that — create a MBEDTLS_ERR_PK_VERIFY_FAILED?

I think the situation would be better if PK_VERIFY_FAILED existed already, but it doesn't I'm no not sure creating it now is OK, perhaps people are already checking for the specific code.

We could also make the translation context-dependent: if doing an RSA operation, translate PSA_ERROR_INVALID_SIGNATURE to MBEDTLS_ERR_RSA_VERIFY_FAILED, otherwise to MBEDTLS_ERR_ECP_VERIFY_FAILED.

[gilles-peskine-arm]

We could also make the translation context-dependent: if doing an RSA operation, translate PSA_ERROR_INVALID_SIGNATURE to MBEDTLS_ERR_RSA_VERIFY_FAILED, otherwise to MBEDTLS_ERR_ECP_VERIFY_FAILED.

I think we should do that, if we can, yes. But does mbedtls_pk_verify know the key type?

[mpg]

But does mbedtls_pk_verify know the key type?

I think it can if it needs to. But currently the translation happens in the wrappers in pk_wrap.c, where you obviously know the type because if you're in ecdsa_verify_wrap() clearly you're not dealing with RSA.

[gilles-peskine-arm]

@mpg I was thinking of the opaque case.

[mpg]

I don't think we plan to support signature verification with opaque keys so far. Should we?

But even in that case, I think we can look fetch the key's attributes and recover its type. We'd just need to write code for it, it wouldn't come as naturally as the transparent case, but shouldn't be hard unless I'm missing something.

[gilles-peskine-arm]

Why wouldn't we support pk_verify for opaque keys? It'll have to return something when the key type and algorithm are in the vendor range. But we don't need to worry about it now, this might be a post-4.0 thing anyway.

[AndrzejKurek]

@mpg - do you have any suggestion on what form of verification would be most suitable for debugging and making sure that the code is called? What I've done is:

-gnutls-serv --http --disable-client-cert --debug=4 --x509certfile data_files/server2-sha256.crt --x509keyfile data_files/server2.key --priority=NONE:+AEAD:+AES-128-GCM:+GROUP-SECP256R1:+SHA256:+SIGN-RSA-PSS-RSAE-SHA256:+VERS-TLS1.3:%NO_TICKETS

-ssl-client2 server_addr=127.0.0.1 server_port=5556 debug_level=4 force_version=tls13 ca_file=data_files/test-ca_cat12.crt force_ciphersuite=TLS1-3-AES-128-GCM-SHA256 sig_algs=rsa_pss_rsae_sha256 curves=secp256r1

This doesn't reach the pk_verify call with USE_CRYPTO_PSA, but without it, using the debugger, I was able to reach the code path just before the set of if's:

 if( pss_opts->mgf1_hash_id == md_alg &&
        ( (size_t) pss_opts->expected_salt_len == hash_len ||
            pss_opts->expected_salt_len  == MBEDTLS_RSA_SALT_LEN_ANY ) )

and I manually (or rather via the IDE) evaluated the inside of it to 1. Let me know if there's any better way to satisfy the requirement.

[mpg]

Hmm, I didn't realise when I wrote that note that it's currently not possible to complete a TLS 1.3 handshake with USE_PSA_CRYPTO enabled, until #5532 is merged - before that, I think what you did is the best we can do.

So, I merged this PR and 5532 in a local branch, then applied the following patch:

diff --git a/library/pk.c b/library/pk.c
index 4eff8e52cb2f..22e6c6ae6ac4 100644
--- a/library/pk.c
+++ b/library/pk.c
@@ -406,6 +406,7 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options,
             return( mbedtls_psa_err_translate_pk( status ) );
         }

+        puts("\nVERIFY_EXT: psa_verify_hash()");
         status = psa_verify_hash( key_id, psa_sig_md, hash,
                                   hash_len, sig, sig_len );
         psa_destroy_key( key_id );
@@ -420,6 +421,7 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options,
         if( sig_len < mbedtls_pk_get_len( ctx ) )
             return( MBEDTLS_ERR_RSA_VERIFY_FAILED );

+        puts("\nVERIFY_EXT: mbedtls_rsa_xxx()");
         ret = mbedtls_rsa_rsassa_pss_verify_ext( mbedtls_pk_rsa( *ctx ),
                                                  md_alg, (unsigned int) hash_len, hash,
                                                  pss_opts->mgf1_hash_id,

and, after enabling MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE in mbedtls_config.h, ran the following:

make -C programs ssl/ssl_{client,server}2 test/udp_proxy test/query_compile_time_config
tests/ssl-opt.sh -p -f "TLS 1.3 m->O: TLS_AES_128_GCM_SHA256,x25519,rsa_pss_rsae_sha256"
grep VERIFY_EXT tests/o-cli-1.log

first without USE_PSA_CRYPTO then with it, and the results are as expected.

So, between your verification and mine, I think we can be confident that the right code path is used.