-
-
Notifications
You must be signed in to change notification settings - Fork 318
Fix CVE-2025-2926 #5841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CVE-2025-2926 #5841
Conversation
|
Umm, FYI, this PR only has the changes in H5Centry.c and RELEASE.txt. Sorry! |
release_docs/RELEASE.txt
Outdated
| =================================== | ||
| Library | ||
| ------- | ||
| - Fixed CVE 2025 2926 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use hyphens in the name, like other CVE issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't know how that happened. I always have hyphens... Thanks, Dana.
src/H5Cimage.c
Outdated
| if (H5C__decode_cache_image_header(f, cache_ptr, &p, image_len + 1) < 0) | ||
| HGOTO_ERROR(H5E_CACHE, H5E_CANTDECODE, FAIL, "cache image header decode failed"); | ||
| assert((size_t)(p - (uint8_t *)cache_ptr->image_buffer) < cache_ptr->image_len); | ||
| assert((size_t)(p - (uint8_t *)cache_ptr->image_buffer) < image_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this a real error check instead of an assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If H5C__decode_cache_image_header checks for overflow an assert is appropriate here. And if it doesn't, it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was merged into this PR by accident and that was fixed now. I'll check out about H5C__decode_cache_image_header() and create another PR instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/H5Centry.c
Outdated
| if (type->get_initial_load_size(udata, &len) < 0) | ||
| HGOTO_ERROR(H5E_CACHE, H5E_CANTGET, NULL, "can't retrieve image size"); | ||
| assert(len > 0); | ||
| if (len == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me this should really be checked in the callbacks, and left as an assert here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why the callback is returning len=0 without returning an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it and push the change soon
a4678ff to
b36c123
Compare
fortnern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See questions about get_initial_load_size returning len==0
|
So the CVE is already fixed without this PR? I think the check in this PR should just be an assertion (unless there's some case where we really can't catch it before here) |
|
We should only do enough non-assert checking as is necessary to guarantee a consistent internal state. Additional checking should be in the form of assertions. |
|
@fortnern The assert was already put back in the commit I mentioned in #5841 (comment). Sorry, I forgot that I did. |
|
There is no assert in H5O__cache_chk_get_initial_load_size() currently and this PR adds an error check instead of an assert. Is there a reason you think this should not be an assert instead? |
fortnern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to an assertion or explain why it needs to be an error check. My understanding is the CVE was fixed by a different PR that added the check in the appropriate place, correct? Did that PR add a Changelog.md note? If so, no need for this one.
| Fixed a heap buffer overflow in H5FS__sinfo_serialize_node_cb() by discarding file free space sections from the file free space manager when they are found to be invalid. Specifically crafted HDF5 files can result in an attempt to insert duplicate or overlapping file free space sections into a file free space manager, later resulting in a buffer overflow when the same free space section is serialized to the file multiple times. | ||
|
|
||
| Fixes GitHub issue #5577 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't know how this entry got into my commit... I hope I didn't cause anything bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to already be in develop, hopefully it gets fixed when it's merged
|
Unused variable is causing the |
I'll fix it. Thank you! |
302922c
|
Committed a conflict in CHANGELOG.md. Previously reviews and approvals by fortnern, glennsong09, and mattjala dismissed by the non-code commit. Will approve and merge as soon as tests pass. |
An image size was corrupted and decoded as 0 resulting in a NULL image buffer, which caused a NULL pointer dereference when the image being copied to the buffer. The invalid image size was caught in the PR HDFGroup#5710. This change catches right before the copying. Fixes GH issue HDFGroup#5384
An image size was corrupted and decoded as 0 resulting in a NULL image buffer, which caused a NULL pointer dereference when the image being copied to the buffer. This PR adds the image size check.
Fixes #5384
Important
Fixes CVE-2025-2926 by adding an image size check in
H5O__cache_chk_get_initial_load_size()to prevent NULL pointer dereference.H5O__cache_chk_get_initial_load_size()inH5Ocache.cto prevent NULL pointer dereference.CHANGELOG.mdto include details of the fix for CVE-2025-2926.This description was created by
for eed45d8. You can customize this summary. It will automatically update as commits are pushed.