Fix vuln OSV-2023-77 by oss-patch · Pull Request #5210 · HDFGroup/hdf5

oss-patch · 2025-01-07T05:52:13Z

[Warning] This PR is generated by AI

Pull Request Description

1. PR Title: Fix for Heap-Buffer-Overflow Vulnerability in HDF5 - OSV-2023-77

2. PR Description:

Bug Type: Heap-Buffer-Overflow
Summary: A heap-buffer-overflow vulnerability was identified in the HDF5 program. The issue occurred when the program attempted to access memory beyond the allocated buffer in the H5C__decode_cache_image_header function. Specifically, it tried to read 4 bytes from a 1-byte allocated buffer, resulting in an out-of-bounds memory access.
Fix Summary:
- The patch introduces a bounds check before performing the memcmp operation. This ensures that the buffer has sufficient data for the signature comparison. If the buffer size is insufficient, the function exits gracefully with an appropriate error message, preventing the overflow.
- This fix improves the program's security and stability by eliminating the possibility of illegal memory access in this scenario and gracefully handling errors.

3. Sanitizer Report Summary:

The sanitizer detected a heap-buffer-overflow error. Specifically:

The program attempted to access 4 bytes at offset 1 in a buffer with only 1 byte allocated.
The error occurred in the H5C__decode_cache_image_header function at /src/H5Cimage.c:1291:9, called by several other functions leading up to the H5Dopen2 function.
The buffer was allocated in the function H5C__load_cache_image at /src/H5Cimage.c:619:48.

4. Full Sanitizer Report:

==29606==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5020000036b1 at pc 0x55591d45f93c bp 0x7ffd4e8bd180 sp 0x7ffd4e8bc928
READ of size 4 at 0x5020000036b1 thread T0
    #0 0x55591d45f93b in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) (/root/out/h5_extended_fuzzer+0x1be93b) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)
    #1 0x55591d45fe10 in memcmp (/root/out/h5_extended_fuzzer+0x1bee10) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)
    #2 0x55591d615dae in H5C__decode_cache_image_header /root/src/H5Cimage.c:1291:9
    #3 0x55591d608b68 in H5C__reconstruct_cache_contents /root/src/H5Cimage.c:2389:9
    #4 0x55591d608273 in H5C__load_cache_image /root/src/H5Cimage.c:627:13
    #5 0x55591d5ee287 in H5C_protect /root/src/H5Centry.c:2985:13
    #6 0x55591d5658d2 in H5AC_protect /root/src/H5AC.c:1302:26
    #7 0x55591da544aa in H5O_protect /root/src/H5Oint.c:1014:32
    #8 0x55591da87042 in H5O_msg_exists /root/src/H5Omessage.c:787:23
    #9 0x55591d8c7523 in H5G__obj_get_linfo /root/src/H5Gobj.c:309:22
    #10 0x55591d8ce826 in H5G__obj_lookup /root/src/H5Gobj.c:1069:25
    #11 0x55591d8de9d2 in H5G__traverse_real /root/src/H5Gtraverse.c:572:13
    #12 0x55591d8dd8a9 in H5G_traverse /root/src/H5Gtraverse.c:845:9
    #13 0x55591d8aecb2 in H5G_loc_find /root/src/H5Gloc.c:423:9
    #14 0x55591d6a1952 in H5D__open_name /root/src/H5Dint.c:1471:9
    #15 0x55591e2ec6cb in H5VL__native_dataset_open /root/src/H5VLnative_dataset.c:331:25
    #16 0x55591e292ea0 in H5VL__dataset_open /root/src/H5VLcallback.c:2053:25
    #17 0x55591e2928bd in H5VL_dataset_open /root/src/H5VLcallback.c:2088:30
    #18 0x55591d6672dd in H5D__open_api_common /root/src/H5D.c:361:25
    #19 0x55591d666b11 in H5Dopen2 /root/src/H5D.c:400:22
    #20 0x55591d51eb8f in LLVMFuzzerTestOneInput /root/src/h5_extended_fuzzer.c:31:27
    #21 0x55591d429f04 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/out/h5_extended_fuzzer+0x188f04) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)
    #22 0x55591d413036 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/out/h5_extended_fuzzer+0x172036) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)
    #23 0x55591d418aea in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/out/h5_extended_fuzzer+0x177aea) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)
    #24 0x55591d4432a6 in main (/root/out/h5_extended_fuzzer+0x1a22a6) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)
    #25 0x7f8c0a7761c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #26 0x7f8c0a77628a in __libc_start_main csu/../csu/libc-start.c:360:3
    #27 0x55591d40dc04 in _start (/root/out/h5_extended_fuzzer+0x16cc04) (BuildId: 6ecd336a42c3c4e6b59a0e213d00d5e162a6c7ff)

