Lucio Andrés Illanes Albornoz daac7c
From e1cd2d7ab032e7fe80b4c13e07895194c8bac85e Mon Sep 17 00:00:00 2001
Lucio Andrés Illanes Albornoz daac7c
From: Brian May <brian@linuxpenguins.xyz>
Lucio Andrés Illanes Albornoz daac7c
Date: Thu, 7 Dec 2017 07:46:47 +1100
Lucio Andrés Illanes Albornoz daac7c
Subject: [PATCH 1/4] [PATCH] tiff2pdf: Fix CVE-2017-9935
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
Fix for http://bugzilla.maptools.org/show_bug.cgi?id=2704
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
This vulnerability - at least for the supplied test case - is because we
Lucio Andrés Illanes Albornoz daac7c
assume that a tiff will only have one transfer function that is the same
Lucio Andrés Illanes Albornoz daac7c
for all pages. This is not required by the TIFF standards.
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
We than read the transfer function for every page.  Depending on the
Lucio Andrés Illanes Albornoz daac7c
transfer function, we allocate either 2 or 4 bytes to the XREF buffer.
Lucio Andrés Illanes Albornoz daac7c
We allocate this memory after we read in the transfer function for the
Lucio Andrés Illanes Albornoz daac7c
page.
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
For the first exploit - POC1, this file has 3 pages. For the first page
Lucio Andrés Illanes Albornoz daac7c
we allocate 2 extra extra XREF entries. Then for the next page 2 more
Lucio Andrés Illanes Albornoz daac7c
entries. Then for the last page the transfer function changes and we
Lucio Andrés Illanes Albornoz daac7c
allocate 4 more entries.
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
When we read the file into memory, we assume we have 4 bytes extra for
Lucio Andrés Illanes Albornoz daac7c
each and every page (as per the last transfer function we read). Which
Lucio Andrés Illanes Albornoz daac7c
is not correct, we only have 2 bytes extra for the first 2 pages. As a
Lucio Andrés Illanes Albornoz daac7c
result, we end up writing past the end of the buffer.
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
There are also some related issues that this also fixes. For example,
Lucio Andrés Illanes Albornoz daac7c
TIFFGetField can return uninitalized pointer values, and the logic to
Lucio Andrés Illanes Albornoz daac7c
detect a N=3 vs N=1 transfer function seemed rather strange.
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
It is also strange that we declare the transfer functions to be of type
Lucio Andrés Illanes Albornoz daac7c
float, when the standard says they are unsigned 16 bit values. This is
Lucio Andrés Illanes Albornoz daac7c
fixed in another patch.
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
This patch will check to ensure that the N value for every transfer
Lucio Andrés Illanes Albornoz daac7c
function is the same for every page. If this changes, we abort with an
Lucio Andrés Illanes Albornoz daac7c
error. In theory, we should perhaps check that the transfer function
Lucio Andrés Illanes Albornoz daac7c
itself is identical for every page, however we don't do that due to the
Lucio Andrés Illanes Albornoz daac7c
confusion of the type of the data in the transfer function.
Lucio Andrés Illanes Albornoz daac7c
---
Lucio Andrés Illanes Albornoz daac7c
 libtiff/tif_dir.c |  3 +++
Lucio Andrés Illanes Albornoz daac7c
 tools/tiff2pdf.c  | 69 +++++++++++++++++++++++++++++++----------------
Lucio Andrés Illanes Albornoz daac7c
 2 files changed, 49 insertions(+), 23 deletions(-)
