Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.02k stars 2.5k forks source link

Implementation of RSA PSS signing and verification not strictly compliant with RFC #9303

Closed xinyufort closed 1 week ago

xinyufort commented 1 week ago

Please note that this was previously discussed in an email thread with the Mbed TLS security team, and was deemed not security-sensitive. I am opening this GitHub issue for tracking purposes.

Summary

Mbed TLS has a non-compliant implementation of EMSA-PSS-ENCODE (encoding step used in PSS signing) and EMSA-PSS-VERIFY (analogous step during PSS signature verification). Fortunately, this only occurs in corner cases where users try to specify parameter combinations that are allowed, but not RECOMMENDED (RFC 2119 semantics) by RFC 8017 (PKCS #⁠1: RSA Cryptography Specifications Version 2.2). In those specific cases, this results in non-interoperability with OpenSSL.

Specifically, when signing and verifying signatures,

The Mbed TLS PSS signing implementation uses two hash algorithms: md_alg, which I presume is the algorithm used to hash the original message (step 2 in both EMSA-PSS-ENCODE and EMSA-PSS-VERIFY), and ctx->hash_id, which is used for everything else. (The latter is normally set via a call to mbedtls_rsa_set_padding.) The deviation from the RFC occurs when users configure these two algorithms to differ from one another, since Mbed TLS will, instead of using md_alg for step 6 of EMSA-PSS-ENCODE, use ctx->hash_id instead.

(Aside note: ctx->hash_id is also used as part of the MGF1 hash function (step 9 of EMSA-PSS-ENCODE). While the RFC RECOMMENDS (RFC 2119 semantics) that all hash functions involved in the encoding process be the same (in steps 2, 6, and 9), the Mbed TLS implementation not only does not enforce this rule, but also allows the hash algorithms in steps 2 and 6 to differ (md_alg vs ctx->hash_id), which deviates from the algorithm as described in the RFC.)

Likewise, we see a similar issue with signature verification -- specifically, when doing EMSA-PSS-VERIFY. Here, the two hash algorithms in question are md_alg and mgf1_hash_id. Step 13 of EMSA-PSS-VERIFY is performed using mgf1_hash_id as opposed to md_alg.

System information

Mbed TLS version (number or commit id): 3.60 and 2.28.0 Operating system and version: Ubuntu 20.04 Configuration (if not default, please attach mbedtls_config.h): default configuration, unmodified from source Compiler and options (if you used a pre-built binary, please indicate how you obtained it):

Expected behavior

I would have expected one of the following:

Actual behavior

Mbed TLS allows the hash algorithm used to originally hash the data (passed in via the md_alg argument) to differ from the hash algorithm used to perform step 6 in EMSA-PSS-ENCODE or step 13 in EMSA-PSS-VERIFY, which is not allowed by the RFC spec. This leads to non-interoperability with OpenSSL (which also allows users to pass in two different hash algorithms when performing PSS signing or verification).

Steps to reproduce

For brevity, I am only describing the steps to generate a non-compliant signature on 3.6 code. But a similar process applies for 2.28 code, and also for signature verification.

Note that if you modify line 2197 in rsa.c to use md_alg as the last argument to hash_prime (instead of hash_id), and re-run the above steps, OpenSSL will successfully validate the signature.

gilles-peskine-arm commented 1 week ago

As discussed in email:

As a consequence, we don't consider it a bug that Mbed TLS allows different hash algorithms for the message processing and the hash+salt hashing. At most, we might add a note to the documentation.

gilles-peskine-arm commented 1 week ago

As for the fact that you can't use a different hash for the hash+salt hashing step and for MGF1, this is a limitation, but we aren't going to lift it unless there is a concrete use case. If we do lift it, it would have to be through the PSA API, not through rsa.h, since this will no longer be a public API.

gilles-peskine-arm commented 1 week ago

I am marking this issue as completed because I don't think we need to take any action about it. But if you think it's important to add a warning to the documentation, please make a pull request.