CVE-2016-10270: Out-of-bounds Read (CWE-125)
value: LibTIFF 4.0.7 allows remote attackers to cause a denial of service (heap-based buffer over-read) or possibly have unspecified other impact via a crafted TIFF image, related to "READ of size 8" and libtiff/tif_read.c:523:22.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.8
Published: 2017-03-24T19:59Z
Repository: https://github.com/vadz/libtiff
Language: C
Files changed: ChangeLog, libtiff/tif_dirread.c, libtiff/tif_strip.c
Commit date: 2016-12-03 11:02:15 +0000
Commit message: * libtiff/tif_dirread.c: modify ChopUpSingleUncompressedStrip() to
instanciate compute ntrips as TIFFhowmany_32(td->td_imagelength, rowsperstrip),
instead of a logic based on the total size of data. Which is faulty is
the total size of data is not sufficient to fill the whole image, and thus
results in reading outside of the StripByCounts/StripOffsets arrays when
using TIFFReadScanline().
Reported by Agostino Sarubbo.
Fixes http://bugzilla.maptools.org/show_bug.cgi?id=2608.

* libtiff/tif_strip.c: revert the change in TIFFNumberOfStrips() done
for http://bugzilla.maptools.org/show_bug.cgi?id=2587 / CVE-2016-9273 since
the above change is a better fix that makes it unnecessary.

Diff: diff --git a/ChangeLog b/ChangeLog
index 93c01f80e..9dbc7a0cf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,20 @@
+2016-12-03 Even Rouault <even.rouault at spatialys.com>
+
+	* libtiff/tif_dirread.c: modify ChopUpSingleUncompressedStrip() to
+	instanciate compute ntrips as TIFFhowmany_32(td->td_imagelength, rowsperstrip),
+	instead of a logic based on the total size of data. Which is faulty is
+	the total size of data is not sufficient to fill the whole image, and thus
+	results in reading outside of the StripByCounts/StripOffsets arrays when
+	using TIFFReadScanline().
+	Reported by Agostino Sarubbo.
+	Fixes http://bugzilla.maptools.org/show_bug.cgi?id=2608.
+
+	* libtiff/tif_strip.c: revert the change in TIFFNumberOfStrips() done
+	for http://bugzilla.maptools.org/show_bug.cgi?id=2587 / CVE-2016-9273 since
+	the above change is a better fix that makes it unnecessary.
+
 2016-12-03 Even Rouault <even.rouault at spatialys.com>
 
 	* libtiff/tif_pixarlog.c, libtiff/tif_luv.c: fix heap-based buffer
 	overflow on generation of PixarLog / LUV compressed files, with
 	ColorMap, TransferFunction attached and nasty plays with bitspersample.
diff --git a/libtiff/tif_dirread.c b/libtiff/tif_dirread.c
index 3eec79c9d..570d0c327 100644
--- a/libtiff/tif_dirread.c
+++ b/libtiff/tif_dirread.c
@@ -5500,12 +5500,11 @@ ChopUpSingleUncompressedStrip(TIFF* tif)
 	uint64 offset;
 	uint32 rowblock;
 	uint64 rowblockbytes;
 	uint64 stripbytes;
 	uint32 strip;
-	uint64 nstrips64;
-	uint32 nstrips32;
+	uint32 nstrips;
 	uint32 rowsperstrip;
 	uint64* newcounts;
 	uint64* newoffsets;
 
 	bytecount = td->td_stripbytecount[0];
@@ -5532,22 +5531,21 @@ ChopUpSingleUncompressedStrip(TIFF* tif)
 	}
 	else
 	    return;
 
 	/*
-	 * never increase the number of strips in an image
+	 * never increase the number of rows per strip
 	 */
 	if (rowsperstrip >= td->td_rowsperstrip)
 		return;
-	nstrips64 = TIFFhowmany_64(bytecount, stripbytes);
-	if ((nstrips64==0)||(nstrips64>0xFFFFFFFF)) /* something is wonky, do nothing. */
-	    return;
-	nstrips32 = (uint32)nstrips64;
+        nstrips = TIFFhowmany_32(td->td_imagelength, rowsperstrip);
+        if( nstrips == 0 )
+            return;
 
