[backport 2.7] Fix pk_parse_key()'s use of rsa_complete() #3049
Conversation
This file contains hidden or 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
When parsing a PKCS#1 RSAPrivateKey structure, all parameters are always present. After importing them, we need to call rsa_complete() for the sake of alternative implementations. That function interprets zero as a signal for "this parameter was not provided". As that's never the case, we mustn't pass any zero value to that function, so we need to explicitly check for it.
- remove incorrect compile-time dependency (the individual cases already have correct run-time dependency information) - remove unused argument - remove unused stack buffer - remove useless code block
(Only the top-level ones, ie, for each call to eg asn1_get_mpi(), ensure there's at least one test case that makes this call fail in one way, but don't test the various ways to make asn1_get_mpi fail - that should be covered elsewhere.) - the new checks added by the previous commits needed exercising - existing tests sometimes had wrong descriptions or where passing for the wrong reason (eg with the "length mismatch" test, the function actually failed before reaching the length check) - while at it, add tests for the rest as well The valid minimal-size key was generated with: openssl genrsa 128 2>/dev/null | openssl rsa -outform der 2>/dev/null | xxd -p
Some code paths want to access members of the mbedtls_rsa_context structure. We can only do that when using our own implementation, as otherwise we don't know anything about that structure.
mpg
added
bug
mbed TLS team
needs-review
2 tasks
yanesca
approved these changes
Feb 18, 2020
gilles-peskine-arm
requested changes
Feb 18, 2020
| { | ||
| mbedtls_pk_context pk; | ||
| unsigned char buf[2000]; | ||
| unsigned char output[2000]; | ||
| unsigned char buf[128]; |
There was a problem hiding this comment.
This is uncomfortably small. We may well add larger test data without thinking about it. The old test framework in 2.7 isn't convenient for this, but please at least add a check that data_len <= sizeof(buf) after the call to unhexify, which will have a decent chance of catching a buffer overflow.
There was a problem hiding this comment.
Good point, I added a test for overflow.
yanesca
requested changes
Feb 18, 2020
gilles-peskine-arm
added
needs-work
and removed
needs-review
|
Thanks @yanesca for noticing the issue with the ChangeLog and @gilles-peskine-arm for noticing the danger in the test function. I fixed both, please review again. |
mpg
added
needs-review
gilles-peskine-arm
approved these changes
Feb 19, 2020
gilles-peskine-arm
added
approved
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.
This is the 2.7 backport of ARMmbed/mbed-crypto#367