pk.c: Ensure min hash_len in pk_hashlen_helper

[nick-child-ibm]

Hello,
This Pull Request is to patch an area of confusion on pk_sign and pk_verify functions. Both take in an argument hash_len, in the API docs, it is noted that:

"If hash_len is 0, then the length associated with md_alg is used instead, or an error returned if it is invalid"

I have run into issues where I passed the incorrect value to hash_len and never ran into any "errors". I am sure most users are smarter than I and won't make such foolish mistakes but I do believe that either the documentation or function should be updated to match the other. In this PR, I have added a commit that makes the pk_verify/sign behave as specified by the API docs.

Currently, the hash_len args in these functions are behaving more so like the description in rsa...verify/sign documentation. here it lists that hashlen:

is only used if md_alg is MBEDTLS_MD_NONE. "

If this is intentional than I think it could be worth updating the documentation of pk_verify/sign. If not, then if hash_len is passed in as not zero then I believe it should at least be examined to be a valid hash length for the given hash algorithm. In the later called functions, if md_alg is not MD_NONE, the hash_len is overwritten anyway. So, if the user passed in a hash_len that is less than the associated md, an error should be returned in order to avoid invalid reads by reaching the end of the buffer.

I hope this is useful in some way.
I would like to thank @naynajain for bringing this to my attention and helping me out.

Notes:

• This commit throws an error if the user gives a hash_len that is LESS THAN the expected. Test suites will fail for test_suite_pk if we throw errors if the hash_len is NOT EQUAL to the expected. This is because these test suites allocate and use MBEDTLS_MD_MAX_SIZE as their hash lengths.
• I have looked over the relevant rsa...verify/sign() functions called by the ->verify/sign_func() function pointers, they all seem to overwrite the hash_len so it may be worth removing *hash_len = mbedtls_md_get_size( md_info ); in pk_hashlen_helper. I did not remove it in this PR since I do not know the behavior of other functions pointed to by ->verify/sign_func() and it is a very unimportant thing to change.

Description

From commit message:

The function pk_hashlen_helper exists to ensure a valid hash_len is
used in pk_verify and pk_sign functions. This function has been
used to adjust to the corrsponding hash_len if the user passes in 0
for the hash_len argument based on the md algorithm given. If the user
does not pass in 0 as the hash_len, then it is not adjusted. This is
problematic if the user gives a hash_len and hash buffer that is less than the
associated length of the md algorithm. This error would go unchecked
and eventually lead to buffer overread when given to specific pk_sign/verify
functions, since they both ignore the hash_len argument if md_alg is not MBEDTLS_MD_NONE.
This commit, adds a conditional to pk_hashlen_helper so that an
error is thrown if the user specifies a hash_length (not 0) and it is
less than the expected for the associated message digest algorithm.
This aligns better with the api documentation where it states "If
hash_len is 0, then the length associated with md_alg is used instead,
or an error returned if it is invalid"

Status

READY

Requires Backporting

Not sure if reading past a buffer can be considered a bug so probably not.
NO

Migrations

NO

Steps to test or reproduce

Run valgrind on any mbedtls_pk_verify/read command where the following conditions for arguments are met:

1. md_alg is an actual hash function (not MBEDTLS_MD_NONE)
2. hash_len is less than md_algs associated length
3. hash is only allocated to have hash_len bytes

[mpg]

Thanks for your contribution! This seems related to #3990 on which @yanesca is already working, so perhaps you should coordinate on how to fix those two issues?

[yanesca]

@nick-child-ibm Thank you for your contribution! As @mpg mentioned, this issue is similar to #3990, but don't worry it is in a parallel API and the two fixes won't interfere with each other.

[nick-child-ibm]

@yanesca Thanks for reaching out! I am guessing we should probably make sure that all mbedtls_...._verify/sign operations treat hash_len and md_alg in a similar style. After looking at your issue and patch it seems you are in the boat that believes that hash_len should accurately reflect the length of the given hash and it should be exactly equal to mbedtls_md_get_size( md_info ). Please correct me if I have misunderstood. I am also seeing that you have edited the tests so that if the hash_len is given 0 and the md_alg is SHA256, then it should fail. So if hash_len is zero and an md_alg is given, do you recommend an error is thrown rather than an assuming the hash length from the given md_alg. Let me know what your thoughts are and I can make adjustments to my commit to better align with yours.

[yanesca]

@nick-child-ibm That is correct. Your PR is already an improvement to what we have, but as you said, ideally we would return an error if the hash length and the hash algorithm are not consistent.

[nick-child-ibm]

Rebased to require that the user supplied hash_len is equal to the length of the the message digest algorithm output. Previously, the condition was that hash_len had to be at least the length of the message digest.
As previously mentioned, this is going to cause tests to fail, going to fix tests now to supply the correct arguments to the function

[nick-child-ibm]

Added commit to allow tests to pass by not using MBEDTLS_MD_MAX_SIZE as hash_len argument to pk_verify/sign function calls.

[mpg]

Looks good to me, thanks for this fix!

[mpg]

Not sure if reading past a buffer can be considered a bug so probably not.

I think reading pass the end of a buffer is absolutely a bug. It could at least cause a crash or worse (buffer over-read comes with a risk of leaking secret data, though in that case I think we're fine).

So I think this should be backported. Currently the two active branches are development_2.x the the LTS branch 2.16. Just to be on the safe side, I think for 2.16 we should probably use the first version of your patch: the one that only returned an error if hashlen is less than the expected - that way, we reduce the risk of compatibility issues in case someone passed a value larger than necessary, as we did in the test suite.

Could you create those backports?

[mpg]

Nearly forgot: this needs a ChangeLog entry (see ChangeLog.d/00README.md, under "Bugfix".

[nick-child-ibm]

Rebase adds changelog

[nick-child-ibm]

Rebase fixes changelog space at EOL

[nick-child-ibm]

Closing since covered by #4707