mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

RSA keys set to PSS/OAEP padding perform PKCS1v1.5 when MBEDTLS_USE_PSA_CRYPTO is enabled

Open gilles-peskine-arm opened this issue 1 year ago • 4 comments

In the default configuration, if you have an mbedtls_pk_context of type MBEDTLS_PK_RSA, and the underlying RSA context has its padding mode set to MBEDTLS_RSA_PKCS_V21, then the functions mbedtls_pk_sign, mbedtls_pk_verify, mbedtls_pk_encrypt and mbedtls_pk_decrypt perform PSS or OAEP. If the padding mode is left at the default MBEDTLS_RSA_PKCS_V15, then those functions perform a PKCS#1v1.5 algorithm. This has been the case since the PolarSSL days.

(There are functions mbedtls_pk_sign_ext and mbedtls_pk_verify_ext that bypass the underlying context's padding mode, but they aren't relevant to the behavior of the non-ext function.)

This is not clearly documented, but it's a reasonable expectation. The documentation of the pk functions state that “For RSA keys, the default padding type is PKCS#1 v1.5.” That's the default if you construct a key with mbedtls_pk_parse_xxx, but if you manipulate the underlying RSA context, it changes the algorithm.

Bug: if you enable MBEDTLS_USE_PSA_CRYPTO, then since #5519 released in Mbed TLS 3.2, the PK module dispatches to PSA and forces the PKCS1 algorithm (PSA_ALG_RSA_PKCS1V15_CRYPT or PSA_ALG_RSA_PKCS1V15_SIGN(hash_alg), and we're either missing a case for PSA_ALG_RSA_PKCS1V15_SIGN_RAW or it's happening fortuitously due to the encoding of psa_algorithm_t).

Reproducer: https://github.com/gilles-peskine-arm/mbedtls/commit/5e94eb3fc190e6429b1085ede9a031b8f0481f5c in https://github.com/gilles-peskine-arm/mbedtls/tree/pk-rsa-test-padding. When MBEDTLS_USE_PSA_CRYPTO is enabled, the new PSS test cases fail.

gilles-peskine-arm avatar Feb 13 '24 19:02 gilles-peskine-arm

Note: it's probably obvious, but while fixing this we should improve the documentation of sign, verify, decrypt, encrypt in order to explicitly document this.

mpg avatar Feb 14 '24 08:02 mpg

Question: new test cases added by @gilles-peskine-arm on his branch check verify not sign. However looking at pk_wrap.c it seems to me that also rsa_sign_wrap() in the USE_PSA case needs adjustments, right?

valeriosetti avatar Feb 14 '24 10:02 valeriosetti

Yes, I think all 4 functions (sign, verify, decrypt, encrypt) need new tests and corresponding adjustments - I guess Gilles only added tests for 1 of them as a proof of concept.

mpg avatar Feb 14 '24 10:02 mpg

Yes, all 4 functions are affected. I just wanted to confirm the bug and only had time to do one function.

To test mbedtls_pk_sign and mbedtls_pk_encrypt, which are randomized, it's sufficient to test the verify/decrypt functions with known data, and separately test that the sign/verify and encrypt/decrypt functions work when paired together.

gilles-peskine-arm avatar Feb 14 '24 11:02 gilles-peskine-arm