CVE-2024-26540: heap-buffer-overflow in load_analyze(...)

[w1ldb1t]

Vulnerability Report

Summary

It is possible to cause a heap-buffer-overflow in CImg by passing a corrupted file as an input to the load_analyze function that is meant to process ANALYZE7.5/NIFTI files.

Details

The issue is present in the _load_analyze function, and it has to do with the fact that after reading the header_size variable, there is no check if its value is bigger than 4 bytes. Therefore, providing a header_size smaller than 4 bytes will make the first argument of cimg::fread point out of bounds of the header buffer, while it will subsequently underflow the second parameter (the size) passed to the function:

 CImg<T>& _load_analyze(std::FILE *const file, const char *const filename, float *const voxel_size=0) {
      // ...
      bool endian = false;
      unsigned int header_size;
      cimg::fread(&header_size,1,nfile_header); // <--- Read |header_size| from file
      // ...
      unsigned char *const header = new unsigned char[header_size];
      cimg::fread(header + 4,header_size - 4,nfile_header); // <-- Heap buffer overflow
      // ...
}

Due to how cimg::fread is implemented this will write all the contents of the corrupted file out of bounds of the header buffer, enabling an attacker to control both the size and the contents of the overflow.

Version

Tested on CImg 3.3.2

Reproduction

Simply call load_analyze(..) with the "corrupted" file attached in this issue as an input.

ASAN Report

==5028==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x100480020034 at pc 0x7ffcf265fdac bp 0x00000014f960 sp 0x00000014f0f0
WRITE of size 12 at 0x100480020034 thread T0
==5028==WARNING: Failed to use and restart external symbolizer!
    #0 0x7ffcf265fdab in _asan_wrap_GlobalSize+0x4cb55 (C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.36.32532\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x18004fdab)
    #1 0x7ffdd5ea6ce2 in fread_nolock_s+0xf2 (C:\Windows\System32\ucrtbase.dll+0x180006ce2)
    #2 0x7ffdd5ea6bad in fread_s+0x5d (C:\Windows\System32\ucrtbase.dll+0x180006bad)
    #3 0x7ffdd5ea6b37 in fread+0x17 (C:\Windows\System32\ucrtbase.dll+0x180006b37)
    #4 0x7ff6e9867010 in cimg_library::cimg::fread<unsigned char> C:\U\M\s\r\C\C\CImg.h:7595
    #5 0x7ff6e984fb3b in cimg_library::CImg<unsigned char>::_load_analyze C:\U\M\s\r\C\C\CImg.h:56046
    #6 0x7ff6e984de2b in main C:\U\M\s\r\C\C\CImgTest.cpp:14
    #7 0x7ff6e9872d43 in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #8 0x7ffdd640257c in BaseThreadInitThunk+0x1c (C:\Windows\System32\KERNEL32.DLL+0x18001257c)
    #9 0x7ffdd85eaa57 in RtlUserThreadStart+0x27 (C:\Windows\SYSTEM32\ntdll.dll+0x18005aa57)

0x100480020034 is located 3 bytes to the right of 1-byte region [0x100480020030,0x100480020031)
allocated by thread T0 here:
    #0 0x7ff6e9872357 in operator new[] D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_win_new_array_thunk.cpp:41
    #1 0x7ff6e984fb16 in cimg_library::CImg<unsigned char>::_load_analyze C:\U\M\s\r\C\C\CImg.h:56045
    #2 0x7ff6e984de2b in main C:\U\M\s\r\C\C\CImgTest.cpp:14
    #3 0x7ff6e9872d43 in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #4 0x7ffdd640257c in BaseThreadInitThunk+0x1c (C:\Windows\System32\KERNEL32.DLL+0x18001257c)
    #5 0x7ffdd85eaa57 in RtlUserThreadStart+0x27 (C:\Windows\SYSTEM32\ntdll.dll+0x18005aa57)

SUMMARY: AddressSanitizer: heap-buffer-overflow (C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.36.32532\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x18004fdab) in _asan_wrap_GlobalSize+0x4cb55
Shadow bytes around the buggy address:
  0x020110003fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x020110003fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x020110003fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x020110003fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x020110003ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x020110004000: fa fa fd fd fa fa[01]fa fa fa 00 00 fa fa 00 00
  0x020110004010: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x020110004020: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x020110004030: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x020110004040: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x020110004050: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

poc.zip

[dtschump]

Does 6a97a52 seem to be a good solution to you?

[w1ldb1t]

This will indeed fix the underflow issue that causes the cimg::fread heap-buffer-overflow, but I don't think that this fix is enough, mainly because the root cause of this bug (a bogus header_size) can still cause a number of issues in the rest of the function.

To give you an example, a header_size that has the value of 5 (which will satisfy the suggested fix) will allocate successfully the header buffer with that size, but such a small buffer will then cause a series of access violations later down the road where the code will try to access it assuming that it's bigger than it actually is.

For example here:

CImg<T>& _load_analyze(std::FILE *const file, const char *const filename, float *const voxel_size=0) {
   // ...
    if (endian) {
      cimg::invert_endianness((short*)(header + 40),5); // <--- Out of bounds access
      cimg::invert_endianness((short*)(header + 70),1); // ---
      cimg::invert_endianness((short*)(header + 72),1); // ---
      cimg::invert_endianness((float*)(header + 76),4); // ---
      cimg::invert_endianness((float*)(header + 108),1); // ---
      cimg::invert_endianness((float*)(header + 112),1); // ---
    }
    // ...
}    

Or here:

CImg<T>& _load_analyze(std::FILE *const file, const char *const filename, float *const voxel_size=0) {
      // ...
      if (nfile_header==nfile) {
        const unsigned int vox_offset = (unsigned int)*(float*)(header + 108); // <--- Out of bounds access
        std::fseek(nfile,vox_offset,SEEK_SET);
      }
      // ...
}

---

Since the code of _load_analyze seems to me that it accesses the header buffer at fixed offsets, one simple solution I believe is to increase the size of the check suggested above from 4 to the biggest offset used to access the header buffer.

Additionally, I believe that it's also crucial to check the return value of the fread that reads the header data from the file and writes it to the header buffer, and terminate/return if it's not equal to the header_size. That is because with the current function implementation, it's also possible to put a header_size bigger than the actual file size, which means that after fread reads the entirety of the file and puts it into the header buffer, it will leave the rest of the buffer uninitialized, essentially allowing _load_analyze to do all subsequent calculations based on uninitialized memory instead of user-defined data.

Let me know if you need any further clarification or reproduction cases/files.

[dtschump]

According to https://brainder.org/2012/09/23/the-nifti-file-format/, the header size for Analyze and NIFTI files should be 348 bytes, so a more strict check may be possible (let say at least header_size>128).
At least CImg function CImg<T>::save_analyze() always output a 348 bytes header.

My proposal : ec6a1f2

Can you tell me if that seems ok ?

[dtschump]

Sorry, cb9c551

[w1ldb1t]

Yes! The changes look good to me. Thank you for the quick responses regarding this issue :)

Best,
Michelangelo