SUMMARY: AddressSanitizer

5. Files Modified:

src/H5Cimage.c

--- a/src/H5Cimage.c
+++ b/src/H5Cimage.c
@@ -1286,6 +1286,11 @@ 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)
+        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");

6. Patch Validation:

The patch has been validated using the provided PoC. The vulnerability identified in the sanitizer report has been resolved, and no new issues were introduced.

7. Links:

-Original Vulnerability Report
-PoC

jhendersonHDF · 2025-01-08T18:44:31Z

src/H5Cimage.c

    p = *buf;

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


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.

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?

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.

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

add an extra size paramete in H5C__decode_cache_image_header()

jhendersonHDF · 2025-01-14T16:31:27Z

src/H5Cimage.c

    p = *buf;

+    /* Ensure buffer has enough data for signature comparison */
+    if (H5_IS_BUFFER_OVERFLOW(p, H5C__MDCI_BLOCK_SIGNATURE_LEN, *buf + buf_size))


This should be *buf + buf_size - 1 (the last valid byte in *buf), but otherwise the changes look good; thanks for this fix!

jhendersonHDF

Just a small fix needed

qkoziol · 2025-01-15T17:31:58Z

src/H5Cimage.c

    /* Decode metadata cache image header */
    p = (uint8_t *)cache_ptr->image_buffer;
-    if (H5C__decode_cache_image_header(f, cache_ptr, &p) < 0)
+    if (H5C__decode_cache_image_header(f, cache_ptr, &p, cache_ptr->image_len + 1) < 0)


Why are you adding '+1' here and then '-1' in the code in H5C__decode_cache_image_header?

I believe that you can remove both.

The +1 is the real total size of the buffer, so it makes sense to pass it here as that's what the function should expect to receive. The -1 in the function is just because of how the H5_IS_BUFFER_OVERFLOW macro is implemented. They could both be removed, but I think that's making things a lot less clear and introducing the potential for trouble.

The image_len is not the actual size of the image_buffer?

I'm not sure if the extra byte allocated for the cache image buffer is necessary (see H5Cimage.c ~line 622), but if that +1 was removed then this becomes simple

Agree - those '+1' in the allocations are incorrect. Especially since we are reading, writing, and bcasting only 'image_len' everywhere in the code.

qkoziol · 2025-01-15T17:36:11Z

Also, I believe this line in H5C__decode_cache_image_header?

"if (cache_ptr->image_data_len != cache_ptr->image_len)"

should us 'buf_size' instead of 'cache_ptr->image_len'

jhendersonHDF · 2025-01-15T17:48:51Z

Also, I believe this line in H5C__decode_cache_image_header?

"if (cache_ptr->image_data_len != cache_ptr->image_len)"

should us 'buf_size' instead of 'cache_ptr->image_len'

With the extra byte allocated for the cache image buffer, I'm think that comparing against buf_size (which should include the +1) would cause that check to fail. I think relying on the passed in size rather than the size in the structure is a better idea anyway, so it seems like someone should investigate whether that extra byte allocated for the buffer is necessary since it complicates things a little.

