[backport 2.16] Fix pk_parse_key()'s use of rsa_complete() #3048
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
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
gilles-peskine-arm
previously approved these changes
Feb 18, 2020
yanesca
approved these changes
Feb 18, 2020
yanesca
added
approved
yanesca
requested changes
Feb 18, 2020
gilles-peskine-arm
added
needs-work
and removed
approved
mpg
added
needs-review
|
Thanks @yanesca for noticing the missing ChangeLog entry! I fixed that, please review - cc @gilles-peskine-arm as well. |
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.16 backport of ARMmbed/mbed-crypto#367