AWARE SYSTEMS
TIFF and LibTiff Mail List Archive

Thread

2014.12.21 15:35 "[Tiff] For review", by Even Rouault
2014.12.21 18:49 "Re: [Tiff] For review", by Kai-Uwe Behrmann
2014.12.21 19:05 "Re: [Tiff] For review", by Even Rouault
2014.12.22 02:58 "Re: [Tiff] For review", by Bob Friesenhahn
2014.12.29 20:26 "[Tiff] [PATCH][cosmetics] Fix indentation for few last commits", by Yuriy M. Kaminskiy

2014.12.21 15:35 "[Tiff] For review", by Even Rouault

Hi,

In case someone wants to double-check, after painful investigations, I've committed the following changes to avoid crashes/Valgrind errors on a set of fuzzed images that Bob provided to me. I'm not too sure about the stuff related to LOGLUV/CIELAB/ITULAB (changes in libtiff/tif_getimage.c and tools/tiff2pdf.c), so review would be appreciated.

My feeling is that TIFFReadDirectory() is much too tolerant. It can return OK for really corrupted images, and this opens the door to a lot of potential errors in user code/utilities/other parts of the lib that must do later sanity checks, and hardly anybody is good at that. For example, my fix related to ColorMap is really due to accepting images with not increasingly sorted tags. If we had error out before, that issue would have never happened.

Idem for LOGLUV/CIELAB/ITULAB, if those colorspaces have indeed the restrictions I guessed on BitsPerSample/SamplesPerPixel, I feel we should reject them at TIFFReadDirectory() stage, rather than trying to process them in tiff2pdf or TIFFRGBAImageOK().

Even

2014-12-21  Even Rouault  <even.rouault@spatialys.com>

        Fix various crasher bugs on fuzzed images.
        * libtiff/tif_dir.c: TIFFSetField(): refuse to set negative values for
        TIFFTAG_XRESOLUTION and TIFFTAG_YRESOLUTION that cause asserts when 
writing
        the directory
        * libtiff/tif_dirread.c: TIFFReadDirectory(): refuse to read ColorMap or
        TransferFunction if BitsPerSample has not yet been read, otherwise 
reading
        it later will cause user code to crash if BitsPerSample > 1
        * libtiff/tif_getimage.c: TIFFRGBAImageOK(): return FALSE if LOGLUV with
        SamplesPerPixel != 3, or if CIELAB with SamplesPerPixel != 3 or 
BitsPerSample != 8
        * libtiff/tif_next.c: in the "run mode", use tilewidth for tiled images
        instead of imagewidth to avoid crash
        * tools/bmp2tiff.c: fix crash due to int overflow related to input BMP 
dimensions
        * tools/tiff2pdf.c: fix crash due to invalid tile count (should likely be 
checked by
        libtiff too). Detect invalid settings of BitsPerSample/SamplesPerPixel for 
CIELAB / ITULAB
        * tools/tiffcrop.c: fix crash due to invalid TileWidth/TileHeight
        * tools/tiffdump.c: fix crash due to overflow of entry count.

Index: libtiff/tif_dir.c

===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_dir.c,v
retrieving revision 1.117
diff -r1.117 tif_dir.c
162a163
>     double dblval;
287c288,291
<               td->td_xresolution = (float) va_arg(ap, double);
---
>         dblval = va_arg(ap, double);
>         if( dblval < 0 )
>             goto badvaluedouble;
>               td->td_xresolution = (float) dblval;
290c294,297
<               td->td_yresolution = (float) va_arg(ap, double);
---
>         dblval = va_arg(ap, double);
>         if( dblval < 0 )
>             goto badvaluedouble;
>               td->td_yresolution = (float) dblval;
696a704,713
> badvaluedouble:
>         {
>         const TIFFField* fip=TIFFFieldWithTag(tif,tag);
>         TIFFErrorExt(tif->tif_clientdata, module,
>              "%s: Bad value %f for \"%s\" tag",
>              tif->tif_name, dblval,
>              fip ? fip->field_name : "Unknown");
>         va_end(ap);
>         }
>     return (0);
Index: libtiff/tif_dirread.c
===================================================================

RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_dirread.c,v retrieving revision 1.180

diff -r1.180 tif_dirread.c
3432a3433,3434
> int bitspersample_read = FALSE;
>
3708a3711,3712
> if( dp->tdir_tag == TIFFTAG_BITSPERSAMPLE )
> bitspersample_read = TRUE;
3765a3770,3782
> /* It would be dangerous to instanciate those tag values
*/
> /* since if td_bitspersample has not yet been read (due
to */
> /* unordered tags), it could be read afterwards with a
*/
> /* values greater than the default one (1), which may
cause */

