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

2019-11-23 Thread Anton Gladky 
Hello Hugo,

thanks for update!

> Anton, you know this package better than me, would you be available to test 
> the update?
I am also not an expert in the package, but sure, I will try to do it.

Regards

Anton

Am Sa., 23. Nov. 2019 um 10:25 Uhr schrieb Hugo Lefeuvre :
>
> Hi,
>
> Upstream seems to have merged my patch along with some more changes
> regarding CVE-2019-12213[0].
>
> I am planning to take a look at this patch and release a DLA for jessie.
>
> The security team is also planning to release a DSA for stretch and buster.
> I am already working on a jessie upload, so I should also be able to handle
> stretch and buster.  Anton, you know this package better than me, would you
> be available to test the update?
>
> thanks!
>
> regards,
> Hugo
>
> [0] https://sourceforge.net/p/freeimage/svn/1825/
>
> --
> 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

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

2019-11-23 Thread Hugo Lefeuvre 
Hi,

Upstream seems to have merged my patch along with some more changes
regarding CVE-2019-12213[0].

I am planning to take a look at this patch and release a DLA for jessie.

The security team is also planning to release a DSA for stretch and buster.
I am already working on a jessie upload, so I should also be able to handle
stretch and buster.  Anton, you know this package better than me, would you
be available to test the update?

thanks!

regards,
Hugo

[0] https://sourceforge.net/p/freeimage/svn/1825/

-- 
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


signature.asc
Description: PGP signature

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

2019-11-03 Thread Hugo Lefeuvre 
Hi Anton,

> Thanks, Hugo, for analyzing the issue in details and proposing the fix.
> 
> Do you want to add the patch into the corresponding forum-thread
> in freeimage website?

yes, I have just forwarded my message to the SF thread. Let's hope upstream
will find some time to take a look at it.

cheers,
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


signature.asc
Description: PGP signature

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

2019-10-26 Thread Anton Gladky 
Thanks, Hugo, for analyzing the issue in details and proposing the fix.

Do you want to add the patch into the corresponding forum-thread
in freeimage website?

Regards

Anton

Am Sa., 26. Okt. 2019 um 16:11 Uhr schrieb 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

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

2019-10-26 Thread 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 
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