Fix int overflow in mbedtls_asn1_get_int #297
Merged
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 the asn1parse module is enabled but the bignum module is disabled, the asn1parse test suite did not work. Fix this. * Fix a syntax error in get_integer() (label immediately followed by a closing brace). * Fix an unused variable in get_integer(). * Fix `TEST_ASSERT( *p == q );` in nested_parse() failing because `*p` was not set. * Fix nested_parse() not outputting the length of what it parsed.
mbedtls_asn1_get_int() and mbedtls_asn1_get_mpi() behave differently on an empty INTEGER (0200). Don't change the library behavior for now because this might break interoperability in some applications. Write a test function that matches the library behavior.
mbedtls_asn1_get_int() and mbedtls_asn1_get_mpi() behave differently on negative INTEGERs (0200). Don't change the library behavior for now because this might break interoperability in some applications. Change the test function to the library behavior. Fix the test data with negative INTEGERs. These test cases were previously not run (they were introduced but deliberately deactivated in 27d806f). The test data was actually wrong: ASN.1 uses two's complement, which has no negative 0, and some encodings were wrong. Now the tests have correct data, and the test code rectifies the expected data to match the library behavior.
Test more INTEGER values, especially near the boundary of int (which is at 2^31-1 on all our officially supported platforms).
No behavior change.
Fix a signed int overflow in mbedtls_asn1_get_int() for numbers between INT_MAX+1 and UINT_MAX (typically 0x80000000..0xffffffff). This was undefined behavior which in practice would typically have resulted in an incorrect value, but which may plausibly also have caused the postcondition (*p == initial<*p> + len) to be violated. Credit to OSS-Fuzz.
gilles-peskine-arm
added
bug
Patater
approved these changes
Oct 11, 2019
| INTEGER 0x123456789abcdef0 | ||
| get_integer:"0208123456789abcdef0":"123456789abcdef0":0 | ||
|
|
||
| INTEGER 0xfedcab9876543210 |
There was a problem hiding this comment.
Not super important, but if you wanted decreasing digits, use 0xfedcba9876543210. (swap a and b).
AndrzejKurek
approved these changes
Oct 11, 2019
|
CI only failed on atecc608a-K64F jobs which is a known issue that's unrelated to this PR. Ok to merge. |
gilles-peskine-arm
removed
the
needs: review
gilles-peskine-arm
added a commit
to gilles-peskine-arm/mbed-crypto
that referenced
this pull request
Nov 15, 2019
* ARMmbed#272: Insert doxygen comments on old algorithms so they appear in PSA documentation * ARMmbed#285: SE driver: make persistent data work * ARMmbed#279: Include IANA reference in the definition of ECC curves and DH groups * ARMmbed#287: DRBG documentation improvements * ARMmbed#297: Fix int overflow in mbedtls_asn1_get_int (Credit to OSS-Fuzz)
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.
Fix an int overflow in
mbedtls_asn1_get_int()introduced by #75 (so not present in any released version).The signed int overflow in
mbedtls_asn1_get_int()happened for numbers betweenINT_MAX+1andUINT_MAX(typically 0x80000000..0xffffffff). This was undefined behavior which in practice would typically have resulted in an incorrect value, but which may plausibly also have caused the postcondition(*p == initial<*p> + len)to be violated.Also fix up the ASN.1 INTEGER parsing tests a bit.
Credit to OSS-Fuzz (private link: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18102). To fix this in OSS-Fuzz, we'll need to update the crypto submodule in mbedtls after merging this PR.
Internal ref: IOTCRYPT-947