Closed w1ldb1t closed 7 months ago
Does https://github.com/GreycLab/CImg/commit/6a97a5209987e60fcce293ea102a068a88085098 seem to be a good solution to you?
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.
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 : https://github.com/GreycLab/CImg/commit/ec6a1f2183620a90b4dcf456813e597ade791dc6
Can you tell me if that seems ok ?
Yes! The changes look good to me. Thank you for the quick responses regarding this issue :)
Best, Michelangelo
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 processANALYZE7.5/NIFTI
files.Details
The issue is present in the
_load_analyze
function, and it has to do with the fact that after reading theheader_size
variable, there is no check if its value is bigger than 4 bytes. Therefore, providing aheader_size
smaller than 4 bytes will make the first argument ofcimg::fread
point out of bounds of theheader
buffer, while it will subsequently underflow the second parameter (the size) passed to the function:Due to how
cimg::fread
is implemented this will write all the contents of the corrupted file out of bounds of theheader
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
poc.zip