Fix CVE-2025-2915 #5746
Fix CVE-2025-2915 #5746
Conversation
glennsong09
requested review from
byrnHDF,
derobins,
fortnern,
jhendersonHDF,
lrknox,
qkoziol and
vchoi-hdfgroup
as code owners
glennsong09
requested review from
bmribler,
brtnfld and
mattjala
as code owners
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR fixes a critical security vulnerability (CVE-2025-2915) in the HDF5 library by addressing a heap-based buffer overflow in the H5MF_xfree function. The issue occurred when the function was called with an address of 0, which represents the file superblock that should never be freed.
The core change replaces a debug-time assertion with proper runtime error handling. The original code used assert(addr != 0) to check that the superblock wasn't being freed, but assertions are typically disabled in release builds, leaving the vulnerability exposed when processing malicious or corrupted HDF5 files. The fix converts this to a runtime check if (addr <= 0) that returns an appropriate error instead of allowing the invalid operation to proceed.
This change fits into HDF5's memory management subsystem (H5MF - Memory File management) which handles allocation and deallocation of file space. The fix ensures that the fundamental invariant of never freeing the superblock is enforced at runtime, not just during development testing. The condition was also strengthened from != 0 to <= 0 to catch any non-positive addresses, providing more comprehensive protection against similar vulnerabilities.
Important Files Changed
Files Changed
| Filename | Score | Overview |
|---|---|---|
src/H5MF.c |
5/5 | Replaced debug assertion with runtime error check to prevent heap buffer overflow when freeing superblock address |
1 file reviewed, no comments
src/H5MF.c
Outdated
| assert(addr != 0); /* Can't deallocate the superblock :-) */ | ||
|
|
||
| if (addr <= 0) | ||
| HGOTO_ERROR(H5E_RESOURCE, H5E_BADRANGE, FAIL, "attempting to free file superblock"); |
There was a problem hiding this comment.
I know the original assert didn't have, but should addr also be checked for upper bound, not going pass the end of the file?
There was a problem hiding this comment.
This appears to be related to #5202 (possibly the same issue). I have essentially the same comment here. The logic in H5F__accum_free should prevent this buffer overlfow, even if addr is 0, as long as accum->size <= accum->alloc_size. We should figure out how that assumption came to be violated.
It is perhaps appropriate to make sure addr cannot be 0, but ideally we should catch it sooner. Do you know where the 0 address came from?
github-project-automation
bot
moved this from Scheduled/On-Deck
to In progress
in HDF5 - TRIAGE & TRACK
glennsong09
force-pushed
the
issue5380
branch
from
e3f59fc to
57733ff
Compare
src/H5Faccum.c
Outdated
|
|
||
| /* Calculate the size of the overlap with the accumulator, etc. */ | ||
| H5_CHECKED_ASSIGN(overlap_size, size_t, (addr + size) - accum->loc, haddr_t); | ||
| if (overlap_size > accum->size) |
There was a problem hiding this comment.
I'm still not sure we've found the root of the problem. The condition to get into this block is effectively addr+size < accum->loc + accum->size. Therefore overlap_size, which is addr+size-accum->loc should be less than accum->size undern normal conditions. Is one of the quantities involved (addr, size, accum->loc, accum->size) very large and causing wraparound? If so, is there a real reason it should be this way, or can we detect an impossibly large value when it's loaded from disk? We should also add H5_CHECK_OVERFLOW and similar checks as appropriate.
glennsong09
force-pushed
the
issue5380
branch
from
2af5948 to
1508f3f
Compare
src/H5Ocache_image.c
Outdated
| HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "address plus size overflows"); | ||
| if (mesg->addr == HADDR_UNDEF) | ||
| HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "address is undefined"); | ||
| if ((mesg->addr + mesg->size) > H5F_get_eoa(f, H5FD_MEM_DEFAULT)) |
There was a problem hiding this comment.
Have you tried using H5FD_MEM_SUPER for the type?
There was a problem hiding this comment.
Yes, I swear I used H5FD_MEM_SUPER, I'm not sure when I changed it back to H5FD_MEM_DEFAULT.
| HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); | ||
| H5F_DECODE_LENGTH(f, p, mesg->size); | ||
|
|
||
| if (mesg->addr >= (HADDR_UNDEF - mesg->size)) |
Co-authored-by: Neil Fortner <fortnern@gmail.com>
6 participants
This PR fixes issue #5380, which has a heap based buffer overflow after H5MF_xfree is called on an address of 0 (file superblock). This PR changes an assert making sure addr isn't 0 to an if check.
The bug was first reproduced using the fuzzer and the POC file from #5380. With this change, the heap based buffer overflow no longer occurs.