AWARE [SYSTEMS] Imaging expertise for the Delphi developer
AWare Systems, Imaging expertise for the Delphi developer, Home TIFF and LibTiff Mailing List Archive

LibTiff Mailing List

TIFF and LibTiff Mailing List Archive
December 2016

Previous Thread
Next Thread

Previous by Thread
Next by Thread

Previous by Date
Next by Date

Contact

The TIFF Mailing List Homepage
This list is run by Frank Warmerdam
Archive maintained by AWare Systems



Valid HTML 4.01!



Thread

2016.12.17 15:15 "Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?", by Even Rouault
2016.12.17 18:46 "Re: =?utf-8?q?Thoughts_about_a_proposed_patch_to_fix_a_heap-ba?= =?utf-8?q?sed_buffer=09overflow_in_putcontig8bitYCbCr44tile_=3F?=", by Mickey Rose
2016.12.17 20:35 "Re: Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?", by Even Rouault

2016.12.17 15:15 "Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?", by Even Rouault

Hi,

I've spent a couple of hours analyzing the issue raised on
http://bugzilla.maptools.org/show_bug.cgi?id=2637

This is a defect in tif_getimage.c when decoding a tiled old-JPEG
with YCbCr 4:4 resampling, and with a unusual tile width of 3.

==28237== Invalid read of size 1
==28237==    at 0x5A53926: putcontig8bitYCbCr44tile (tif_getimage.c:1885)
==28237==    by 0x5A4D878: gtTileContig (tif_getimage.c:694)
==28237==    by 0x5A4D104: TIFFRGBAImageGet (tif_getimage.c:516)
==28237==    by 0x5A57A09: TIFFReadRGBATile (tif_getimage.c:2933)
==28237==  Address 0x256f86c2 is 18 bytes after a block of size 9,216
alloc'd
==28237==    at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28237==    by 0x5A806BF: TIFFmalloc (tif_vsi.c:159)
==28237==    by 0x5A4D550: gtTileContig (tif_getimage.c:641)
==28237==    by 0x5A4D104: TIFFRGBAImageGet (tif_getimage.c:516)
==28237==    by 0x5A57A09: TIFFReadRGBATile (tif_getimage.c:2933)

The issue arises on "int32 Cb = pp[16];" line 1885, when dealing with the
right
most tile (the image width is 20, so the right most tile has 2 valid
pixels). This
is due to the pp pointer (pointer in the input tile data)  being too much
advanced
at line 1934: pp += fromskew.
fromskep is computed at line 1845 with fromskew = (fromskew * 18) / 4;
The input value of fromskew is 1 (since the right most tile has 1 invalid
pixel), hence
the resulting fromskew value is 4.

putcontig8bitYCbCr44tile has an optimized case when the useful width and
height are
multiple of 4, and here we are in the general case when they are not. So
apparently
this code is supposed to deal with useful width not multiple of 4. I cannot
really
make sense of the (fromskew * 18) / 4 computation (18=4x4+2 and 4 must be
the horizontal
or vertical resampling factor) and the pp += fromskew (note that
those are also used in the optimized case ). Interestingly in the 2x2
resampling case,
the formula is (fromskew / 2) * 6, so the division there is made before the
computation.
So perhaps a fix would be (fromskew / 4) * 18 ?, which would evaluate to 0
here
and avoid the overflow.

But I cannot verify if that doesnt break valid images. Has
someone legit OJPEG tiled 4:4 images... ? I tried to create a regular JPEG
tiled 4:4, with
tile of dimension 32x32 (otherwise this is rejected), and of image dimension
63x63, but this
is kind of fun since by default libjpeg has a
C_MAX_BLOCKS_IN_MCU/D_MAX_BLOCKS_IN_MCU limit
to 10 that prevents that (you need to increase it to 20 for example) (I'm
wondering why
libjpeg doesn't complain for the OJPEG image!), and then you need to
disable the following lines (around line 400) in tif_getimage.c :
					case COMPRESSION_JPEG:
						/*
						 * TODO: when complete tests verify complete desubsampling
						 * and YCbCr handling, remove use of TIFFTAG_JPEGCOLORMODE in
						 * favor of tif_getimage.c native handling
						 */
						//TIFFSetField(tif, TIFFTAG_JPEGCOLORMODE, JPEGCOLORMODE_RGB);
						//img->photometric = PHOTOMETRIC_RGB;
When doing all the above, my image (test_jpeg_ycbcr_44_tile_32_dim_63.tif
attached)
decodes without Valgrind warning with putcontig8bitYCbCr44tile
unmodified,  but the colors are rather funky, whereas the YCbCr to RGB
conversion done in libjpeg results in correct colors. So I'm not sure if
putcontig8bitYCbCr44tile
works at all...

I finally came with the following proposed patch that completely rejects the
decoding
of the image of the ticket, because the tile width or height is not a
multiple of 16 (there's a
warning in tif_dir.c about that). I'm not completely sure it is correct :
could that
break legit images ? and perhaps there are variations of the test image that
would pass
those new tests but still have issues in putcontig8bitYCbCr44tile (for
example ?

Index: libtiff/tif_getimage.c
===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_getimage.c,v
retrieving revision 1.99
diff -u -r1.99 tif_getimage.c
--- libtiff/tif_getimage.c	20 Nov 2016 22:29:47 -0000	1.99
+++ libtiff/tif_getimage.c	17 Dec 2016 14:00:17 -0000
@@ -2637,6 +2637,19 @@
 		case PHOTOMETRIC_YCBCR:
 			if ((img->bitspersample==8) && (img->samplesperpixel==3))
 			{
+                                if( TIFFIsTiled(img->tif) )
+                                {
+                                    uint32 tw, th;
+                                    TIFFGetField(img->tif,
TIFFTAG_TILEWIDTH, &tw);
+                                    TIFFGetField(img->tif,
TIFFTAG_TILEWIDTH, &th);
+                                    if( (tw % 16) != 0 || (th % 16) != 0 )
+                                    {
+                                       
TIFFErrorExt(img->tif->tif_clientdata, TIFFFileName(img->tif),
+                                                     "Nonstandard tile
dimension not handled for YCbCr decoding");
+                                        return 0;
+                                    }
+                                }
+
 				if (initYCbCrConversion(img)!=0)
 				{
 					/*

Even



-- 
Spatialys - Geospatial professional services
http://www.spatialys.com