Stack Buffer Overflow with BMP files - Version 1.9.13

[Voiddy-Dev]

The fix for the issue stack buffer overflow before 1.9.13 does not completely protect against a stack buffer overflow in image_load_bmp().

It is possible to control the read in buffer colormap through the colors_used variable. The previous fix does not mitigate the issue as the color_used is an integer, therefore, regardless of the unsigned return of read_dword(fp) the buffer can be overflowed.

static int			/* O - 0 = success, -1 = fail */
image_load_bmp(image_t *img,	/* I - Image to load into */
               FILE    *fp,	/* I - File to read from */
	       int     gray,	/* I - Grayscale image? */
               int     load_data)/* I - 1 = load image data, 0 = just info */
{
  ...

  uchar		colormap[256][4];
  ...
  colors_used      = (int)read_dword(fp);
  ...
  // Get colormap...
  if (colors_used == 0 && depth <= 8)
    colors_used = 1 << depth;
  else if (colors_used > 256)  // colors_used => 0xffffff00 --> -0x100
    return (-1);
  ...
  fread(colormap, colors_used, 4, fp);
  ...
}

As an example, if colors_used is 0xffffff00 the if statement validates the variable and leads to a buffer overflow.

Impact

• This buffer overflow can lead to modifying the instruction pointer and can therefore lead to remote code execution.
  

POC: vuln_htmldoc_1.9.13.zip

[michaelrsweet]

I'm beginning to think that the best solution for this is to drop BMP support entirely - it is a dead format and I've spent far more time fixing potential security issues than I'd like...

[michaelrsweet]

[master 753c71b] Fix another potential stack overflow issue in the BMP image loader (Issue #456)