Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reading TIFFs with PlanarConfiguration=2 #5364

Merged
merged 17 commits into from Mar 28, 2021

Conversation

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Mar 28, 2021

Superceeds #5178

@wiredfool wiredfool changed the title YA rebase of #4641 Add support for reading TIFFs with PlanarConfiguration=2 Mar 28, 2021
@wiredfool
Copy link
Member Author

@wiredfool wiredfool commented Mar 28, 2021

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

@wiredfool wiredfool merged commit d061203 into python-pillow:master Mar 28, 2021
50 of 53 checks passed
@wiredfool
Copy link
Member Author

@wiredfool wiredfool commented Mar 28, 2021

Spurious failures went away after a rebase to current master.

have a different view of the size of the tiff than we're getting from
other functions. So, we need to check here.
*/
if (!TIFFCheckTile(tiff, x, y, 0, plane)) {
Copy link
Contributor

@kkopachev kkopachev Mar 29, 2021

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

Copy link
Member Author

@wiredfool wiredfool Mar 29, 2021

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

Copy link
Contributor

@kkopachev kkopachev Mar 29, 2021

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
Copy link
Member Author

@wiredfool wiredfool commented Mar 30, 2021

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
Copy link
Contributor

@kkopachev kkopachev commented Mar 31, 2021

@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);

@hugovk hugovk mentioned this pull request Mar 31, 2021
25 tasks
@wiredfool
Copy link
Member Author

@wiredfool wiredfool commented Mar 31, 2021

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
Copy link
Contributor

@kkopachev kkopachev commented Mar 31, 2021

@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
Copy link
Contributor

@kkopachev kkopachev commented Mar 31, 2021

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

@wiredfool
Copy link
Member Author

@wiredfool wiredfool commented Mar 31, 2021

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

wiredfool added a commit that referenced this issue Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants