Closed Bug 1262338 Opened 8 years ago Closed 8 years ago

heap-buffer-overflow write in [@mozilla::image::nsGIFDecoder2::DoLzw]

Categories

(Core :: Graphics: ImageLib, defect)

48 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- unaffected

People

(Reporter: tsmith, Assigned: seth)

References

(Blocks 1 open bug)

Details

(5 keywords)

Attachments

(6 files)

Attached file call_stack.txt —
WRITE of size 64 at 0x6040000718cf thread T39 (ImgDecoder #4)
==983==AddressSanitizer: while reporting a bug found another one.Ignoring.
    #0 0x7f9e9efa316e in operator() /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/SurfaceFilters.h:473
    #1 0x7f9e9efa316e in WriteRows<unsigned int, <lambda at /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/SurfaceFilters.h:472:40> > /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/SurfacePipe.h:245
    #2 0x7f9e9efa316e in mozilla::image::RemoveFrameRectFilter<mozilla::image::DownscalingFilter<mozilla::image::SurfaceSink> >::AdvanceRow() /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/SurfaceFilters.h:472
    #3 0x7f9e9ef740e0 in WritePixels<unsigned int, <lambda at /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/decoders/nsGIFDecoder2.cpp:423:47> > /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/SurfacePipe.h:195
    #4 0x7f9e9ef740e0 in WritePixels<unsigned int, <lambda at /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/decoders/nsGIFDecoder2.cpp:423:47> > /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/SurfacePipe.h:426
    #5 0x7f9e9ef740e0 in mozilla::image::nsGIFDecoder2::DoLzw(unsigned char const*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/decoders/nsGIFDecoder2.cpp:423
...
See attached log.
Attached image test_case.gif —
Regressed between the 2016-03-09 and 2016-03-10 nightlies, so most likely a regression from bug 1247152.
Blocks: 1247152
[Tracking Requested - why for this release]: regression, likely sec-crit
Assignee: nobody → seth
Seth, can you take a look at this? Thanks.
Flags: needinfo?(seth)
[Tracking Requested - why for this release]: sec-critical regression
Tracking since it is sec critical and recent regression.
Flags: needinfo?(bugs)
So unfortunately there are several intertwining bugs here. Fixing any one of
them breaks several tests, so we are forced to fix them all at the same time. It
seems that these bugs have mutually interacted in a way that managed to keep our
tests passing despite the code being busted. The test case I'll add in part 4
(which is a GTest based upon the test case attached to this bug) exposes these
issues, though.

In part 1, we fix a nasty mistake in RemoveFrameRectFilter. It manually calls
AdvanceRow() on the next filter in the pipeline, but doing so prevents
SurfaceFilter (the base class from which all these filters are derived) from
knowing that the row has advanced. In particular, its pointer to the current
row's buffer is not updated. This causes some issues when we manually call
AdvanceRow() and then want to call WriteRows() or WritePixels() later.

This patch fixes the issue by using the same approach we used to
ResetToFirstRow(): AdvanceRow() becomes a wrapper around a protected
DoAdvanceRow() virtual method that actually does the advancing. AdvanceRow()
itself just ensures that SurfaceFilter's state is updated appropriately.

Very little has changed in this patch, but the patch looks big because
AdvanceRow()/DoAdvanceRow() implementations have moved from the public section
to the protected section.
Attachment #8756267 - Flags: review?(n.nethercote)
In RemoveFrameRectFilter::AdvanceRow() we were calling WriteRows(), but writing
to |rowBuffer| instead of |aRow|. This obviously did not work as intended.
Attachment #8756269 - Flags: review?(n.nethercote)
When RemoveFrameRectFilter is done working, |mRow| should be at
|mFrameRect.YMost()| - i.e., the last row in the clamped frame rect. Why?
Because |mRow|, as the comments indicate, is in the space of the frame rect,
*not* in the space of the input data. For example, if both the input and the
frame rect are 50x50, but the frame rect is positioned at (25, 25),
RemoveFrameRectFilter should stop its work when mRow is *75*, not 50. Not doing
this can cause RemoveFrameRectFilter::AdvanceRow() to keep processing input data
even when it should've stopped working.

Everywhere where we're marking RemoveFrameRectFilter's work as done, this patch
just replaces |InputSize().height| with |mFrameRect.YMost()|.
Attachment #8756271 - Flags: review?(n.nethercote)
This patch adds a new GTest test that behaves the same as the testcase attached
to the bug. With the patches in parts 1-3, this test passes, along with all the
existing tests. Without those patches, this test fails. (With different failure
modes depending on which of the patches are left out.)
Attachment #8756272 - Flags: review?(n.nethercote)
Flags: needinfo?(seth)
Flags: needinfo?(bugs)
The try job looks good.

One thing I forgot to mention in my comments above is that the patch in part 2 is what actually fixes the *direct* cause of this regression. By writing to |rowPtr| rather than |aRow|, we were writing to a pointer that we might have called AdjustRowPointer() on and thereby added an offset to. With that offset added, writing |aLength| bytes to |rowPtr| meant we could overflow the buffer.
Comment on attachment 8756267 [details] [diff] [review]
(Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called.

Review of attachment 8756267 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfaceFilters.h
@@ +124,5 @@
> +  {
> +    mNext.ResetToFirstRow();
> +    mPass = 0;
> +    mInputRow = 0;
> +    mOutputRow = InterlaceOffset(mPass);;

Nit: Remove the extra ';' while you're here.
Attachment #8756267 - Flags: review?(n.nethercote) → review+
Attachment #8756269 - Flags: review?(n.nethercote) → review+
Attachment #8756271 - Flags: review?(n.nethercote) → review+
Comment on attachment 8756272 [details] [diff] [review]
(Part 4) - Add a new test for downscaling combined with a top-left frame rect.

Review of attachment 8756272 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for tests.
Attachment #8756272 - Flags: review?(n.nethercote) → review+
Thanks for the reviews! I'll upload an updated version of part 1 that removes that extra semicolon, but let me go ahead and request sec-approval now so we can get this fix out ASAP.
Comment on attachment 8756267 [details] [diff] [review]
(Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called.

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

This particular patch is not directly security-related, it's just necessary for continued correctness after the other changes.
Attachment #8756267 - Flags: sec-approval?
Comment on attachment 8756271 [details] [diff] [review]
(Part 3) - When RemoveFrameRectFilter is finished, mRow should be at mFrameRect.YMost().

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

This particular patch is not directly security-related, it's just necessary for continued correctness after the other changes.
Attachment #8756271 - Flags: sec-approval?
Attachment #8756271 - Flags: sec-approval? → sec-approval+
Attachment #8756267 - Flags: sec-approval? → sec-approval+
We'll want this nominate for Aurora too so it is fixed everywhere.
Comment on attachment 8756269 [details] [diff] [review]
(Part 2) - Use the WriteRows API correctly in RemoveFrameRectFilter::AdvanceRow().

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

It's probably not terribly hard to infer that there's a buffer overflow happening here. What the buffer overflows into is pretty much random since it overflows off the end of a chunk of malloc'ed memory.

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

The comments don't, but the combination of the change in this part and the test might allow somebody somebody to infer reasonably easily that a buffer overflow was happening and give them an idea for how to generate an image that would trigger it.

Which older supported branches are affected by this flaw?

It's on Aurora and Central.

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

See the blocked bug.

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

A backport to Aurora is trivial, as this code has not changed.

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

Fairly unlikely, in my view, given the huge number of tests for this code. It took a really strange combination of bugs to get past the tests this time. (And the new test I've introduced in part 4 will make it even harder to sneak things past in the future.)
Attachment #8756269 - Flags: sec-approval?
Comment on attachment 8756272 [details] [diff] [review]
(Part 4) - Add a new test for downscaling combined with a top-left frame rect.

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

This is just a new test. See the approval request for part 2 for comments. Basically this test doesn't explicitly say anything about buffer overflows and is very similar to other tests in the same file, but when checked in together with part 2, it could be suggestive.
Attachment #8756272 - Flags: sec-approval?
Comment on attachment 8756267 [details] [diff] [review]
(Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called.

Approval Request Comment
[Feature/regressing bug #]: See the blocked bug.
[User impact if declined]: Buffer overflow.
[Describe test coverage new/current, TreeHerder]: A new test has been added in part 4.
[Risks and why]: Should be low risk; this code is very well tested.
[String/UUID change made/needed]: None.
Attachment #8756267 - Flags: approval-mozilla-aurora?
Attachment #8756269 - Flags: approval-mozilla-aurora?
Attachment #8756271 - Flags: approval-mozilla-aurora?
Attachment #8756272 - Flags: approval-mozilla-aurora?
Attachment #8756267 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8756269 - Flags: sec-approval?
Attachment #8756269 - Flags: sec-approval+
Attachment #8756269 - Flags: approval-mozilla-aurora?
Attachment #8756269 - Flags: approval-mozilla-aurora+
Attachment #8756271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8756272 - Flags: sec-approval?
Attachment #8756272 - Flags: sec-approval+
Attachment #8756272 - Flags: approval-mozilla-aurora?
Attachment #8756272 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5442ff12b331819dbcc1121053e68231763cb6c
Bug 1262338 (Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/bea7a677b4191bb1bc5b017fdd73924c77f8b5ba
Bug 1262338 (Part 2) - Use the WriteRows API correctly in RemoveFrameRectFilter::AdvanceRow(). r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7ce4b5807cf02d6132e823439f3bc84904b2df
Bug 1262338 (Part 3) - When RemoveFrameRectFilter is finished, mRow should be at mFrameRect.YMost(). r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/62e88bb12275849e9882e47836466985b349638e
Bug 1262338 (Part 4) - Add a new test for downscaling combined with a top-left frame rect. r=njn
Thanks for the quick approvals!

(In reply to Seth Fowler [:seth] [:s2h] from comment #15)
> Thanks for the reviews! I'll upload an updated version of part 1 that
> removes that extra semicolon, but let me go ahead and request sec-approval
> now so we can get this fix out ASAP.

Doh! I just realized I forgot to do this. I'll land the change as a quick DONTBUILD.

I'll get this uplifted once it hits central.
Group: gfx-core-security → core-security-release
Group: core-security-release
Version: unspecified → 48 Branch
Blocks: grizzly
You need to log in before you can comment on or make changes to this bug.