Lucio Andrés Illanes Albornoz daac7c
Lucio Andrés Illanes Albornoz daac7c
diff --git a/libtiff/tif_dir.c b/libtiff/tif_dir.c
Lucio Andrés Illanes Albornoz daac7c
index f00f808..c36a5f3 100644
Lucio Andrés Illanes Albornoz daac7c
--- a/libtiff/tif_dir.c
Lucio Andrés Illanes Albornoz daac7c
+++ b/libtiff/tif_dir.c
Lucio Andrés Illanes Albornoz daac7c
@@ -1067,6 +1067,9 @@ _TIFFVGetField(TIFF* tif, uint32 tag, va_list ap)
Lucio Andrés Illanes Albornoz daac7c
 			if (td->td_samplesperpixel - td->td_extrasamples > 1) {
Lucio Andrés Illanes Albornoz daac7c
 				*va_arg(ap, uint16**) = td->td_transferfunction[1];
Lucio Andrés Illanes Albornoz daac7c
 				*va_arg(ap, uint16**) = td->td_transferfunction[2];
Lucio Andrés Illanes Albornoz daac7c
+			} else {
Lucio Andrés Illanes Albornoz daac7c
+				*va_arg(ap, uint16**) = NULL;
Lucio Andrés Illanes Albornoz daac7c
+				*va_arg(ap, uint16**) = NULL;
Lucio Andrés Illanes Albornoz daac7c
 			}
Lucio Andrés Illanes Albornoz daac7c
 			break;
Lucio Andrés Illanes Albornoz daac7c
 		case TIFFTAG_REFERENCEBLACKWHITE:
