2017.06.27 20:51 "[Tiff] Excessive memory allocation while chopping strips", by Nicolas RUFF
I saw in https://trac.osgeo.org/gdal/changeset/39082 that you try to defend against overly large memory allocations.
I have a pathological test case from fuzzing where header values are:
ImageWidth = 1060
ImageLength = 536871550
It will hit the following piece of code in tif_dirread.c:
- Some manufacturers make life difficult by writing
- large amounts of uncompressed data as a single strip.
- This is contrary to the recommendations of the spec.
- The following makes an attempt at breaking such images
- into strips closer to the recommended 8k bytes. A
- side effect, however, is that the RowsPerStrip tag
- value may be changed.
if ( !_TIFFFillStriles(tif) || !tif->tif_dir.td_stripbytecount )
ChopUpSingleUncompressedStrip() will compute the required number of strips and reach this point:
newcounts = (uint64*) _TIFFCheckMalloc(tif, nstrips, sizeof (uint64),
"for chopped \"StripByteCounts\" array");
In the my case, nstrips = 268435775 (and rowsperstrip = 2), which will try to allocate 268435775 * 8 bytes. This is already beyond 2**31, but we can make it even larger and beyond 2**32 by playing with ImageLength.
Rather than playing this cat-and-mouse game of assigning arbitrary limits to every possible tag, I would suggest the following patch in tif_aux.c:
_TIFFCheckRealloc(TIFF* tif, void* buffer,
tmsize_t nmemb, tmsize_t elem_size, const char* what)
void* cp = NULL;
tmsize_t bytes = nmemb * elem_size;
- * XXX: Check for integer overflow.
- if (nmemb && elem_size && bytes / elem_size == nmemb)
+ /* TIFF size is limited to 32-bit by design and 31-bit by implementation, so
+ * there should be no reason to ever allocate more than INT_MAX bytes.
+ */+ if ((nmemb <= INT_MAX) && (elem_size <= INT_MAX) && (bytes <=
cp = _TIFFrealloc(buffer, bytes);
Note: this patch assumes that tmsize_t is 64 bits, otherwise the check should be done in a different way.
What do you think?
- Nicolas RUFF