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

2016.12.17 15:15 "[Tiff] 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