heap-use-after-free (write) at DoLzw

RESOLVED FIXED in Firefox 43

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: aki.helin, Assigned: seth)

Tracking

({csectype-uaf, sec-critical})

Trunk
mozilla44
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox42+ unaffected, firefox43+ fixed, firefox44+ fixed, firefox-esr38 affected)

Details

(Whiteboard: [b2g-adv-main2.5-])

Attachments

(4 attachments)

Posted file dolzw.html
ASan builds spot this when the attached image is included in a page. Sometimes a few refresh are needed. Tested with a few recent tinderbox builds on Linux.

WRITE of size 1 at 0x6110002c5fc8 thread T20 (ImgDecoder #3)
    #0 0x7f0e1f254a42 in DoLzw /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/decoders/nsGIFDecoder2.cpp:550
    #1 0x7f0e1f256332 in WriteInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/decoders/nsGIFDecoder2.cpp:732
    #2 0x7f0e1f1f9a62 in Write /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/Decoder.cpp:183
    #3 0x7f0e1f1f7d1c in Decode /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/Decoder.cpp:128
    #4 0x7f0e1f1f7742 in Decode /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/DecodePool.cpp:453
    #5 0x7f0e1f2163cc in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/DecodePool.cpp:282
    #6 0x7f0e1d1bbd6f in ProcessNextEvent /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:960
    #7 0x7f0e1d2351ea in NS_ProcessNextEvent /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:277
    #8 0x7f0e1dad4f2f in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #9 0x7f0e1da426ec in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #10 0x7f0e1da426ec in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #11 0x7f0e1da426ec in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #12 0x7f0e1d1b7ae0 in ThreadFunc /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:382
    #13 0x7f0e2a6da4b5 in _pt_root /builds/slave/m-cen-l64-asan-000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #14 0x7f0e2ad19181 in start_thread /build/buildd/eglibc-2.19/nptl/pthread_create.c:312 (discriminator 2)
    #15 0x7f0e1ac3547c in clone /build/buildd/eglibc-2.19/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:111
Posted image dolzw.gif
Seth, could you look at this?
Component: Graphics → ImageLib
Flags: needinfo?(seth)
This actually crashes immediately on my machine. I'll take a look.
Flags: needinfo?(seth)
The GIF contains a frame that is positioned *far* outside of the GIF's drawable area:

(lldb) p frameRect
(mozilla::gfx::IntRect) $0 = {
  mozilla::gfx::BaseRect<int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::IntMarginTyped<mozilla::gfx::UnknownUnits> > = (x = 210, y = 37632, width = 60, height = 60)
This is a preliminary patch that gives us two small Downscaler features we'll use in part 2. They are:

- Support for detecting when we've written to every row in the frame.

- Support for an empty frame rects. (When the frame rect is empty, we just skip
  every row in the frame, because we don't need or want to write anything in
  this case.)
Attachment #8673364 - Flags: review?(tnikkel)
This part actually fixes the bug. Two steps are necessary:

- We clamp the frame rect to the image rect (i.e., the rect that's actually
  visible) before passing it to Downscaler::BeginFrame(). That makes us pass the
  assertions that this test case fires in debug mode, and it ensures that we
  don't try to skip past the end of the frame.

- In DoLzw() (where we actually output data for GIFs), we check if we've already
  written to the entire frame, and bail early if so.

These two things interact in the case of this testcase. There is no intersection
between the frame rect and the image rect for this image, so we pass an empty
frame rect to BeginFrame(). BeginFrame() then ends up using SkipRow() to skip
the entire frame, since the frame rect is empty. That means that we never
actually write any output in DoLzw(), because Downscaler::IsFrameComplete()
returns true immediately, and hence we never get to the bad write that triggers
this bug.

A test case for this will be part of the DDD testcase suite.
Attachment #8673366 - Flags: review?(tnikkel)
See Also: → 1207830
Blocks: 1194058
Attachment #8673364 - Flags: review?(tnikkel) → review+
Attachment #8673366 - Flags: review?(tnikkel) → review+
Group: core-security → gfx-core-security
Seth, could you request for the sec-approval and the uplifts request? FYI, beta 8 gtb is today.
Flags: needinfo?(seth)
Assignee: nobody → seth
Flags: sec-bounty?
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Seth, could you request for the sec-approval and the uplifts request? FYI,
> beta 8 gtb is today.

I'm sorry for the poor responsiveness here; I was unavailable due to a sudden illness.

This has not even landed on trunk yet. I'm guessing the prospects for it ending up in 42 are poor.

I'll go ahead and land it now and once it gets merged I'll fill out the appropriate requests.
Flags: needinfo?(seth)
This will need sec-approval to land on trunk. I expect that we'll want to wait until two weeks into the next cycle unless it is especially non-obvious.
(In reply to Al Billings [:abillings] from comment #9)
> This will need sec-approval to land on trunk. I expect that we'll want to
> wait until two weeks into the next cycle unless it is especially non-obvious.

I don't think it's super obvious; my inclination would be to just get the fix out there. But I'll fill out the form and let the pros make the decision. =)
Comment on attachment 8673364 [details] [diff] [review]
(Part 1) - Support zero-size frame rects and detecting the end of the frame in Downscaler.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It wouldn't be tremendously hard to hex edit a GIF to trigger the bug. The patches do not explicitly state the requirements the GIF would have to meet, but they do give enough information that someone could figure it out.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't think so. This is in some sense a matter of input validation, though, and if someone is paying attention and notices that these are patches for a security bug, and they had some understanding of the GIF file format, they would have at least an idea of what type of modifications they'd need to make to a GIF header to affect the things that the patch changes.

Which older supported branches are affected by this flaw?

At least back through 43. 42 is marked as affected in the bug but I'm pretty sure that's incorrect, as we didn't land the affected code until 43.

If not all supported branches, which bug introduced the flaw?

Bug 1060609.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The same patch should apply to 43 and 44. The code has not diverged.

How likely is this patch to cause regressions; how much testing does it need?

It's not a trivial patch, so there is the possibility of regressions. I would like to get this on trunk ASAP so it can start getting tested.
Attachment #8673364 - Flags: sec-approval?
(In reply to Seth Fowler [:seth] [:s2h] from comment #11)
> Bug 1060609.

I meant bug 1194058; my mistake.
Attachment #8673366 - Flags: sec-approval?
Comment on attachment 8673364 [details] [diff] [review]
(Part 1) - Support zero-size frame rects and detecting the end of the frame in Downscaler.

sec-approval+. Let's get this on Aurora too.
Attachment #8673364 - Flags: sec-approval? → sec-approval+
Is ESR38 affected at all?
(In reply to Al Billings [:abillings] from comment #14)
> Is ESR38 affected at all?

Definitely not.
Target Milestone: --- → mozilla44
Flags: sec-bounty? → sec-bounty+
Attachment #8673366 - Flags: sec-approval? → sec-approval+
Group: gfx-core-security → core-security-release
Group: core-security-release
Whiteboard: [b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.