Check null stack to prevent heap-buffer-overflow#299
Closed
mmz-zmm wants to merge 1 commit into
Closed
Conversation
perlpunk
reviewed
Jun 12, 2024
| yaml_node_t node; | ||
|
|
||
| assert(document); /* Non-NULL document object is expected. */ | ||
| if (STACK_NULL(&context, document->nodes)) goto error; |
Member
There was a problem hiding this comment.
I guess same the check would have to be added to yaml_document_add_scalar, yaml_document_add_mapping etc. to make this useful.
Author
There was a problem hiding this comment.
Yes, there are other places needs stack checking. I saw your discussions and not sure this cve needs fix or not, just sending this patch to discuss. Will send a V2.
This patch adds a new macro STACK_NULL to check if given stack was initialized, in order to fix #298, which is CVE-2024-35329. The root cause is stack(document->nodes) was used before initialized, so check stack before push. According to the poc in [1], building it with `gcc poc.c -o poc -lyaml -fsanitize=address` Before this patch, the output is: [root@test yaml-0.2.5]# ./poc heap-buffer-overflow on libyaml/src/api.c:1274:10 ================================================================= ==3867981==ERROR: LeakSanitizer: detected memory leaks Direct leak of 64 byte(s) in 1 object(s) allocated from: #0 0x7f571f6af1a7 in __interceptor_malloc (/usr/lib64/libasan.so.6+0xaf1a7) #1 0x7f5720127ac9 in yaml_document_add_sequence /root/libxml/yaml-0.2.5/src/api.c:1271 Direct leak of 22 byte(s) in 1 object(s) allocated from: #0 0x7f571f659707 in strdup (/usr/lib64/libasan.so.6+0x59707) #1 0x7f5720127ab7 in yaml_document_add_sequence /root/libxml/yaml-0.2.5/src/api.c:1268 Direct leak of 1 byte(s) in 1 object(s) allocated from: #0 0x7f571f6af1a7 in __interceptor_malloc (/usr/lib64/libasan.so.6+0xaf1a7) #1 0x7f5720125762 in yaml_stack_extend /root/libxml/yaml-0.2.5/src/api.c:126 SUMMARY: AddressSanitizer: 87 byte(s) leaked in 3 allocation(s). After this patch, there are no memory leaks warnnings. [1] https://drive.google.com/file/d/1xgQ9hJ7Sn5RVEsdMGvIy0s3b_bg3Wyk-/view?usp=sharing Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 patch adds a new macro STACK_NULL to check if given stack was initialized, in order to fix #298, which is CVE-2024-35329.
The root cause is stack(document->nodes) was used before initialized, so check stack before push.
According to the poc in [1], building it with
gcc poc.c -o poc -lyaml -fsanitize=addressBefore this patch, the output is:
After this patch, there are no memory leaks warnnings.
[1] https://drive.google.com/file/d/1xgQ9hJ7Sn5RVEsdMGvIy0s3b_bg3Wyk-/view?usp=sharing
edit @perlpunk: add code markers