Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
From be4c85b16e8801a16eec25e80eb9f3dd6a96731b Mon Sep 17 00:00:00 2001
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
From: Hugo Lefeuvre <hle@debian.org>
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Date: Sun, 8 Apr 2018 14:07:08 -0400
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Subject: [PATCH] Fix NULL pointer dereference in TIFFPrintDirectory
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
The TIFFPrintDirectory function relies on the following assumptions,
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
supposed to be guaranteed by the specification:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(a) A Transfer Function field is only present if the TIFF file has
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    photometric type < 3.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(b) If SamplesPerPixel > Color Channels, then the ExtraSamples field
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    has count SamplesPerPixel - (Color Channels) and contains
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    information about supplementary channels.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
While respect of (a) and (b) are essential for the well functioning of
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
TIFFPrintDirectory, no checks are realized neither by the callee nor
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
by TIFFPrintDirectory itself. Hence, following scenarios might happen
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
and trigger the NULL pointer dereference:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(1) TIFF File of photometric type 4 or more has illegal Transfer
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    Function field.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(2) TIFF File has photometric type 3 or less and defines a
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    SamplesPerPixel field such that SamplesPerPixel > Color Channels
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    without defining all extra samples in the ExtraSamples fields.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
In this patch, we address both issues with respect of the following
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
principles:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(A) In the case of (1), the defined transfer table should be printed
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    safely even if it isn't 'legal'. This allows us to avoid expensive
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    checks in TIFFPrintDirectory. Also, it is quite possible that
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    an alternative photometric type would be developed (not part of the
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    standard) and would allow definition of Transfer Table. We want
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    libtiff to be able to handle this scenario out of the box.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(B) In the case of (2), the transfer table should be printed at its
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    right size, that is if TIFF file has photometric type Palette
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    then the transfer table should have one row and not three, even
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
    if two extra samples are declared.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
In order to fulfill (A) we simply add a new 'i < 3' end condition to
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
the broken TIFFPrintDirectory loop. This makes sure that in any case
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
where (b) would be respected but not (a), everything stays fine.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
(B) is fulfilled by the loop condition
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
'i < td->td_samplesperpixel - td->td_extrasamples'. This is enough as
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
long as (b) is respected.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Naturally, we also make sure (b) is respected. This is done in the
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
TIFFReadDirectory function by making sure any non-color channel is
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
counted in ExtraSamples.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
This commit addresses CVE-2018-7456.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
---
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 libtiff/tif_dirread.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 libtiff/tif_print.c   |  2 +-
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 2 files changed, 63 insertions(+), 1 deletion(-)
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
diff --git a/libtiff/tif_dirread.c b/libtiff/tif_dirread.c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
index 6baa7b3..af5b84a 100644
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
--- a/libtiff/tif_dirread.c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+++ b/libtiff/tif_dirread.c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
@@ -165,6 +165,7 @@ static int TIFFFetchStripThing(TIFF* tif, TIFFDirEntry* dir, uint32 nstrips, uin
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 static int TIFFFetchSubjectDistance(TIFF*, TIFFDirEntry*);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 static void ChopUpSingleUncompressedStrip(TIFF*);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 static uint64 TIFFReadUInt64(const uint8 *value);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+static int _TIFFGetMaxColorChannels(uint16 photometric);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 static int _TIFFFillStrilesInternal( TIFF *tif, int loadStripByteCount );
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
@@ -3505,6 +3506,35 @@ static void TIFFReadDirEntryOutputErr(TIFF* tif, enum TIFFReadDirEntryErr err, c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 }
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 /*
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+ * Return the maximum number of color channels specified for a given photometric
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+ * type. 0 is returned if photometric type isn't supported or no default value
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+ * is defined by the specification.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+ */
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+static int _TIFFGetMaxColorChannels( uint16 photometric )
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+{
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+    switch (photometric) {
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_PALETTE:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_MINISWHITE:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_MINISBLACK:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+            return 1;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_YCBCR:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_RGB:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_CIELAB:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+            return 3;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_SEPARATED:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_MASK:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+            return 4;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_LOGL:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_LOGLUV:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_CFA:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_ITULAB:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	case PHOTOMETRIC_ICCLAB:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	default:
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+            return 0;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+    }
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+}
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+/*
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
  * Read the next TIFF directory from a file and convert it to the internal
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
  * format. We read directories sequentially.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
  */
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
@@ -3520,6 +3550,7 @@ TIFFReadDirectory(TIFF* tif)
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	uint32 fii=FAILED_FII;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
         toff_t nextdiroff;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
     int bitspersample_read = FALSE;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+        int color_channels;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	tif->tif_diroff=tif->tif_nextdiroff;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	if (!TIFFCheckDirOffset(tif,tif->tif_nextdiroff))
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
@@ -4024,6 +4055,37 @@ TIFFReadDirectory(TIFF* tif)
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 			}
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 		}
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	}
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	/*
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	 * Make sure all non-color channels are extrasamples.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	 * If it's not the case, define them as such.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+	 */
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+        color_channels = _TIFFGetMaxColorChannels(tif->tif_dir.td_photometric);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+        if (color_channels && tif->tif_dir.td_samplesperpixel - tif->tif_dir.td_extrasamples > color_channels) {
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                uint16 old_extrasamples;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                uint16 *new_sampleinfo;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                TIFFWarningExt(tif->tif_clientdata,module, "Sum of Photometric type-related "
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                    "color channels and ExtraSamples doesn't match SamplesPerPixel. "
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                    "Defining non-color channels as ExtraSamples.");
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                old_extrasamples = tif->tif_dir.td_extrasamples;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                tif->tif_dir.td_extrasamples = (tif->tif_dir.td_samplesperpixel - color_channels);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                // sampleinfo should contain information relative to these new extra samples
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                new_sampleinfo = (uint16*) _TIFFcalloc(tif->tif_dir.td_extrasamples, sizeof(uint16));
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                if (!new_sampleinfo) {
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                    TIFFErrorExt(tif->tif_clientdata, module, "Failed to allocate memory for "
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                                "temporary new sampleinfo array (%d 16 bit elements)",
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                                tif->tif_dir.td_extrasamples);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                    goto bad;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                }
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                memcpy(new_sampleinfo, tif->tif_dir.td_sampleinfo, old_extrasamples * sizeof(uint16));
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                _TIFFsetShortArray(&tif->tif_dir.td_sampleinfo, new_sampleinfo, tif->tif_dir.td_extrasamples);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+                _TIFFfree(new_sampleinfo);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+        }
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	/*
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	 * Verify Palette image has a Colormap.
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 	 */
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
diff --git a/libtiff/tif_print.c b/libtiff/tif_print.c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
index 8deceb2..1d86adb 100644
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
--- a/libtiff/tif_print.c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+++ b/libtiff/tif_print.c
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
@@ -544,7 +544,7 @@ TIFFPrintDirectory(TIFF* tif, FILE* fd, long flags)
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 				uint16 i;
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 				fprintf(fd, "    %2ld: %5u",
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 				    l, td->td_transferfunction[0][l]);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
-				for (i = 1; i < td->td_samplesperpixel; i++)
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
+				for (i = 1; i < td->td_samplesperpixel - td->td_extrasamples && i < 3; i++)
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 					fprintf(fd, " %5u",
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 					    td->td_transferfunction[i][l]);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
 				fputc('\n', fd);
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
--
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc
libgit2 0.27.0
Lucio Andrés Illanes Albornoz (arab, vxp) af7abc