Fix minor defects found by Coverity #349
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
Check the value only once, as soon as we've obtained it.
Fix get_len_step when buffer_size==0. The intent of this test is to ensure (via static or runtime buffer overflow analysis) that mbedtls_asn1_get_len does not attempt to access beyond the end of the buffer. When buffer_size is 0 (reached from get_len when parsing a 1-byte buffer), the buffer is buf[1..1] because allocating a 0-byte buffer might yield a null pointer rather than a valid pointer. In this case the end of the buffer is p==buf+1, not buf+buffer_size which is buf+0. The test passed because calling mbedtls_asn1_get_len(&p,end,...) with end < p happens to work, but this is not guaranteed.
This was the intended behavior of ASSERT_ALLOC_WEAK all along, but skipping was not implemented yet when ASSERT_ALLOC_WEAK was introduced.
gilles-peskine-arm
added
bug
gilles-peskine-arm
force-pushed
the
coverity-20200115-crypto
branch
from
0b7f33f to
84984ae
Compare
|
The CI passed all tests except the Mbed OS one, which are expected to fail until Mbed OS is updated. So that's as good as a pass. |
yanesca
approved these changes
Jan 22, 2020
yanesca
removed
the
needs: review
This was referenced Jan 22, 2020
mpg
added
the
ready for merge
gilles-peskine-arm
added a commit
to gilles-peskine-arm/mbed-crypto
that referenced
this pull request
Feb 3, 2020
Previously in d875285: * ARMmbed#333: Streamline PSA key type encodings: prepare * ARMmbed#323: Initialise return values to an error Previously in dbcb442: * ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY * ARMmbed#334: Fix some pylint warnings Previously in ceceedb: * ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4 * ARMmbed#354: Fix incrementing pointer instead of value In this commit: * ARMmbed#349: Fix minor defects found by Coverity * ARMmbed#179: Add option to build SHA-512 without SHA-384 * ARMmbed#327: Implement psa_hash_compute and psa_hash_compare * ARMmbed#330: Streamline PSA key type and curve encodings
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.
ASSERT_ALLOC_WEAKskip the test rather than pass it if it fails, as was intended all along.The missing return value checks should be backported where applicable. The other bugs are so minor that they don't need backporting (but can be).
ASSERT_ALLOC_WEAKis not in the LTS branches.Backports