Add support for reading TIFFs with PlanarConfiguration=2

[wiredfool]

Superceeds #5178

[wiredfool]

Test failures appear to be unrelated -- startup on pypy3.7, and two qt sanity failures on minggw.

[wiredfool]

Spurious failures went away after a rebase to current master.

[kkopachev]

I believe this is redundant call because TIFFReadTile() below internally calls this. Comment above does not apply too.

[wiredfool]

Maybe, but this check did fix an OOB read CVE.

[kkopachev]

Right, I remember that. However it was fixed when reading code was using both TIFFReadTile and TiffReadRGBATile depending on flag. This PR changed that so RGBA reading is done in another code path, TiffReadRGBATile and is gone completely.
I'll create PR to remove this check and see how tests behave.

[wiredfool]

I've actually got a new fuzz crash with this patch -- I'm not able to replicate it anywhere but in the oss-fuzz image, so it appears to be tiff version specific. I need to get a vm setup with this set of libraries to debug that one.

I'm thinking that this needs to be done prior to release, because it's less than an hour of fuzz time, so it's going to get hit right quick. @hugovk @radarhere

tif.zip

(note, I'm not embargoing this one because it's never been in a release version)

[kkopachev]

@wiredfool could you try this patch for the fuzz image:

Index: src/libImaging/TiffDecode.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/libImaging/TiffDecode.c b/src/libImaging/TiffDecode.c
--- a/src/libImaging/TiffDecode.c	(revision 6eae8fd59213a6a403dd69f673ce596e5b9d7fa1)
+++ b/src/libImaging/TiffDecode.c	(date 1617153563099)
@@ -265,7 +265,7 @@
         ret = TIFFGetFieldDefaulted(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_block);
     }
 
-    if (ret != 1) {
+    if (ret != 1 || rows_per_block==(UINT32)(-1)) {
         rows_per_block = state->ysize;
     }
 
@@ -281,17 +281,6 @@
     img.req_orientation = ORIENTATION_TOPLEFT;
     img.col_offset = 0;
 
-    if (state->xsize != img.width || state->ysize != img.height) {
-        TRACE(
-            ("Inconsistent Image Error: %d =? %d, %d =? %d",
-             state->xsize,
-             img.width,
-             state->ysize,
-             img.height));
-        state->errcode = IMAGING_CODEC_BROKEN;
-        goto decodergba_err;
-    }
-
     /* overflow check for row byte size */
     if (INT_MAX / 4 < img.width) {
         state->errcode = IMAGING_CODEC_MEMORY;
@@ -429,7 +418,7 @@
             for (x = state->xoff; x < state->xsize; x += tile_width) {
                 /* Sanity Check. Apparently in some cases, the TiffReadRGBA* functions
                    have a different view of the size of the tiff than we're getting from
-                   other functions. So, we need to check here. 
+                   other functions. So, we need to check here.
                 */
                 if (!TIFFCheckTile(tiff, x, y, 0, plane)) {
                     TRACE(("Check Tile Error, Tile at %dx%d\n", x, y));
@@ -568,6 +557,7 @@
     uint16 planarconfig = 0;
     int planes = 1;
     ImagingShuffler unpackers[4];
+    UINT32 img_width, img_height;
 
     memset(unpackers, 0, sizeof(ImagingShuffler) * 4);
 
@@ -663,6 +653,20 @@
             goto decode_err;
         }
     }
+
+    TIFFGetField(tiff, TIFFTAG_IMAGEWIDTH, &img_width);
+    TIFFGetField(tiff, TIFFTAG_IMAGELENGTH, &img_height);
+
+	if (state->xsize != img_width || state->ysize != img_height) {
+        TRACE(
+            ("Inconsistent Image Error: %d =? %d, %d =? %d",
+             state->xsize,
+             img_width,
+             state->ysize,
+             img_height));
+        state->errcode = IMAGING_CODEC_BROKEN;
+        goto decode_err;
+    }
 
 
     TIFFGetField(tiff, TIFFTAG_PHOTOMETRIC, &photometric);

[wiredfool]

Ok, that one is fixed, there's a new one that I can reproduce. I can't really get to this before this evening, but I'll do it then if no one else finds it.

tiff-8115.zip

On 8.1.2, this is undefined behavior only, on current master this is an OOB read.

[kkopachev]

@wiredfool I think this one is happening because SAMPLESPERPIXEL value in the test file does not match with SampleFormat.
Pillow does not check that they match.
I think simplest fix would be to add such check is in python code:

Index: src/PIL/TiffImagePlugin.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/PIL/TiffImagePlugin.py b/src/PIL/TiffImagePlugin.py
--- a/src/PIL/TiffImagePlugin.py	(revision 6eae8fd59213a6a403dd69f673ce596e5b9d7fa1)
+++ b/src/PIL/TiffImagePlugin.py	(date 1617211655442)
@@ -1250,6 +1250,10 @@
         if bps_count > len(bps_tuple) and len(bps_tuple) == 1:
             bps_tuple = bps_tuple * bps_count
 
+        samplesPerPixel = self.tag_v2.get(SAMPLESPERPIXEL, 1)
+        if len(bps_tuple) != samplesPerPixel:
+            raise SyntaxError("unknown data organization")
+
         # mode: check photometric interpretation and bits per pixel
         key = (
             self.tag_v2.prefix,

[kkopachev]

On that note, might be good to replicate libtiffs checks it does in TIFFRGBAImageOK in python code

[wiredfool]

That looks like it fixes the fuzzer. Thanks for your help.