Backport 2.16: pk.c: Ensure min hash_len in pk_hashlen_helper #4719
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nick-child-ibm
changed the title
yanesca
added
Community
needs-review
gilles-peskine-arm
changed the title
| return( 0 ); | ||
|
|
||
| if( ( md_info = mbedtls_md_info_from_type( md_alg ) ) == NULL ) | ||
| return( -1 ); | ||
|
|
||
| if ( *hash_len != 0 && *hash_len < mbedtls_md_get_size( md_info ) ) |
There was a problem hiding this comment.
This guarantees that signature functions won't do a buffer overread when called with hash_len = size of the hash buffer and hash_len is less than the size of the hash indicated by md_alg. But when hash_len is larger, the last bytes are still ignored. So even in an LTS, it might make sense to go all the way?
There was a problem hiding this comment.
I believe the concern carried over from #4561 was that many current implementations and even the mbedtls test suites allocate MBEDTLS_MD_MAX_SIZE bytes for the hash buffer and for the hash_len. If we required the exact expected hash length then all of these use cases would fail and need to be updated. Let me know what you think.
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" Signed-off-by: Nick Child <nick.child@ibm.com> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
nick-child-ibm
force-pushed
the
2.16-hashlen
branch
from
01cc2f3 to
c15e31d
Compare
|
Rebased to update changelog. Request made from #4718 comment |
gilles-peskine-arm
approved these changes
Jun 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #4561 . Related to other backport for
development_2.x#4718.Very similar except rather than requiring the given
hashlento be equal tombedtls_md_get_size( md_info ), it must be at leastmbedtls_md_get_size( md_info ). This will not break the currentpktests so no commits were added like they were in #4561 and #4718 to address test failures.