qkoziol · 2025-01-15T17:52:01Z

Also, I believe this line in H5C__decode_cache_image_header?
"if (cache_ptr->image_data_len != cache_ptr->image_len)"
should us 'buf_size' instead of 'cache_ptr->image_len'

With the extra byte allocated for the cache image buffer, I'm think that comparing against buf_size (which should include the +1) would cause that check to fail. I think relying on the passed in size rather than the size in the structure is a better idea anyway, so it seems like someone should investigate whether that extra byte allocated for the buffer is necessary since it complicates things a little.

Sure, but the +1 should be removed, per our other discussion.

jhendersonHDF · 2025-01-15T17:59:22Z

Sure, but the +1 should be removed, per our other discussion.

I'd be happy to make those changes in a separate PR since I think it's mostly orthogonal and even though I can't imagine removing that extra byte will cause problems, I like to prepare for the worst.

qkoziol · 2025-01-15T18:20:31Z

Sure, but the +1 should be removed, per our other discussion.

I'd be happy to make those changes in a separate PR since I think it's mostly orthogonal and even though I can't imagine removing that extra byte will cause problems, I like to prepare for the worst.

Seems like extra work, but I'm fine with that.

oss-patch and others added 2 commits December 24, 2024 07:55

Fix vuln OSV-2023-77

c9110c7

Committing clang-format changes

8cebb9c

oss-patch requested review from bmribler, brtnfld, byrnHDF, derobins, fortnern, glennsong09, jhendersonHDF, lrknox, mattjala, qkoziol and vchoi-hdfgroup as code owners January 7, 2025 05:52

oss-patch changed the title ~~Fix~~ Fix vuln OSV-2023-77 Jan 7, 2025

bmribler self-assigned this Jan 7, 2025

bmribler added the Priority - 1. High 🔼 label Jan 7, 2025

jhendersonHDF reviewed Jan 8, 2025

View reviewed changes

oss-patch and others added 4 commits January 9, 2025 11:09

use H5_IS_BUFFER_OVERFLOW to check overflow

2d5377d

add size parameter

b4ebd70

Committing clang-format changes

7e54602

Merge pull request #10 from aled-ua/aled-ua-patch-test

64568ea

add an extra size paramete in H5C__decode_cache_image_header()

derobins added Component - C Library Core C library issues (usually in the src directory) Type - Bug / Bugfix labels Jan 13, 2025

mattjala previously approved these changes Jan 14, 2025

View reviewed changes

jhendersonHDF reviewed Jan 14, 2025

View reviewed changes

jhendersonHDF requested changes Jan 14, 2025

View reviewed changes

Fix the last valid byte in buf

79e7168

oss-patch dismissed mattjala’s stale review via 79e7168 January 15, 2025 12:40

jhendersonHDF approved these changes Jan 15, 2025

View reviewed changes

qkoziol reviewed Jan 15, 2025

View reviewed changes

qkoziol approved these changes Jan 15, 2025

View reviewed changes

bmribler removed their assignment Jan 15, 2025

lrknox merged commit 7f27ba8 into HDFGroup:develop Jan 15, 2025
76 checks passed

Uh oh!

Conversation

oss-patch commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

1. PR Title: Fix for Heap-Buffer-Overflow Vulnerability in HDF5 - OSV-2023-77

2. PR Description:

3. Sanitizer Report Summary:

4. Full Sanitizer Report:

5. Files Modified:

6. Patch Validation:

7. Links:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oss-patch Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhendersonHDF left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qkoziol commented Jan 15, 2025

Uh oh!

jhendersonHDF commented Jan 15, 2025

Uh oh!

qkoziol commented Jan 15, 2025

Uh oh!

jhendersonHDF commented Jan 15, 2025

Uh oh!

qkoziol commented Jan 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

oss-patch commented Jan 7, 2025 •

edited

Loading

oss-patch Jan 9, 2025 •

edited

Loading