-	newcounts = (uint64*) _TIFFCheckMalloc(tif, nstrips32, sizeof (uint64),
+	newcounts = (uint64*) _TIFFCheckMalloc(tif, nstrips, sizeof (uint64),
 				"for chopped \"StripByteCounts\" array");
-	newoffsets = (uint64*) _TIFFCheckMalloc(tif, nstrips32, sizeof (uint64),
+	newoffsets = (uint64*) _TIFFCheckMalloc(tif, nstrips, sizeof (uint64),
 				"for chopped \"StripOffsets\" array");
 	if (newcounts == NULL || newoffsets == NULL) {
 		/*
 		 * Unable to allocate new strip information, give up and use
 		 * the original one strip information.
@@ -5560,22 +5558,22 @@ ChopUpSingleUncompressedStrip(TIFF* tif)
 	}
 	/*
 	 * Fill the strip information arrays with new bytecounts and offsets
 	 * that reflect the broken-up format.
 	 */
-	for (strip = 0; strip < nstrips32; strip++) {
+	for (strip = 0; strip < nstrips; strip++) {
 		if (stripbytes > bytecount)
 			stripbytes = bytecount;
 		newcounts[strip] = stripbytes;
-		newoffsets[strip] = offset;
+		newoffsets[strip] = stripbytes ? offset : 0;
 		offset += stripbytes;
 		bytecount -= stripbytes;
 	}
 	/*
 	 * Replace old single strip info with multi-strip info.
 	 */
-	td->td_stripsperimage = td->td_nstrips = nstrips32;
+	td->td_stripsperimage = td->td_nstrips = nstrips;
 	TIFFSetField(tif, TIFFTAG_ROWSPERSTRIP, rowsperstrip);
 
 	_TIFFfree(td->td_stripbytecount);
 	_TIFFfree(td->td_stripoffset);
 	td->td_stripbytecount = newcounts;
diff --git a/libtiff/tif_strip.c b/libtiff/tif_strip.c
index 4c46ecf5a..1676e47d3 100644
--- a/libtiff/tif_strip.c
+++ b/libtiff/tif_strip.c
@@ -61,19 +61,10 @@ uint32
 TIFFNumberOfStrips(TIFF* tif)
 {
 	TIFFDirectory *td = &tif->tif_dir;
 	uint32 nstrips;
 
-    /* If the value was already computed and store in td_nstrips, then return it,
-       since ChopUpSingleUncompressedStrip might have altered and resized the
-       since the td_stripbytecount and td_stripoffset arrays to the new value
-       after the initial affectation of td_nstrips = TIFFNumberOfStrips() in
-       tif_dirread.c ~line 3612.
-       See http://bugzilla.maptools.org/show_bug.cgi?id=2587 */
-    if( td->td_nstrips )
-        return td->td_nstrips;
-
 	nstrips = (td->td_rowsperstrip == (uint32) -1 ? 1 :
 	     TIFFhowmany_32(td->td_imagelength, td->td_rowsperstrip));
 	if (td->td_planarconfig == PLANARCONFIG_SEPARATE)
 		nstrips = _TIFFMultiply32(tif, nstrips, (uint32)td->td_samplesperpixel,
 		    "TIFFNumberOfStrips");

Vulnerable code: 	uint64 nstrips64;
	uint32 nstrips32;
	 * never increase the number of strips in an image
	nstrips64 = TIFFhowmany_64(bytecount, stripbytes);
	if ((nstrips64==0)||(nstrips64>0xFFFFFFFF)) /* something is wonky, do nothing. */
	    return;
	nstrips32 = (uint32)nstrips64;
	newcounts = (uint64*) _TIFFCheckMalloc(tif, nstrips32, sizeof (uint64),
	newoffsets = (uint64*) _TIFFCheckMalloc(tif, nstrips32, sizeof (uint64),
	for (strip = 0; strip < nstrips32; strip++) {
		newoffsets[strip] = offset;
	td->td_stripsperimage = td->td_nstrips = nstrips32;
    /* If the value was already computed and store in td_nstrips, then return it,
       since ChopUpSingleUncompressedStrip might have altered and resized the
       since the td_stripbytecount and td_stripoffset arrays to the new value
       after the initial affectation of td_nstrips = TIFFNumberOfStrips() in
       tif_dirread.c ~line 3612.
       See http://bugzilla.maptools.org/show_bug.cgi?id=2587 */
    if( td->td_nstrips )
        return td->td_nstrips;

Fixed code: 2016-12-03 Even Rouault <even.rouault at spatialys.com>

	* libtiff/tif_dirread.c: modify ChopUpSingleUncompressedStrip() to
	instanciate compute ntrips as TIFFhowmany_32(td->td_imagelength, rowsperstrip),
	instead of a logic based on the total size of data. Which is faulty is
	the total size of data is not sufficient to fill the whole image, and thus
	results in reading outside of the StripByCounts/StripOffsets arrays when
	using TIFFReadScanline().
	Reported by Agostino Sarubbo.
	Fixes http://bugzilla.maptools.org/show_bug.cgi?id=2608.

	* libtiff/tif_strip.c: revert the change in TIFFNumberOfStrips() done
	for http://bugzilla.maptools.org/show_bug.cgi?id=2587 / CVE-2016-9273 since
	the above change is a better fix that makes it unnecessary.

	uint32 nstrips;
	 * never increase the number of rows per strip
        nstrips = TIFFhowmany_32(td->td_imagelength, rowsperstrip);
        if( nstrips == 0 )
            return;
	newcounts = (uint64*) _TIFFCheckMalloc(tif, nstrips, sizeof (uint64),
	newoffsets = (uint64*) _TIFFCheckMalloc(tif, nstrips, sizeof (uint64),
	for (strip = 0; strip < nstrips; strip++) {
		newoffsets[strip] = stripbytes ? offset : 0;
	td->td_stripsperimage = td->td_nstrips = nstrips;