Require matching hashlen in RSA functions: implementation #4707
Conversation
Hashes used in RSA-PSS encoding (EMSA-PSS-ENCODE, see §9.1.1 in RFC 8017): - H1: Hashing the message (step 2) - H2: Hashing in the salt (step 6) - H3: Mask generation function (step 9) According to the standard: - H1 and H2 MUST be done by the same hash function - H3 is RECOMMENDED to be the same as the hash used for H1 and H2. According to the implementation: - H1 happens outside of the function call. It might or might not happen and the implementation might or might not be aware of the hash used. - H2 happens inside the function call, consistency with H1 is not enforced and might not even be possible to detect. - H3 is done with the same hash as H2 (with the exception of mbedtls_rsassa_pss_verify_ext(), which takes a dedicated parameter for the hash used in the MGF). Issues with the documentation: - The comments weren't always clear about the three hashes involved and often only mentioned two of them (which two varied from function to function). - The documentation was giving the impression that the standard recommends aligning H2 and H1 (which is not a recommendation but a must). Signed-off-by: Janos Follath <janos.follath@arm.com>
PSA Crypto always passed MBEDTLS_MD_NONE to Mbed TLS, which worked well as Mbed TLS does not use this parameter for anything beyond determining the input lengths. Some alternative implementations however check the consistency of the algorithm used for pre-hash and for other uses in verification (verify operation and mask generation) and fail if they don't match. This makes all such verifications fail. Furthermore, the PSA Crypto API mandates that the pre-hash and internal uses are aligned as well. Fixes Mbed-TLS#3990. Signed-off-by: Janos Follath <janos.follath@arm.com>
The psa_verify_hash() is the pre-hashed version of the API and supposed to work on hashes generated by the user. There were tests passing that were getting "hashes" of sizes different from the expected. Transform these into properly failing tests. Signed-off-by: Janos Follath <janos.follath@arm.com>
Hash and sign algorithms require the alignment of the input length with the hash length at verification as well not just when signing. Signed-off-by: Janos Follath <janos.follath@arm.com>
Signed-off-by: Janos Follath <janos.follath@arm.com>
Remove a case that cannot be triggered as PSA_ALG_SIGN_GET_HASH always returns 0 for raw algorithms. Signed-off-by: Janos Follath <janos.follath@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Where hashlen was previously ignored when the hash length could be inferred from an md_alg parameter, the two must now match. Adapt the existing tests accordingly. Adapt the sample programs accordingly. This commit does not add any negative testing. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
enhancement
component-crypto
gilles-peskine-arm
added
needs-preceding-pr
There was a problem hiding this comment.
This looks good to me.
I'm closing #4702 in favour of this PR in order to avoid diluting or review effort and having dependency chains, as this PR is no more risky than #4702 considering it passed CI.
I'm not closing #4665 now because it has an associated 2.x backport.
We should probably also close #4561 as the changes in this PR are a superset what it does, but someone should probably still backport the relevant parts of #4561 to 2.x and 2.16. In the interest of speed, I suggest you do it rather than the original contributor. If you agree, please close #4561 and let its author know what's going on.
| @@ -2031,6 +2032,8 @@ static int rsa_rsassa_pkcs1_v15_encode( mbedtls_md_type_t md_alg, | |||
| * TAG-NULL + LEN [ NULL ] ] | |||
| * TAG-OCTET + LEN [ HASH ] ] | |||
| */ | |||
| if( 0x08 + oid_size + hashlen >= 0x80 ) | |||
There was a problem hiding this comment.
Note: I'm not sure how that is related to the hashlen work. I this is an independent hardening, we should probably backport it?
There was a problem hiding this comment.
Now that the functions have a clearly defined input length, I wanted to make the code robust to changes that could result in accepting different input lengths (for example, one day this code might run on a long SHAKE output). That meant checking that hashlen fits, regardless of any assumption we might make on hashes being small enough.
Indeed I think this change should be backported.
There was a problem hiding this comment.
The backports don't need to hold up the upcoming release. Filed as a Q3 tech debt item in #4722
There was a problem hiding this comment.
Hmm
Should this check be backported?
If md_alg == MBEDTLS_MD_NONE then we never reach it because we've already returned (in 2019), and if md_alg != MBEDTLS_MD_NONE then we have already performed this check in 1974, and neither hashlen nor oid_size have changed since.
Have I missed something here?
There was a problem hiding this comment.
I (and the reviewers of this PR) missed that the check was already done earlier. So instead of backporting it, please make a PR on development to remove the redundant check!
There was a problem hiding this comment.
As a matter of fact, I noticed the check was already done earlier but noticed it was inside a conditional, and didn't go to the next step and failed to notice that this path was always taken if we ever get to the point where the new check was added. Should probably have said something, though. Thanks @davidhorstmann-arm for noticing.
| @@ -611,7 +612,8 @@ void pk_sign_verify( int type, int parameter, int sign_ret, int verify_ret ) | |||
| { | |||
| mbedtls_pk_context pk; | |||
| size_t sig_len; | |||
| unsigned char hash[MBEDTLS_MD_MAX_SIZE]; | |||
| unsigned char hash[32]; // Hard-coded for SHA256 | |||
There was a problem hiding this comment.
tom-daubney-arm
removed
the
needs-reviewer
Remove a check in rsa_rsassa_pkcs1_v15_encode() that is not needed because the same check is performed earlier. This check was added in Mbed-TLS#4707. Signed-off-by: David Horstmann <david.horstmann@arm.com>
RSA and PK signature function that take both a
hashparameter and ahashlenorhash_lenparameter now require the length parameter to be the length of the input. If there is additional information about the hash algorithm and the length does not match, the functions return an error instead of silently ignoring the length parameter.Continuation of #4665 and #4702, will need to be rebased when #4665 is merged.