Buffer Overflow in get_load_jpeg_buffer

[francobel]

The vulnerable get_load_jpeg_buffer function in file jpeg_buffer.h (https://github.com/GreycLab/CImg/blob/master/plugins/jpeg_buffer.h#L227) is used to decompress jpegs and create a raw bitmap version of the image.

In get_load_jpeg_buffer, the values for cinfo.output_width and cinfo.output_height are retrieved directly from a jpeg file's header.

cinfo.output_width and cinfo.output_height can be manipulated by editing the header of the jpeg file being processed. They are two bytes each in the image's header so their values can range from 0x0000 to 0xFFFF. These variables are multiplied with cinfo.output_components.

When these three values are multiplied together they can exceed the limit of a 32-bit unsigned integer, leading to an integer overflow vulnerability. This product is used to set the size of the buf array, which will store the decompressed jpeg (https://github.com/GreycLab/CImg/blob/master/plugins/jpeg_buffer.h#L237). When the sizing arguments overflow, the array becomes too small to store the decompressed data.

The program writes the decompressed image to the array using the jpeg_read_scanlines function. The function ends up writing to out-of-bounds memory due to the array’s small size (https://github.com/GreycLab/CImg/blob/master/plugins/jpeg_buffer.h#L242). This causes data in memory adjacent to the array to be overwritten.

An attacker is in control of the image's height, width, and contents. This allows an attacker to craft an exploit to overwrite data in memory with data they control.

[dtschump]

Not much time to look at this right now. As you seem to understand the issue quite well, is this possible to get a PR about that?

[AlexSutila]

Hello, this particular issue caught my eye and I decided to do a little experimentation.

I was able to re-create the issue described up above by tampering with the header on an existing jpeg file. After looking through the code, it doesn't look like there is super fine control over tampering with cinfo.output_components as it looks like there are a small set of expected values for this particular field (I could be wrong about this) however cinfo.output_width and cinfo.output_height are a lot easier to control with a wider range of possible values.

I was able to produce a crash on a 32-bit architecture after tampering with the header and setting both the width and height to large values (the cinfo.output_components was 3 in my case). The following calculation is used to determine how much memory to allocate on the heap 0x93CC * 0x93CF * 0x3 = 0x10000f1dc and 0x10000f1dc > 0xffffffff leading to only 61916 bytes being allocated for what should be a pretty large image - so the buffer is too small and will overflow within the following while loop:

  while (cinfo.output_scanline < cinfo.output_height) {
    row_pointer[0] = buf + cinfo.output_scanline*row_stride;
    jpeg_read_scanlines(&cinfo,row_pointer,1);
  }

Here is what ends up happening using the use_jpeg_buffer.cpp example provided using the tampered image I was describing above:

 - Reading file 'foo.jpg'
 - Construct input JPEG-coded buffer
 - Create CImg instance from JPEG-coded buffer
Corrupt JPEG data: premature end of data segment
Segmentation fault (core dumped)

The reason for the segfault is because during this while loop and the jpeg I'm using, I've managed to not only overflow buf but overwrite everything from the start of buf all the way until the end of the pages allocated for the heap. I've verified this in a debugger:

By using CImg::safe_size, which is already exists in CImg.h, it's possible to prevent this and have it abort in a more graceful manner via a CImgArgumentException instead of just crashing (and also it should consequentially eliminate the risk of heap corruption if someone is able to be a little more careful with how far they write).

With modification - CImg::safe_size usage in get_load_jpeg_buffer:

 - Reading file 'foo.jpg'
 - Construct input JPEG-coded buffer
 - Create CImg instance from JPEG-coded buffer

[CImg] *** CImgArgumentException *** CImg<uint8>::safe_size(): Specified size (37839,37836,1,3) overflows 'size_t'.
terminate called after throwing an instance of 'cimg_library::CImgArgumentException'
  what():  CImg<uint8>::safe_size(): Specified size (37839,37836,1,3) overflows 'size_t'.
Aborted (core dumped)

I would be more than willing to submit a PR as the changes are quite minimal :)

[dtschump]

Thanks for your report.
Unfortunately I've no time to look at it in details right now, but a PR would be highly appreciated!
(if possible for the develop branch).

Thanks!