328509 - GIF2.cpp: do_lzw() tries to access memory out of bounds [@ do_lzw]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[jhp (no longer active)]

Offshoot of bug 328258.

Debugging using the gif attached at bug 328258 (https://bugzilla.mozilla.org/attachment.cgi?id=212852), I noticed that do_lzw() is accessing memory out of bounds of the array.  Specifically, in this code:

      while (code >= clear_code)
      {
        if (code == prefix[code])
          return -1;

        *stackp++ = suffix[code];
        code = prefix[code];

        if (stackp == stack + MAX_BITS)
          return -1;
      }

At one point, when count == 190, the line "code = prefix[code]" sets code to 43690.  The next time through the loop, it tries to access prefix[43690], but prefix is of size 4097.

Comment 1

[jhp (no longer active)]

In GIFInit, I changed this line:
  memset(gs, 0, offsetof(gif_struct, prefix));
to:
  memset(gs, 0, sizeof(gif_struct));

With this, I don't see it trying to access invalid memory.  Seems the while loop tries to access an offset of 'prefix' that has never been set up.

Comment 2

[Alfred Kayser]

Attachment: One line patch to fix clearing of struct

The offsetof thing was to clear only part of the struct, but is clearly wrong. 
It is saver anyway to clear the whole struct.

Comment 3

[jhp (no longer active)]

Comment on attachment 213232 [details] [diff] [review]
One line patch to fix clearing of struct

This is fine.  However, as part of LZW, should the algorithm access an offset of prefix that hasn't previously been set?  With this fix, the result will now be zero.  Is that correct according to the LZW spec?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

isn't this a duplicate of bug 243653?

Comment 5

[Alfred Kayser]

Comment on attachment 213232 [details] [diff] [review]
One line patch to fix clearing of struct

Pavlov, can you take quickly a look at this. This is related to a 'Access Denied' bug...

Comment 6

[Alfred Kayser]

Thanks, who can check this for me?

Comment 7

[Alfred Kayser]

Correction: 
Previous comment should be:

Thanks, who can check this in the tree(s) for me?

Also requesting to check it into the 1.8 branch for FF 2.0, because this a crasher (and related to bug 328258).

Comment 8

[Daniel Veditz [:dveditz]]

The memset line was added as part of bug 274391, well after the 1.7/aviary101 branches.

Comment 9

[Daniel Veditz [:dveditz]]

I guess I'll take this to land it.

Comment 10

[Daniel Veditz [:dveditz]]

Fix landed on trunk, 1.8 branch, and 1.8.1 branch

What is the security impact here? Looks like it's only reading random memory, right?

Comment 11

[Dave Liebreich [:davel]]

Please provide testcase and guidance on how to verify this fix.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, this patch fixed the uninitialized memory reads and free memory reads reported by valgrind on the GIF in the testcase on bug 317536.  That bug should be marked duplicate or depends+fixed when this bug is opened.

Crashes in do_lzw are a topcrash for Fx 1.5.0.1; I suspect this fix fixes them.

Comment 13

[Daniel Veditz [:dveditz]]

fixed per comment 10 on March 3

Comment 14

[Daniel Veditz [:dveditz]]

*** Bug 317536 has been marked as a duplicate of this bug. ***