>                     /* crashes in user code */
>                     if( !bitspersample_read )
>                     {
>                         fip = TIFFFieldWithTag(tif,dp->tdir_tag);
>                         TIFFWarningExt(tif->tif_clientdata,module,

> "Ignoring %s since BitsPerSample tag
not found",
> fip? fip->field_name: "unknown
tagname");
> continue;
> }
Index: libtiff/tif_getimage.c

=================================================================== RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_getimage.c,v retrieving revision 1.82

diff -r1.82 tif_getimage.c
184a185,191
> if( td->td_samplesperpixel != 3 )

>             {
>                 sprintf(emsg,
>                         "Sorry, can not handle image with %s=%d",
>                         "Samples/pixel", td->td_samplesperpixel);
>                 return 0;
>             }
186a194,201
>             if( td->td_samplesperpixel != 3 || td->td_bitspersample != 8 )
>             {
>                 sprintf(emsg,
>                         "Sorry, can not handle image with %s=%d and %s=%d",
>                         "Samples/pixel", td->td_samplesperpixel,
>                         "Bits/sample", td->td_bitspersample);
>                 return 0;
>             }

Index: libtiff/tif_next.c

===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_next.c,v
retrieving revision 1.13
diff -r1.13 tif_next.c
104a105,106
>             if( isTiled(tif) )
>                 imagewidth = tif->tif_dir.td_tilewidth;
Index: tools/bmp2tiff.c
===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/tools/bmp2tiff.c,v
retrieving revision 1.23
diff -r1.23 bmp2tiff.c
405a406,412
>         if( width <= 0 || length <= 0 )
>         {
>             TIFFError(infilename,
>                   "Invalid dimensions of BMP file" );
>             close(fd);
>             return -1;
>         }
595a603,610
>             /* Detect int overflow */
>             if( uncompr_size / width != length )
>             {
>                 TIFFError(infilename,
>                     "Invalid dimensions of BMP file" );
>                 close(fd);
>                 return -1;
>             }
Index: tools/tiff2pdf.c
===================================================================

RCS file: /cvs/maptools/cvsroot/libtiff/tools/tiff2pdf.c,v retrieving revision 1.77

diff -r1.77 tiff2pdf.c
1169a1170,1178

>                 if( (t2p->tiff_tiles[i].tiles_tilecount % xuint16) != 0 )
>                 {
>                     TIFFError(
>                         TIFF2PDF_MODULE, 
>                         "Invalid tile count, %s", 
>                         TIFFFileName(input));
>                     t2p->t2p_error = T2P_ERR_ERROR;
>                     return;
>                 }
1554a1564,1579
>             if( t2p->tiff_samplesperpixel != 3){
>                 TIFFError(
>                     TIFF2PDF_MODULE, 
>                     "Unsupported samplesperpixel = %d for CIELAB", 
>                     t2p->tiff_samplesperpixel);
>                 t2p->t2p_error = T2P_ERR_ERROR;
>                 return;
>             }
>             if( t2p->tiff_bitspersample != 8){
>                 TIFFError(
>                     TIFF2PDF_MODULE, 
>                     "Invalid bitspersample = %d for CIELAB", 
>                     t2p->tiff_bitspersample);
>                 t2p->t2p_error = T2P_ERR_ERROR;
>                 return;
>             }
1569a1595,1610
>             if( t2p->tiff_samplesperpixel != 3){
>                 TIFFError(
>                     TIFF2PDF_MODULE, 
>                     "Unsupported samplesperpixel = %d for ITULAB", 
>                     t2p->tiff_samplesperpixel);
>                 t2p->t2p_error = T2P_ERR_ERROR;
>                 return;
>             }
>             if( t2p->tiff_bitspersample != 8){
>                 TIFFError(
>                     TIFF2PDF_MODULE, 
>                     "Invalid bitspersample = %d for ITULAB", 
>                     t2p->tiff_bitspersample);
>                 t2p->t2p_error = T2P_ERR_ERROR;
>                 return;
>             }

Index: tools/tiffcrop.c

===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/tools/tiffcrop.c,v
retrieving revision 1.23
diff -r1.23 tiffcrop.c
1208,1210c1208,1211
<   TIFFGetField(out, TIFFTAG_TILELENGTH, &tl);
<   TIFFGetField(out, TIFFTAG_TILEWIDTH, &tw);
<   TIFFGetField(out, TIFFTAG_BITSPERSAMPLE, &bps);
---
>   if( !TIFFGetField(out, TIFFTAG_TILELENGTH, &tl) ||
>       !TIFFGetField(out, TIFFTAG_TILEWIDTH, &tw) ||
>       !TIFFGetField(out, TIFFTAG_BITSPERSAMPLE, &bps) )
>       return 1;
Index: tools/tiffdump.c
===================================================================

RCS file: /cvs/maptools/cvsroot/libtiff/tools/tiffdump.c,v retrieving revision 1.28

diff -r1.28 tiffdump.c
376a377,378
> int datasizeoverflow;
>
414a417
> datasizeoverflow = (typewidth > 0 && datasize / typewidth != count);
421c424
< if (datasize>4)
---
> if (datasizeoverflow || datasize>4)
435c438
< if (datasize>8)
---
> if (datasizeoverflow || datasize>8)
445c448
< if (datasize>0x10000)
---

>               if (datasizeoverflow || datasize>0x10000)

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