From 52dcdc1d89925fe7e362b2cc9cae699773f9618e Mon Sep 17 00:00:00 2001 From: Hugo Lefeuvre Date: Sun, 8 Apr 2018 14:07:08 -0400 Subject: [PATCH] Fix NULL pointer dereference in TIFFPrintDirectory The TIFFPrintDirectory function relies on the following assumptions, supposed to be guaranteed by the specification: (a) A Transfer Function field is only present if the TIFF file has photometric type < 3. (b) If SamplesPerPixel > Color Channels, then the ExtraSamples field has count SamplesPerPixel - (Color Channels) and contains information about supplementary channels. While respect of (a) and (b) are essential for the well functioning of TIFFPrintDirectory, no checks are realized neither by the callee nor by TIFFPrintDirectory itself. Hence, following scenarios might happen and trigger the NULL pointer dereference: (1) TIFF File of photometric type 4 or more has illegal Transfer Function field. (2) TIFF File has photometric type 3 or less and defines a SamplesPerPixel field such that SamplesPerPixel > Color Channels without defining all extra samples in the ExtraSamples fields. In this patch, we address both issues with respect of the following principles: (A) In the case of (1), the defined transfer table should be printed safely even if it isn't 'legal'. This allows us to avoid expensive checks in TIFFPrintDirectory. Also, it is quite possible that an alternative photometric type would be developed (not part of the standard) and would allow definition of Transfer Table. We want libtiff to be able to handle this scenario out of the box. (B) In the case of (2), the transfer table should be printed at its right size, that is if TIFF file has photometric type Palette then the transfer table should have one row and not three, even if two extra samples are declared. In order to fulfill (A) we simply add a new 'i < 3' end condition to the broken TIFFPrintDirectory loop. This makes sure that in any case where (b) would be respected but not (a), everything stays fine. (B) is fulfilled by the loop condition 'i < td->td_samplesperpixel - td->td_extrasamples'. This is enough as long as (b) is respected. Naturally, we also make sure (b) is respected. This is done in the TIFFReadDirectory function by making sure any non-color channel is counted in ExtraSamples. This commit addresses CVE-2018-7456. --- libtiff/tif_dirread.c | 61 +++++++++++++++++++++++++++++++++++++++++++ libtiff/tif_print.c | 2 +- libtiff/tif_unix.c | 8 ++++++ libtiff/tif_win32.c | 8 ++++++ libtiff/tiffio.h | 1 + 5 files changed, 79 insertions(+), 1 deletion(-) diff --git a/libtiff/tif_dirread.c b/libtiff/tif_dirread.c index 6667228..4f07bc8 100644 --- a/libtiff/tif_dirread.c +++ b/libtiff/tif_dirread.c @@ -165,6 +165,7 @@ static int TIFFFetchStripThing(TIFF* tif, TIFFDirEntry* dir, uint32 nstrips, uin static int TIFFFetchSubjectDistance(TIFF*, TIFFDirEntry*); static void ChopUpSingleUncompressedStrip(TIFF*); static uint64 TIFFReadUInt64(const uint8 *value); +static int _TIFFGetMaxColorChannels(uint16 photometric); typedef union _UInt64Aligned_t { @@ -3415,6 +3416,34 @@ static void TIFFReadDirEntryOutputErr(TIFF* tif, enum TIFFReadDirEntryErr err, c } } +/* + * Return the maximum number of color channels specified for a given photometric + * type. 0 is returned if photometric type isn't supported or no default value + * is defined by the specification. + */ +static int _TIFFGetMaxColorChannels( uint16 photometric ) +{ + switch (photometric) { + case PHOTOMETRIC_PALETTE: + case PHOTOMETRIC_MINISWHITE: + case PHOTOMETRIC_MINISBLACK: + return 1; + case PHOTOMETRIC_YCBCR: + case PHOTOMETRIC_RGB: + case PHOTOMETRIC_CIELAB: + return 3; + case PHOTOMETRIC_SEPARATED: + case PHOTOMETRIC_MASK: + return 4; + case PHOTOMETRIC_LOGL: + case PHOTOMETRIC_LOGLUV: + case PHOTOMETRIC_ITULAB: + case PHOTOMETRIC_ICCLAB: + default: + return 0; + } +} + /* * Read the next TIFF directory from a file and convert it to the internal * format. We read directories sequentially. @@ -3430,6 +3459,7 @@ TIFFReadDirectory(TIFF* tif) const TIFFField* fip; uint32 fii=FAILED_FII; toff_t nextdiroff; + int color_channels; tif->tif_diroff=tif->tif_nextdiroff; if (!TIFFCheckDirOffset(tif,tif->tif_nextdiroff)) return 0; /* last offset or bad offset (IFD looping) */ @@ -3909,6 +3939,37 @@ TIFFReadDirectory(TIFF* tif) } } } + + /* + * Make sure all non-color channels are extrasamples. + * If it's not the case, define them as such. + */ + color_channels = _TIFFGetMaxColorChannels(tif->tif_dir.td_photometric); + if (color_channels && tif->tif_dir.td_samplesperpixel - tif->tif_dir.td_extrasamples > color_channels) { + uint16 old_extrasamples; + uint16 *new_sampleinfo; + + TIFFWarningExt(tif->tif_clientdata,module, "Sum of Photometric type-related " + "color channels and ExtraSamples doesn't match SamplesPerPixel. " + "Defining non-color channels as ExtraSamples."); + + old_extrasamples = tif->tif_dir.td_extrasamples; + tif->tif_dir.td_extrasamples = (tif->tif_dir.td_samplesperpixel - color_channels); + + // sampleinfo should contain information relative to these new extra samples + new_sampleinfo = (uint16*) _TIFFcalloc(tif->tif_dir.td_extrasamples, sizeof(uint16)); + if (!new_sampleinfo) { + TIFFErrorExt(tif->tif_clientdata, module, "Failed to allocate memory for " + "temporary new sampleinfo array (%d 16 bit elements)", + tif->tif_dir.td_extrasamples); + goto bad; + } + + memcpy(new_sampleinfo, tif->tif_dir.td_sampleinfo, old_extrasamples * sizeof(uint16)); + _TIFFsetShortArray(&tif->tif_dir.td_sampleinfo, new_sampleinfo, tif->tif_dir.td_extrasamples); + _TIFFfree(new_sampleinfo); + } + /* * Verify Palette image has a Colormap. */ diff --git a/libtiff/tif_print.c b/libtiff/tif_print.c index b323ef1..8c075e5 100644 --- a/libtiff/tif_print.c +++ b/libtiff/tif_print.c @@ -541,7 +541,7 @@ TIFFPrintDirectory(TIFF* tif, FILE* fd, long flags) for (l = 0; l < n; l++) { fprintf(fd, " %2lu: %5u", l, td->td_transferfunction[0][l]); - for (i = 1; i < td->td_samplesperpixel; i++) + for (i = 1; i < td->td_samplesperpixel - td->td_extrasamples && i < 3; i++) fprintf(fd, " %5u", td->td_transferfunction[i][l]); fputc('\n', fd); diff --git a/libtiff/tif_unix.c b/libtiff/tif_unix.c index 0a3a4a8..5c00917 100644 --- a/libtiff/tif_unix.c +++ b/libtiff/tif_unix.c @@ -262,6 +262,14 @@ _TIFFmalloc(tmsize_t s) return (malloc((size_t) s)); } +void* _TIFFcalloc(tmsize_t nmemb, tmsize_t siz) +{ + if( nmemb == 0 || siz == 0 ) + return ((void *) NULL); + + return calloc((size_t) nmemb, (size_t)siz); +} + void _TIFFfree(void* p) { diff --git a/libtiff/tif_win32.c b/libtiff/tif_win32.c index 4a2466f..ef6be08 100644 --- a/libtiff/tif_win32.c +++ b/libtiff/tif_win32.c @@ -334,6 +334,14 @@ _TIFFmalloc(tmsize_t s) return (malloc((size_t) s)); } +void* _TIFFcalloc(tmsize_t nmemb, tmsize_t siz) +{ + if( nmemb == 0 || siz == 0 ) + return ((void *) NULL); + + return calloc((size_t) nmemb, (size_t)siz); +} + void _TIFFfree(void* p) { diff --git a/libtiff/tiffio.h b/libtiff/tiffio.h index 038b670..9b39392 100644 --- a/libtiff/tiffio.h +++ b/libtiff/tiffio.h @@ -293,6 +293,7 @@ extern TIFFCodec* TIFFGetConfiguredCODECs(void); */ extern void* _TIFFmalloc(tmsize_t s); +extern void* _TIFFcalloc(tmsize_t nmemb, tmsize_t siz); extern void* _TIFFrealloc(void* p, tmsize_t s); extern void _TIFFmemset(void* p, int v, tmsize_t c); extern void _TIFFmemcpy(void* d, const void* s, tmsize_t c); -- 2.17.2