BE-139-cve-2021-28676

Add documentation changes

Cherry picked bb6c11fb88

Update tests to work in old code

Add function missing in old code

CHANGES.rst

@@ -27,6 +27,10 @@ Changelog (Pillow)
   combination of \r and \n as line endings.
   [rickprice]
 
+- Fix CVE-2021-28676: FliDecode did not properly check that the block advance
+  was non-zero, potentally leading to an infinite loop on load.
+  [rickprice]
+
 6.2.2.4 (2023-03-29)
 ------------------
 

Tests/test_file_fli.py

@@ -46,7 +46,8 @@ class TestFileFli(PillowTestCase):
     def test_invalid_file(self):
         invalid_file = "Tests/images/flower.jpg"
 
-        self.assertRaises(SyntaxError, FliImagePlugin.FliImageFile, invalid_file)
+        self.assertRaises(
+            SyntaxError, FliImagePlugin.FliImageFile, invalid_file)
 
     def test_n_frames(self):
         im = Image.open(static_test_file)
@@ -96,3 +97,13 @@ class TestFileFli(PillowTestCase):
 
         expected = Image.open("Tests/images/a_fli.png")
         self.assert_image_equal(im, expected)
+
+    def test_timeouts(self):
+        for test_file in [
+            "Tests/images/timeout-9139147ce93e20eb14088fe238e541443ffd64b3.fli",
+            "Tests/images/timeout-bff0a9dc7243a8e6ede2408d2ffa6a9964698b87.fli",
+        ]:
+            with open(test_file, "rb") as f:
+                with Image.open(f) as im:
+                    with self.assertRaises(OSError):
+                        im.load()

docs/releasenotes/6.2.2.5.rst

@@ -25,6 +25,9 @@ This release addresses several critical CVEs.
 
 :cve:`CVE-2020-10994`: In libImaging/Jpeg2KDecode.c in Pillow before 7.1.0, there are multiple out-of-bounds reads via a crafted JP2 file.
 
