-
-
Notifications
You must be signed in to change notification settings - Fork 298
Fix oss-fuzz vulns #5209
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
base: develop
Are you sure you want to change the base?
Fix oss-fuzz vulns #5209
Conversation
Patch OSV 2023 76
Fix vuln OSV-2024-379
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.
These are not bad, but seem like they are addressing symptoms rather that root causes. Are there test files that trigger them to look at?
@qkoziol You can download them by clicking on the POC link. I wonder why I can't upload Sanitizer Report... |
@aled-ua Probably because of the type of the file. What kind of file format is the Sanitizer Report? |
@bmribler Update now |
@qkoziol Hi Quincey, are you looking at the test file, by any chance? |
I have been busy on some internal projects for the past few days, but I am planning to. Please don't let me impede you diving in and investigating also. :-) |
I don't want to double the efforts though. :-) |
@qkoziol Are you still planning to look at this issue too? |
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.
Pull Request Overview
This PR addresses several OSS-Fuzz reported heap-buffer-overflow vulnerabilities by replacing asserts with explicit error checks. The changes add runtime validations to ensure that critical fields such as the SOHM address and bin indices are valid before proceeding.
- H5SM.c: Added an explicit runtime check for the SOHM address in H5SM_delete.
- H5FSsection.c: Replaced an assert with a proper error check to validate the computed bin index.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/H5SM.c | Added runtime validation for the SOHM address to prevent misuse. |
src/H5FSsection.c | Replaced assert with error handling for bin index validation. |
Comments suppressed due to low confidence (1)
src/H5FSsection.c:935
- The added check correctly prevents out-of-bounds access on the bins array; ensure similar validations are applied consistently wherever bin indices are computed.
if (bin >= sinfo->nbins)
@@ -1538,6 +1538,11 @@ H5SM_delete(H5F_t *f, H5O_t *open_oh, H5O_shared_t *sh_mesg) | |||
assert(H5_addr_defined(H5F_SOHM_ADDR(f))); | |||
assert(sh_mesg); | |||
|
|||
/* Validate the SOHM address */ | |||
if (!H5_addr_defined(H5F_SOHM_ADDR(f))) { |
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.
[nitpick] The SOHM address is already asserted at the top of the function; consider removing the earlier assert to avoid redundancy or inconsistency in error handling.
Copilot uses AI. Check for mistakes.
commit c792aa1 Fix
Full Sanitizer Report:
commit 1126b83 Fix
Full Sanitizer Report:
Full Sanitizer Report:
What these issues have in common is that they should trigger an assert before the vulnerability.
I'm not sure if it should be fixed more completely than just replacing assert with a real error checking.