Fix CVE-2025-2925 #5739
Fix CVE-2025-2925 #5739
Conversation
glennsong09
requested review from
bmribler,
brtnfld,
byrnHDF,
derobins,
fortnern,
jhendersonHDF,
mattjala,
qkoziol and
vchoi-hdfgroup
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-2925) that causes a double free condition in the HDF5 cache entry loading functionality. The issue occurs in src/H5Centry.c within the H5C__load_entry function at two specific locations where H5MM_realloc is called.
The vulnerability manifests when actual_len + H5C_IMAGE_EXTRA_SPACE equals 0, causing H5MM_realloc to free the original image buffer and return NULL. The original code would immediately assign this NULL result to the image pointer, creating a dangerous situation where the cleanup code would attempt to free a NULL or dangling pointer, leading to a double free.
The fix implements a safer pattern by:
- Storing the realloc result in a temporary variable (
new_image) - Only assigning the result to
imageif the realloc succeeded (non-NULL) - Preserving the original
imagepointer for proper cleanup if realloc fails
This change maintains the existing error handling flow while preventing the double free condition. The fix was validated using fuzzing and the proof-of-concept file from the original issue report.
Important Files Changed
Files Modified
| Filename | Score | Overview |
|---|---|---|
src/H5Centry.c |
5/5 | Fixed double free vulnerability in cache entry loading by improving realloc error handling |
1 file reviewed, no comments
src/H5Centry.c
Outdated
| if (NULL == (new_image = H5MM_realloc(image, len + H5C_IMAGE_EXTRA_SPACE))) | ||
| HGOTO_ERROR(H5E_CACHE, H5E_CANTALLOC, NULL, "image null after H5MM_realloc()"); | ||
| new_image = H5MM_realloc(image, len + H5C_IMAGE_EXTRA_SPACE); | ||
| image = (uint8_t *)new_image; |
There was a problem hiding this comment.
This pattern has the same issue that the usage of new_image is avoiding. If realloc returned NULL because of failure, assigning image = (uint8_t *)new_image loses the image pointer and results in a memory leak. Assigning here could potentially be fine if realloc returned NULL due to the size being 0, but as there's implementation-specific behavior involved there, it seems to me checking for a realloc size of 0 and throwing an error is probably the best approach.
There was a problem hiding this comment.
Okay, I can do that instead.
src/H5Centry.c
Outdated
| do { | ||
| if (actual_len != len) { | ||
| /* Verify that the length isn't a bad value */ | ||
| if (actual_len <= 0) |
There was a problem hiding this comment.
This should be checking len rather than actual_len (the second one is correct). These are also size_t variables, so should use a check of == 0 rather than <= 0.
There was a problem hiding this comment.
Sorry about that, I just made the changes.
| do { | ||
| if (actual_len != len) { | ||
| /* Verify that the length isn't a bad value */ | ||
| if (len == 0) |
There was a problem hiding this comment.
@jhendersonHDF Does len need to be traced back to where it was obtained and caught there?
There was a problem hiding this comment.
That could be useful to do in addition to these checks, though catching it at this level will probably cover a wider range of issues.
There was a problem hiding this comment.
Being pragmatic, I think this change is fine without needing to trace back to where an image size of 0 came from, as it could be in multiple places and this check should cover them anyway. Though this still needs a release note, either in this PR or after merging since #5778 will affect where that note goes.
I have just added a release note change to this PR. |
|
With the CHANGELOG.md PR having been merged, you should rebase this PR off of the current develop and add the note to the "Bugs Fixed" section of release_docs/CHANGELOG.md. Also make sure to mention the CVE number somewhere in the note. Other than that, I think this looks good. |
glennsong09
force-pushed
the
5383issue
branch
from
1b006d3 to
15da7a3
Compare
There was a problem hiding this comment.
Need to remove the changes to this file
github-project-automation
bot
moved this from Scheduled/On-Deck
to In progress
in HDF5 - TRIAGE & TRACK
Co-authored-by: jhendersonHDF <[email protected]>
github-project-automation
bot
moved this from In progress
to Done
in HDF5 - TRIAGE & TRACK
7 participants
This PR fixes issue #5383, which was occurring due to actual_len + H5C_IMAGE_EXTRA_SPACE being 0. When realloc was called, it freed image, but gets sent to done before new_image can be assigned to image. Because the pointer for image isn't null, it attempts to free it here again, causing the double free to occur. This PR addresses Quincey's concern and fixes the issue while preserving new_image and image.
The bug was first reproduced using the fuzzer and the POC file from #5383. With this change, the double free no longer occurs.