Skip to content
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.

Sign up for GitHub

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 vuln OSV-2023-77 #5210

Merged
merged 7 commits into from
Jan 15, 2025
Merged

Fix vuln OSV-2023-77 #5210

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c9110c7
Fix vuln OSV-2023-77
aled-ua Dec 24, 2024
8cebb9c
Committing clang-format changes
github-actions[bot] Dec 24, 2024
2d5377d
use H5_IS_BUFFER_OVERFLOW to check overflow
aled-ua Jan 9, 2025
b4ebd70
add size parameter
aled-ua Jan 10, 2025
7e54602
Committing clang-format changes
github-actions[bot] Jan 10, 2025
64568ea
Merge pull request #10 from aled-ua/aled-ua-patch-test
aled-ua Jan 10, 2025
79e7168
Fix the last valid byte in buf
aled-ua Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/H5Cimage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,6 +1287,10 @@ H5C__decode_cache_image_header(const H5F_t *f, H5C_t *cache_ptr, const uint8_t *
/* Point to buffer to decode */
p = *buf;

/* Ensure buffer has enough data for signature comparison */
if ((size_t)(*buf + H5C__MDCI_BLOCK_SIGNATURE_LEN - p) > cache_ptr->image_len)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is generally fine, but a couple of issues here:

  • This should really use the H5_IS_BUFFER_OVERFLOW macro from H5private.h to be consistent with other decode routines throughout the library
  • This is a yet-to-be-fixed case where the size of buf should really be passed in as a parameter to the function rather than relying on it from elsewhere. For example, the size of buf here is actually cache_ptr->image_len + 1, which doesn't really make a difference for this case, but it's easy to see how relying on cache_ptr->image_len here can cause trouble.

Copy link
Contributor Author

@aled-ua aled-ua Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is generally fine, but a couple of issues here:

  • This should really use the H5_IS_BUFFER_OVERFLOW macro from H5private.h to be consistent with other decode routines throughout the library
  • This is a yet-to-be-fixed case where the size of buf should really be passed in as a parameter to the function rather than relying on it from elsewhere. For example, the size of buf here is actually cache_ptr->image_len + 1, which doesn't really make a difference for this case, but it's easy to see how relying on cache_ptr->image_len here can cause trouble.

Thanks for your suggestion, I updated using macros for checking.
But for the second point, all arguments passed in this function so far, buf, seem to be cache_ptr->image_buffer, is there any possibility of expansion in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have been more clear; what I meant was that we should update H5C__decode_cache_image_header() with an extra size parameter for the size of buf and then use that for overflow calculations rather than cache_ptr->image_len. This is similar to other decode routines where the functions have both a buffer and size of the buffer parameter. The calculations should essentially be the same since cache_ptr->image_len + 1 is what the callers will be passing in for the size parameter, but it should be a bit safer than relying on the size from the cache_ptr structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I understand, sounds good, I have added the size parameter.

HGOTO_ERROR(H5E_CACHE, H5E_OVERFLOW, FAIL, "Insufficient buffer size for signature");

/* Check signature */
if (memcmp(p, H5C__MDCI_BLOCK_SIGNATURE, (size_t)H5C__MDCI_BLOCK_SIGNATURE_LEN) != 0)
HGOTO_ERROR(H5E_CACHE, H5E_BADVALUE, FAIL, "Bad metadata cache image header signature");
Expand Down
Loading