Fix vuln OSV-2023-77

[aled-ua]

[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]

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.

[aled-ua]

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?

[jhendersonHDF]

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.