PKCS7: Fix some memory management errors #6670
Merged
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
This fixes a use-after-free in PKCS#7 parsing when the signer data is malformed. Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53798). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Remove useless goto in several functions. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This may have been a use-after-free, but I haven't worked out whether it was a problem or not. Even if it turns out to have been ok, keeping invalid pointers around is fragile. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
mbedtls_x509_name allocates memory, which must be freed if there is a subsequent error. Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53811). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
bug
needs-review
daverodgman
previously approved these changes
Nov 28, 2022
daverodgman
reviewed
Nov 28, 2022
tests/suites/test_suite_pkcs7.data
Outdated
Comment on lines
65
to
66
| pkcs7_get_signers_info_set error handling (6213931373035520) | ||
| pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG |
There was a problem hiding this comment.
Suggested change
| pkcs7_get_signers_info_set error handling (6213931373035520) | |
| pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG | |
| pkcs7_get_signers_info_set error handling (6213931373035520) | |
| depends_on:MBEDTLS_RIPEMD160_C | |
| pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG |
There was a problem hiding this comment.
Thanks for figuring out which one it is. RIPEMD160 and SHA1 have shorter OIDs than MD5 or the SHA2, so I guess they're more likely to appear in fuzz data.
daverodgman
reviewed
Nov 28, 2022
tests/suites/test_suite_pkcs7.data
Outdated
Comment on lines
68
to
69
| pkcs7_get_signers_info_set error handling (4541044530479104) | ||
| pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG |
There was a problem hiding this comment.
Suggested change
| pkcs7_get_signers_info_set error handling (4541044530479104) | |
| pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG | |
| pkcs7_get_signers_info_set error handling (4541044530479104) | |
| depends_on:MBEDTLS_RIPEMD160_C | |
| pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG |
daverodgman
reviewed
Nov 28, 2022
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
daverodgman
approved these changes
Nov 28, 2022
gilles-peskine-arm
added
needs-reviewer
mpg
added
approved
|
The internal CI fully passed, and Open CI only failed to windows components due to infrastructure glitches. |
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 one or two use-after-free and a memory leak in the newly merged PKCS7 parser.
The test cases are direct from OSS-Fuzz. Preferably we'd construct minimal reproducers but I'm pushing this out quickly because it's release-critical.
Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53798, https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53811).
Gatekeeper checklist