+:cve:`CVE-2021-28676``: FliDecode did not properly check that the block advance was non-zero,
+ potentally leading to an infinite loop on load.
+
 :cve:`CVE-2021-28677`: An issue was discovered in Pillow before 8.2.0. For EPS
                        data, the readline implementation used in EPSImageFile
                        has to deal with any combination of \r and \n as line

src/libImaging/FliDecode.c

@@ -14,203 +14,246 @@
  * See the README file for information on usage and redistribution.
  */
 
-
 #include "Imaging.h"
 
+#define I16(ptr) ((ptr)[0] + ((ptr)[1] << 8))
 
-#define	I16(ptr)\
+#define I32(ptr)                                                               \
-    ((ptr)[0] + ((ptr)[1] << 8))
+  ((ptr)[0] + ((ptr)[1] << 8) + ((ptr)[2] << 16) + ((ptr)[3] << 24))
 
-#define	I32(ptr)\
+#define ERR_IF_DATA_OOB(offset)                                                \
-    ((ptr)[0] + ((ptr)[1] << 8) + ((ptr)[2] << 16) + ((ptr)[3] << 24))
+  if ((data + (offset)) > ptr + bytes) {                                       \
+    state->errcode = IMAGING_CODEC_OVERRUN;                                    \
+    return -1;                                                                 \
+  }
 
+int ImagingFliDecode(Imaging im, ImagingCodecState state, UINT8 *buf,
+                     Py_ssize_t bytes) {
+  UINT8 *ptr;
+  int framesize;
+  int c, chunks, advance;
+  int l, lines;
+  int i, j, x = 0, y, ymax;
 
-int
+  /* If not even the chunk size is present, we'd better leave */
-ImagingFliDecode(Imaging im, ImagingCodecState state, UINT8* buf, Py_ssize_t bytes)
-{
-    UINT8* ptr;
-    int framesize;
-    int c, chunks, advance;
-    int l, lines;
-    int i, j, x = 0, y, ymax;
 
-    /* If not even the chunk size is present, we'd better leave */
+  if (bytes < 4)
+    return 0;
 
-    if (bytes < 4)
+  /* We don't decode anything unless we have a full chunk in the
-	return 0;
+     input buffer */
 
-    /* We don't decode anything unless we have a full chunk in the
+  ptr = buf;
-       input buffer */
 
-    ptr = buf;
+  framesize = I32(ptr);
+  if (framesize < I32(ptr))
+    return 0;
 
-    framesize = I32(ptr);
+  /* Make sure this is a frame chunk.  The Python driver takes
-    if (framesize < I32(ptr))
+     case of other chunk types. */
-	return 0;
 
-    /* Make sure this is a frame chunk.  The Python driver takes
+  if (bytes < 8) {
-       case of other chunk types. */
+    state->errcode = IMAGING_CODEC_OVERRUN;
+    return -1;
+  }
+  if (I16(ptr + 4) != 0xF1FA) {
+    state->errcode = IMAGING_CODEC_UNKNOWN;
+    return -1;
+  }
 
-    if (bytes < 8) {
+  chunks = I16(ptr + 6);
+  ptr += 16;
+  bytes -= 16;
+
+  /* Process subchunks */
+  for (c = 0; c < chunks; c++) {
+    UINT8 *data;
+    if (bytes < 10) {
+      state->errcode = IMAGING_CODEC_OVERRUN;
+      return -1;
+    }
+    data = ptr + 6;
+    switch (I16(ptr + 4)) {
+    case 4:
+    case 11:
+      /* FLI COLOR chunk */
+      break; /* ignored; handled by Python code */
+    case 7:
+      /* FLI SS2 chunk (word delta) */
+      /* OOB ok, we've got 4 bytes min on entry */
+      lines = I16(data);
+      data += 2;
+      for (l = y = 0; l < lines && y < state->ysize; l++, y++) {
+        UINT8 *local_buf = (UINT8 *)im->image[y];
+        int p, packets;
+        ERR_IF_DATA_OOB(2)
+        packets = I16(data);
+        data += 2;
+        while (packets & 0x8000) {
+          /* flag word */
+          if (packets & 0x4000) {
+            y += 65536 - packets; /* skip lines */
+            if (y >= state->ysize) {
+              state->errcode = IMAGING_CODEC_OVERRUN;
+              return -1;
+            }
+            local_buf = (UINT8 *)im->image[y];
+          } else {
+            /* store last byte (used if line width is odd) */
+            local_buf[state->xsize - 1] = (UINT8)packets;
+          }
+          ERR_IF_DATA_OOB(2)
+          packets = I16(data);
+          data += 2;
+        }
+        for (p = x = 0; p < packets; p++) {
+          ERR_IF_DATA_OOB(2)
+          x += data[0]; /* pixel skip */
+          if (data[1] >= 128) {
+            ERR_IF_DATA_OOB(4)
+            i = 256 - data[1]; /* run */
+            if (x + i + i > state->xsize) {
+              break;
+            }
+            for (j = 0; j < i; j++) {
+              local_buf[x++] = data[2];
+              local_buf[x++] = data[3];
+            }
+            data += 2 + 2;
+          } else {
+            i = 2 * (int)data[1]; /* chunk */
+            if (x + i > state->xsize) {
+              break;
+            }
+            ERR_IF_DATA_OOB(2 + i)
+            memcpy(local_buf + x, data + 2, i);
+            data += 2 + i;
+            x += i;
+          }
+        }
+        if (p < packets) {
+          break; /* didn't process all packets */
+        }
+      }
+      if (l < lines) {
+        /* didn't process all lines */
         state->errcode = IMAGING_CODEC_OVERRUN;
         return -1;
+      }
+      break;
+    case 12:
+      /* FLI LC chunk (byte delta) */
+      /* OOB Check ok, we have 4 bytes min here */
+      y = I16(data);
+      ymax = y + I16(data + 2);
+      data += 4;
+      for (; y < ymax && y < state->ysize; y++) {
+        UINT8 *out = (UINT8 *)im->image[y];
+        ERR_IF_DATA_OOB(1)
+        int p, packets = *data++;
+        for (p = x = 0; p < packets; p++, x += i) {
+          ERR_IF_DATA_OOB(2)
+          x += data[0]; /* skip pixels */
+          if (data[1] & 0x80) {
+            i = 256 - data[1]; /* run */
+            if (x + i > state->xsize) {
+              break;
+            }
+            ERR_IF_DATA_OOB(3)
+            memset(out + x, data[2], i);
+            data += 3;
+          } else {
+            i = data[1]; /* chunk */
+            if (x + i > state->xsize) {
+              break;
+            }
+            ERR_IF_DATA_OOB(2 + i)
+            memcpy(out + x, data + 2, i);
+            data += i + 2;
+          }
+        }
+        if (p < packets) {
+          break; /* didn't process all packets */
+        }
+      }
+      if (y < ymax) {
+        /* didn't process all lines */
+        state->errcode = IMAGING_CODEC_OVERRUN;
+        return -1;
+      }
+      break;
+    case 13:
+      /* FLI BLACK chunk */
+      for (y = 0; y < state->ysize; y++) {
+        memset(im->image[y], 0, state->xsize);
+      }
+      break;
+    case 15:
+      /* FLI BRUN chunk */
+      /* OOB, ok, we've got 4 bytes min on entry */
+      for (y = 0; y < state->ysize; y++) {
+        UINT8 *out = (UINT8 *)im->image[y];
+        data += 1; /* ignore packetcount byte */
+        for (x = 0; x < state->xsize; x += i) {
+          ERR_IF_DATA_OOB(2)
+          if (data[0] & 0x80) {
+            i = 256 - data[0];
+            if (x + i > state->xsize) {
+              break; /* safety first */
+            }
+            ERR_IF_DATA_OOB(i + 1)
+            memcpy(out + x, data + 1, i);
+            data += i + 1;
+          } else {
+            i = data[0];
+            if (x + i > state->xsize) {
+              break; /* safety first */
+            }
+            memset(out + x, data[1], i);
+            data += 2;
+          }
+        }
+        if (x != state->xsize) {
+          /* didn't unpack whole line */
+          state->errcode = IMAGING_CODEC_OVERRUN;
+          return -1;
+        }
+      }
+      break;
+    case 16:
+      /* COPY chunk */
+      if (state->xsize > bytes / state->ysize) {
+        /* not enough data for frame */
+        return ptr - buf; /* bytes consumed */
+      }
+      for (y = 0; y < state->ysize; y++) {
+        UINT8 *local_buf = (UINT8 *)im->image[y];
+        memcpy(local_buf, data, state->xsize);
+        data += state->xsize;
+      }
+      break;
+    case 18:
+      /* PSTAMP chunk */
+      break; /* ignored */
+    default:
+      /* unknown chunk */
+      /* printf("unknown FLI/FLC chunk: %d\n", I16(ptr+4)); */
+      state->errcode = IMAGING_CODEC_UNKNOWN;
+      return -1;
     }
-    if (I16(ptr+4) != 0xF1FA) {
+    advance = I32(ptr);
-	state->errcode = IMAGING_CODEC_UNKNOWN;
+    if (advance == 0) {
-	return -1;
+      // If there's no advance, we're in in infinite loop
+      state->errcode = IMAGING_CODEC_BROKEN;
+      return -1;
     }
-
+    if (advance < 0 || advance > bytes) {
-    chunks = I16(ptr+6);
+      state->errcode = IMAGING_CODEC_OVERRUN;
-    ptr += 16;
+      return -1;
-    bytes -= 16;
-
-    /* Process subchunks */
-    for (c = 0; c < chunks; c++) {
-	UINT8* data;
-	if (bytes < 10) {
-	    state->errcode = IMAGING_CODEC_OVERRUN;
-	    return -1;
-	}
-	data = ptr + 6;
-	switch (I16(ptr+4)) {
-	case 4: case 11:
-	    /* FLI COLOR chunk */
-	    break; /* ignored; handled by Python code */
-	case 7:
-	    /* FLI SS2 chunk (word delta) */
-	    lines = I16(data); data += 2;
-	    for (l = y = 0; l < lines && y < state->ysize; l++, y++) {
-		UINT8* buf = (UINT8*) im->image[y];
-		int p, packets;
-		packets = I16(data); data += 2;
-		while (packets & 0x8000) {
-		    /* flag word */
-		    if (packets & 0x4000) {
-			y += 65536 - packets; /* skip lines */
-			if (y >= state->ysize) {
-			    state->errcode = IMAGING_CODEC_OVERRUN;
-			    return -1;
-			}
-			buf = (UINT8*) im->image[y];
-		    } else {
-			/* store last byte (used if line width is odd) */
-			buf[state->xsize-1] = (UINT8) packets;
-		    }
-		    packets = I16(data); data += 2;
-		}
-		for (p = x = 0; p < packets; p++) {
-		    x += data[0]; /* pixel skip */
-		    if (data[1] >= 128) {
-			i = 256-data[1]; /* run */
-			if (x + i + i > state->xsize)
-			    break;
-			for (j = 0; j < i; j++) {
-			    buf[x++] = data[2];
-			    buf[x++] = data[3];
-			}
-			data += 2 + 2;
-		    } else {
-			i = 2 * (int) data[1]; /* chunk */
-			if (x + i > state->xsize)
-			    break;
-			memcpy(buf + x, data + 2, i);
-			data += 2 + i;
-			x += i;
-		    }
-		}
-		if (p < packets)
-		    break; /* didn't process all packets */
-	    }
-	    if (l < lines) {
-		/* didn't process all lines */
-		state->errcode = IMAGING_CODEC_OVERRUN;
-		return -1;
-	    }
-	    break;
-	case 12:
-	    /* FLI LC chunk (byte delta) */
-	    y = I16(data); ymax = y + I16(data+2); data += 4;
-	    for (; y < ymax && y < state->ysize; y++) {
-		UINT8* out = (UINT8*) im->image[y];
-		int p, packets = *data++;
-		for (p = x = 0; p < packets; p++, x += i) {
-		    x += data[0]; /* skip pixels */
-		    if (data[1] & 0x80) {
-			i = 256-data[1]; /* run */
-			if (x + i > state->xsize)
-			    break;
-			memset(out + x, data[2], i);
-			data += 3;
-		    } else {
-			i = data[1]; /* chunk */
-			if (x + i > state->xsize)
-			    break;
-			memcpy(out + x, data + 2, i);
-			data += i + 2;
-		    }
-		}
-		if (p < packets)
-		    break; /* didn't process all packets */
-	    }
-	    if (y < ymax) {
-		/* didn't process all lines */
-		state->errcode = IMAGING_CODEC_OVERRUN;
-		return -1;
-	    }
-	    break;
-	case 13:
-	    /* FLI BLACK chunk */
-	    for (y = 0; y < state->ysize; y++)
-		memset(im->image[y], 0, state->xsize);
-	    break;
-	case 15:
-	    /* FLI BRUN chunk */
-	    for (y = 0; y < state->ysize; y++) {
-		UINT8* out = (UINT8*) im->image[y];
-		data += 1; /* ignore packetcount byte */
-		for (x = 0; x < state->xsize; x += i) {
-		    if (data[0] & 0x80) {
-			i = 256 - data[0];
-			if (x + i > state->xsize)
-			    break; /* safety first */
-			memcpy(out + x, data + 1, i);
-			data += i + 1;
-		    } else {
-			i = data[0];
-			if (x + i > state->xsize)
-			    break; /* safety first */
-			memset(out + x, data[1], i);
-			data += 2;
-		    }
-		}
-		if (x != state->xsize) {
-		    /* didn't unpack whole line */
-		    state->errcode = IMAGING_CODEC_OVERRUN;
-		    return -1;
-		}
-	    }
-	    break;
-	case 16:
-	    /* COPY chunk */
-	    for (y = 0; y < state->ysize; y++) {
-		UINT8* buf = (UINT8*) im->image[y];
-		memcpy(buf, data, state->xsize);
-		data += state->xsize;
-	    }
-	    break;
-	case 18:
-	    /* PSTAMP chunk */
-	    break; /* ignored */
-	default:
-	    /* unknown chunk */
-	    /* printf("unknown FLI/FLC chunk: %d\n", I16(ptr+4)); */
-	    state->errcode = IMAGING_CODEC_UNKNOWN;
-	    return -1;
-	}
-	advance = I32(ptr);
-	ptr += advance;
-	bytes -= advance;
     }
+    ptr += advance;
+    bytes -= advance;
+  }
 
-    return -1; /* end of frame */
+  return -1; /* end of frame */
 }