Bug#929597: [PATCH] CVE-2019-12211: heap buffer overflow via memcpy

[Hugo Lefeuvre]

Hi,

The overflow happens during the following call to memcpy:

    // convert to strip
    if(x + tileWidth > width) {
            src_line = imageRowSize - rowSize;
    } else {
            src_line = tileRowSize;
    }
    BYTE *src_bits = tileBuffer;
    BYTE *dst_bits = bits + rowSize;
    for(int k = 0; k < nrows; k++) {
            memcpy(dst_bits, src_bits, src_line);
            src_bits += tileRowSize;
            dst_bits -= dst_pitch;
    }

This portion of code copies image data from a libTIFF-provided buffer to an
internal buffer. The overflow happens because src_line is larger than the
size of dst_bits.

This is the result of an inconsistency between libTIFF and freeimage:

In the libTIFF case, tile row size is
= samplesperpixel * bitspersample * tilewidth / 8
= bitsperpixel * tilewidth / 8
= 6 * 32 * 7 / 8 = 168

In the freeimage case, tile row size is
bitsperpixel * tilewidth / 8
= 32 * 7 / 8 = 28

As a result, the two buffers are differently sized.

freeimage has a bpp of 32 because CreateImageType calls
FreeImage_AllocateHeader with MIN(bpp, 32).

This 'MIN(bpp, 32)' looks like a terrible hack to me, but we can't change
it to 'bpp' because FIT_BITMAP images with bpp > 32 does not seem to be
supported by freeimage. Also, in this case, bpp > 32 doesn't even make
sense:

Looking closely at the reproducer, we can notice that it defines a bilevel
image with samplesperpixel and bitspersample parameters, both unexpected in
bilevel images.

Pixels in bilevel images can either be black or white. There is as such
only one sample per pixel, and a single bit per sample is sufficient.  The
spec defines bpp = 8. It is unclear whether the specification allows for
arbitrary values of bitspersample or samplesperpixel (extrasamples?) in
this case.

This file gets rejected by most libTIFF tools.

# patch

+ add check to CreateImageType() to reject FIT_BITMAP images with bpp > 32
  instead of passing MIN(bpp, 32).
+ change type of dst_pitch to unsigned
+ call memcpy with MIN(dst_pitch, src_line) instead of src_line. this will
  help overcome any further (future) discrepancy between libTIFF and
  freeimage.

# tests

I have tested for regressions with the following samples, using a modified
version of Examples/Linux/linux-gtk.c:

http://www.simplesystems.org/libtiff/images.html

During these tests, I found other issues with bilevel images, unrelated to
this patch. I will try to take a look at them in the future.

I can provide additional explanations if there is anything unclear.

I'd like to get this patch peer-reviewed/merged upstream before shipping
it in a Debian release.

regards,
Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

Description: fix heap buffer overflow when bpp > 32 and fit == FIT_BITMAP
 + add check to CreateImageType() to reject FIT_BITMAP images with bpp > 32
   instead of passing MIN(bpp, 32).
 + change type of dst_pitch to unsigned.
 + call memcpy with MIN(dst_pitch, src_line) instead of src_line. this will
   help overcome any further (future) discrepancy between libTIFF and
   freeimage.
Author: Hugo Lefeuvre <h...@debian.org>
Bug-Debian: https://bugs.debian.org/929597
--- a/Source/FreeImage/PluginTIFF.cpp	2019-10-26 14:21:39.329052757 +0200
+++ b/Source/FreeImage/PluginTIFF.cpp	2019-10-26 15:03:18.597957090 +0200
@@ -461,8 +461,12 @@
 			
 		}
 		else {
+			if(bpp > 32) {
+				// check for malicious images
+				return NULL;
+			}
 
-			dib = FreeImage_AllocateHeader(header_only, width, height, MIN(bpp, 32), FI_RGBA_RED_MASK, FI_RGBA_GREEN_MASK, FI_RGBA_BLUE_MASK);
+			dib = FreeImage_AllocateHeader(header_only, width, height, bpp, FI_RGBA_RED_MASK, FI_RGBA_GREEN_MASK, FI_RGBA_BLUE_MASK);
 		}
 
 
@@ -2041,7 +2045,7 @@
 				}
 
 				// calculate src line and dst pitch
-				int dst_pitch = FreeImage_GetPitch(dib);
+				unsigned int dst_pitch = FreeImage_GetPitch(dib);
 				uint32 tileRowSize = (uint32)TIFFTileRowSize(tif);
 				uint32 imageRowSize = (uint32)TIFFScanlineSize(tif);
 
@@ -2071,7 +2075,7 @@
 						BYTE *src_bits = tileBuffer;
 						BYTE *dst_bits = bits + rowSize;
 						for(int k = 0; k < nrows; k++) {
-							memcpy(dst_bits, src_bits, src_line);
+							memcpy(dst_bits, src_bits, MIN(dst_pitch, src_line));
 							src_bits += tileRowSize;
 							dst_bits -= dst_pitch;
 						}

signature.asc
Description: PGP signature