cups: gif reader infinite loop and heap buffer overflow

[michaelrsweet]

Version: 1.4.6
CUPS.org User: thoger

Petr Sklenar created a fuzzed gif image which triggers a heap corruption in the GIF reading code in CUPS.

The file triggers stack[] buffer overflow in gif_read_lzw() in filter/image-gif.c. Relevant code part is (line numbers are for 1.4.6):

661 while (code >= clear_code)
662 {
663 *sp++ = table[1][code];
664 if (code == table[0][code])
665 return (255);
666
667 code = table[0][code];
668 }

This ensures that table[0][code] != code, but it's possible to create loop between multiple table entries, if table[0][code] > code. I believe that's an incorrect case that should be detected in this part of the code:

655 if (code >= max_code)
656 {
657 *sp++ = firstcode;
658 code = oldcode;
659 }

That's the code for the LZW decoding special case. code == max_code seems to be the correct check to be used there, with code > max_code being invalid and should be rejected.

I've actually had a look at couple of other code bases that seem to have GIF reader derived from the same implementation as the one used by CUPS. Some of them reject code > max_code:

http://git.savannah.gnu.org/gitweb/?p=gzip.git;a=blob;f=unlzw.c;h=63f941c6213533bf3140ce642bc16d5bfe3e02a9;hb=HEAD#l297
http://tktoolkit.cvs.sf.net/viewvc/tktoolkit/tk/generic/tkImgGIF.c?revision=1.48&view=markup#l1077

while other check for stack[] overflow in the while() loop:

http://svn.php.net/viewvc/gd/trunk/libgd/src/gd_gif_in.c?revision=282370&view=markup#l522
http://hg.mozilla.org/mozilla-central/file/c4b84b05c46c/modules/libpr0n/decoders/nsGIFDecoder2.cpp#l508
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp#L317

This should not have any bad security implications for CUPS beyond filter crash, as this keeps looping until some unmapped memory is hit, without trying to use corrupted memory. I'm tagging this as security so you can review and decide how you want to handle this. Please let me know if you prefer to handle this as embargoed, as there are few other projects that seem to have this problem.

[michaelrsweet]

CUPS.org User: mike

We won't treat this as a security issue - as you say we'll just overflow and SIGBUS pretty quickly in imageto*.

Patch coming up...

[michaelrsweet]

CUPS.org User: mike

Patch attached which also addresses STR #3868 and STR #3869. Please let me know if you think we need any further changes.

[michaelrsweet]

CUPS.org User: thoger

The fix looks reasonable to me, thank you!

[michaelrsweet]

CUPS.org User: mike

Fixed in Subversion repository.

[michaelrsweet]

CUPS.org User: thoger

One thing I realized later, the crash as described above depends on stack[] being located above the table[][]. If memory allocator places it below, stack[] overflow can result in table[][] modification, which can break the infinite loop, and the program may continue executing with corrupted heap.

[michaelrsweet]

"str3867.patch":

Index: filter/image-gif.c

--- filter/image-gif.c (revision 9839)
+++ filter/image-gif.c (working copy)
@@ -353,7 +353,7 @@
* Read in another buffer...
*/

• if ((count = gif_get_block (fp, buf + last_byte)) <= 0)
• if ((count = gif_get_block(fp, buf + last_byte)) <= 0)
  {
  /*
• Whoops, no more data!
  @@ -582,20 +582,14 @@
  gif_get_code(fp, 0, 1);

/*

• * Wipe the decompressor table...

• • Wipe the decompressor table (already mostly 0 due to the calloc above...)
    */

    fresh = 1;

• for (i = 0; i < clear_code; i ++)
• {

•  table[0][i] = 0;
  

• for (i = 1; i < clear_code; i ++)
  table[1][i] = i;
• }
• for (; i < 4096; i ++)

- table[0][i] = table[1][0] = 0;

 sp = stack;

 return (0);

@@ -605,30 +599,31 @@
fresh = 0;

 do

• {
  firstcode = oldcode = gif_get_code(fp, code_size, 0);
• }
  while (firstcode == clear_code);
• return (firstcode);
• return (firstcode & 255);
  }
  else if (!table)
  return (0);

if (sp > stack)

• return (*--sp);
• return ((*--sp) & 255);
• while ((code = gif_get_code (fp, code_size, 0)) >= 0)
• while ((code = gif_get_code(fp, code_size, 0)) >= 0)
  {
  if (code == clear_code)
  {

•  for (i = 0; i < clear_code; i ++)
  

•  {
  

• table[0][i] = 0;

• /*
  

•  \* Clear/reset the compression table...
  

•  */
  

•  memset(table, 0, 2 \* sizeof(gif_table_t));
  

•  for (i = 1; i < clear_code; i ++)
  

  table[1][i] = i;

•  }
  

•  for (; i < 4096; i ++)
  

- table[0][i] = table[1][i] = 0;

   code_size     = set_code_size + 1;
   max_code_size = 2 * clear_code;
   max_code      = clear_code + 2;

@@ -637,13 +632,12 @@

   firstcode = oldcode = gif_get_code(fp, code_size, 0);

•  return (firstcode);
  

•  return (firstcode & 255);
  

  }
• else if (code == end_code)
• else if (code == end_code || code > max_code)
  {

•  unsigned char    buf[260];
  

•  unsigned char    buf[260];   /\* Block buffer */
  

   if (!gif_eof)
     while (gif_get_block(fp, buf) > 0);

@@ -652,7 +646,7 @@

 incode = code;

• if (code >= max_code)
• if (code == max_code)
  {
  *sp++ = firstcode;
  code = oldcode;
  @@ -686,10 +680,10 @@
  oldcode = incode;

if (sp > stack)

•  return (*--sp);
  

•  return ((*--sp) & 255);
  

  }

• return (code);

• return (code & 255);
  }