Lucio Andrés Illanes Albornoz daac7c
diff --git a/tools/tiff2pdf.c b/tools/tiff2pdf.c
Lucio Andrés Illanes Albornoz daac7c
index bdb9126..bd23c9e 100644
Lucio Andrés Illanes Albornoz daac7c
--- a/tools/tiff2pdf.c
Lucio Andrés Illanes Albornoz daac7c
+++ b/tools/tiff2pdf.c
Lucio Andrés Illanes Albornoz daac7c
@@ -239,7 +239,7 @@ typedef struct {
Lucio Andrés Illanes Albornoz daac7c
 	float tiff_whitechromaticities[2];
Lucio Andrés Illanes Albornoz daac7c
 	float tiff_primarychromaticities[6];
Lucio Andrés Illanes Albornoz daac7c
 	float tiff_referenceblackwhite[2];
Lucio Andrés Illanes Albornoz daac7c
-	float* tiff_transferfunction[3];
Lucio Andrés Illanes Albornoz daac7c
+	uint16* tiff_transferfunction[3];
Lucio Andrés Illanes Albornoz daac7c
 	int pdf_image_interpolate;	/* 0 (default) : do not interpolate,
Lucio Andrés Illanes Albornoz daac7c
 					   1 : interpolate */
Lucio Andrés Illanes Albornoz daac7c
 	uint16 tiff_transferfunctioncount;
Lucio Andrés Illanes Albornoz daac7c
@@ -1049,6 +1049,8 @@ void t2p_read_tiff_init(T2P* t2p, TIFF* input){
Lucio Andrés Illanes Albornoz daac7c
 	uint16 pagen=0;
Lucio Andrés Illanes Albornoz daac7c
 	uint16 paged=0;
Lucio Andrés Illanes Albornoz daac7c
 	uint16 xuint16=0;
Lucio Andrés Illanes Albornoz daac7c
+	uint16 tiff_transferfunctioncount=0;
Lucio Andrés Illanes Albornoz daac7c
+	uint16* tiff_transferfunction[3];
Lucio Andrés Illanes Albornoz daac7c
 
Lucio Andrés Illanes Albornoz daac7c
 	directorycount=TIFFNumberOfDirectories(input);
Lucio Andrés Illanes Albornoz daac7c
 	if(directorycount > TIFF_DIR_MAX) {
Lucio Andrés Illanes Albornoz daac7c
@@ -1157,26 +1159,48 @@ void t2p_read_tiff_init(T2P* t2p, TIFF* input){
Lucio Andrés Illanes Albornoz daac7c
                 }
Lucio Andrés Illanes Albornoz daac7c
 #endif
Lucio Andrés Illanes Albornoz daac7c
 		if (TIFFGetField(input, TIFFTAG_TRANSFERFUNCTION,
Lucio Andrés Illanes Albornoz daac7c
-                                 &(t2p->tiff_transferfunction[0]),
Lucio Andrés Illanes Albornoz daac7c
-                                 &(t2p->tiff_transferfunction[1]),
Lucio Andrés Illanes Albornoz daac7c
-                                 &(t2p->tiff_transferfunction[2]))) {
Lucio Andrés Illanes Albornoz daac7c
-			if((t2p->tiff_transferfunction[1] != (float*) NULL) &&
Lucio Andrés Illanes Albornoz daac7c
-                           (t2p->tiff_transferfunction[2] != (float*) NULL) &&
Lucio Andrés Illanes Albornoz daac7c
-                           (t2p->tiff_transferfunction[1] !=
Lucio Andrés Illanes Albornoz daac7c
-                            t2p->tiff_transferfunction[0])) {
Lucio Andrés Illanes Albornoz daac7c
-				t2p->tiff_transferfunctioncount = 3;
Lucio Andrés Illanes Albornoz daac7c
-				t2p->tiff_pages[i].page_extra += 4;
Lucio Andrés Illanes Albornoz daac7c
-				t2p->pdf_xrefcount += 4;
Lucio Andrés Illanes Albornoz daac7c
-			} else {
Lucio Andrés Illanes Albornoz daac7c
-				t2p->tiff_transferfunctioncount = 1;
Lucio Andrés Illanes Albornoz daac7c
-				t2p->tiff_pages[i].page_extra += 2;
Lucio Andrés Illanes Albornoz daac7c
-				t2p->pdf_xrefcount += 2;
Lucio Andrés Illanes Albornoz daac7c
-			}
Lucio Andrés Illanes Albornoz daac7c
-			if(t2p->pdf_minorversion < 2)
Lucio Andrés Illanes Albornoz daac7c
-				t2p->pdf_minorversion = 2;
Lucio Andrés Illanes Albornoz daac7c
+                                 &(tiff_transferfunction[0]),
Lucio Andrés Illanes Albornoz daac7c
+                                 &(tiff_transferfunction[1]),
Lucio Andrés Illanes Albornoz daac7c
+                                 &(tiff_transferfunction[2]))) {
Lucio Andrés Illanes Albornoz daac7c
+
Lucio Andrés Illanes Albornoz daac7c
+                        if((tiff_transferfunction[1] != (uint16*) NULL) &&
Lucio Andrés Illanes Albornoz daac7c
+                           (tiff_transferfunction[2] != (uint16*) NULL)
Lucio Andrés Illanes Albornoz daac7c
+                          ) {
Lucio Andrés Illanes Albornoz daac7c
+                            tiff_transferfunctioncount=3;
Lucio Andrés Illanes Albornoz daac7c
+                        } else {
Lucio Andrés Illanes Albornoz daac7c
+                            tiff_transferfunctioncount=1;
Lucio Andrés Illanes Albornoz daac7c
+                        }
Lucio Andrés Illanes Albornoz daac7c
                 } else {
Lucio Andrés Illanes Albornoz daac7c
-			t2p->tiff_transferfunctioncount=0;
Lucio Andrés Illanes Albornoz daac7c
+			tiff_transferfunctioncount=0;
Lucio Andrés Illanes Albornoz daac7c
 		}
Lucio Andrés Illanes Albornoz daac7c
+
Lucio Andrés Illanes Albornoz daac7c
+                if (i > 0){
Lucio Andrés Illanes Albornoz daac7c
+                    if (tiff_transferfunctioncount != t2p->tiff_transferfunctioncount){
Lucio Andrés Illanes Albornoz daac7c
+                        TIFFError(
Lucio Andrés Illanes Albornoz daac7c
+                            TIFF2PDF_MODULE,
Lucio Andrés Illanes Albornoz daac7c
+                            "Different transfer function on page %d",
Lucio Andrés Illanes Albornoz daac7c
+                            i);
Lucio Andrés Illanes Albornoz daac7c
+                        t2p->t2p_error = T2P_ERR_ERROR;
Lucio Andrés Illanes Albornoz daac7c
+                        return;
Lucio Andrés Illanes Albornoz daac7c
+                    }
Lucio Andrés Illanes Albornoz daac7c
+                }
Lucio Andrés Illanes Albornoz daac7c
+
Lucio Andrés Illanes Albornoz daac7c
+                t2p->tiff_transferfunctioncount = tiff_transferfunctioncount;
Lucio Andrés Illanes Albornoz daac7c
+                t2p->tiff_transferfunction[0] = tiff_transferfunction[0];
Lucio Andrés Illanes Albornoz daac7c
+                t2p->tiff_transferfunction[1] = tiff_transferfunction[1];
Lucio Andrés Illanes Albornoz daac7c
+                t2p->tiff_transferfunction[2] = tiff_transferfunction[2];
Lucio Andrés Illanes Albornoz daac7c
+                if(tiff_transferfunctioncount == 3){
Lucio Andrés Illanes Albornoz daac7c
+                        t2p->tiff_pages[i].page_extra += 4;
Lucio Andrés Illanes Albornoz daac7c
+                        t2p->pdf_xrefcount += 4;
Lucio Andrés Illanes Albornoz daac7c
+                        if(t2p->pdf_minorversion < 2)
Lucio Andrés Illanes Albornoz daac7c
+                                t2p->pdf_minorversion = 2;
Lucio Andrés Illanes Albornoz daac7c
+                } else if (tiff_transferfunctioncount == 1){
Lucio Andrés Illanes Albornoz daac7c
+                        t2p->tiff_pages[i].page_extra += 2;
Lucio Andrés Illanes Albornoz daac7c
+                        t2p->pdf_xrefcount += 2;
Lucio Andrés Illanes Albornoz daac7c
+                        if(t2p->pdf_minorversion < 2)
Lucio Andrés Illanes Albornoz daac7c
+                                t2p->pdf_minorversion = 2;
Lucio Andrés Illanes Albornoz daac7c
+                }
Lucio Andrés Illanes Albornoz daac7c
+
Lucio Andrés Illanes Albornoz daac7c
 		if( TIFFGetField(
Lucio Andrés Illanes Albornoz daac7c
 			input, 
Lucio Andrés Illanes Albornoz daac7c
 			TIFFTAG_ICCPROFILE, 
Lucio Andrés Illanes Albornoz daac7c
@@ -1837,10 +1861,9 @@ void t2p_read_tiff_data(T2P* t2p, TIFF* input){
Lucio Andrés Illanes Albornoz daac7c
 			 &(t2p->tiff_transferfunction[0]),
Lucio Andrés Illanes Albornoz daac7c
 			 &(t2p->tiff_transferfunction[1]),
Lucio Andrés Illanes Albornoz daac7c
 			 &(t2p->tiff_transferfunction[2]))) {
Lucio Andrés Illanes Albornoz daac7c
-		if((t2p->tiff_transferfunction[1] != (float*) NULL) &&
Lucio Andrés Illanes Albornoz daac7c
-                   (t2p->tiff_transferfunction[2] != (float*) NULL) &&
Lucio Andrés Illanes Albornoz daac7c
-                   (t2p->tiff_transferfunction[1] !=
Lucio Andrés Illanes Albornoz daac7c
-                    t2p->tiff_transferfunction[0])) {
Lucio Andrés Illanes Albornoz daac7c
+		if((t2p->tiff_transferfunction[1] != (uint16*) NULL) &&
Lucio Andrés Illanes Albornoz daac7c
+                   (t2p->tiff_transferfunction[2] != (uint16*) NULL)
Lucio Andrés Illanes Albornoz daac7c
+                  ) {
Lucio Andrés Illanes Albornoz daac7c
 			t2p->tiff_transferfunctioncount=3;
Lucio Andrés Illanes Albornoz daac7c
 		} else {
Lucio Andrés Illanes Albornoz daac7c
 			t2p->tiff_transferfunctioncount=1;
Lucio Andrés Illanes Albornoz daac7c
-- 
Lucio Andrés Illanes Albornoz daac7c
2.17.0
Lucio Andrés Illanes Albornoz daac7c