Closed Bug 328509 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: ImageLib, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jhpedemonte, Assigned: dveditz)

References

()

Details

(4 keywords, Whiteboard: [sg:high?] Doesn't affect ff1.0/moz1.7 [tcn-dl])

Crash Data

Attachments

(1 file)

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.
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.
The offsetof thing was to clear only part of the struct, but is clearly wrong. 
It is saver anyway to clear the whole struct.
Attachment #213232 - Flags: review?(jhpedemonte)
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?
Attachment #213232 - Flags: review?(jhpedemonte) → review+
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...
Attachment #213232 - Flags: superreview?(pavlov)
Attachment #213232 - Flags: superreview?(pavlov) → superreview+
Thanks, who can check this for me?
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).
Flags: blocking1.8.0.2?
Group: security
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Whiteboard: [sg:??]
The memset line was added as part of bug 274391, well after the 1.7/aviary101 branches.
Keywords: regression
Whiteboard: [sg:??] → [sg:??] Doesn't affect ff1.0/moz1.7
I guess I'll take this to land it.
Assignee: pavlov → 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?
Please provide testcase and guidance on how to verify this fix.
Whiteboard: [sg:??] Doesn't affect ff1.0/moz1.7 → [sg:??] Doesn't affect ff1.0/moz1.7 [tcn-dl]
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.
Keywords: topcrash
Summary: GIF2.cpp: do_lzw() tries to access memory out of bounds → GIF2.cpp: do_lzw() tries to access memory out of bounds [@ do_lzw]
fixed per comment 10 on March 3
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [sg:??] Doesn't affect ff1.0/moz1.7 [tcn-dl] → [sg:high?] Doesn't affect ff1.0/moz1.7 [tcn-dl]
*** Bug 317536 has been marked as a duplicate of this bug. ***
Group: security
No longer blocks: 328258
Crash Signature: [@ do_lzw]
You need to log in before you can comment on or make changes to this bug.