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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: 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.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.gif

Comment 2

[Timothy Nikkel (:tnikkel)]

Regressed between the 2016-03-09 and 2016-03-10 nightlies, so most likely a regression from bug 1247152.

Comment 3

[Timothy Nikkel (:tnikkel)]

[Tracking Requested - why for this release]: regression, likely sec-crit

Comment 4

[Andrew McCreight [:mccr8]]

Seth, can you take a look at this? Thanks.

Comment 5

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-critical regression

Comment 6

[Marcia Knous [:marcia]]

Tracking since it is sec critical and recent regression.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called.

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.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Use the WriteRows API correctly in RemoveFrameRectFilter::AdvanceRow().

In RemoveFrameRectFilter::AdvanceRow() we were calling WriteRows(), but writing
to |rowBuffer| instead of |aRow|. This obviously did not work as intended.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - When RemoveFrameRectFilter is finished, mRow should be at mFrameRect.YMost().

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()|.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a new test for downscaling combined with a top-left frame rect.

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.)

Comment 11

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db8c13ef7a27

Comment 12

[Seth Fowler [:seth] [:s2h]]

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 13

[Nicholas Nethercote [inactive]]

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.

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Seth Fowler [:seth] [:s2h]]

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 16

[Seth Fowler [:seth] [:s2h]]

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.

Comment 17

[Seth Fowler [:seth] [:s2h]]

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.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

We'll want this nominate for Aurora too so it is fixed everywhere.

Comment 19

[Seth Fowler [:seth] [:s2h]]

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.)

Comment 20

[Seth Fowler [:seth] [:s2h]]

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.

Comment 21

[Seth Fowler [:seth] [:s2h]]

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.

Comment 22

[Seth Fowler [:seth] [:s2h]]

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

Comment 23

[Seth Fowler [:seth] [:s2h]]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5442ff12b33
https://hg.mozilla.org/mozilla-central/rev/bea7a677b419
https://hg.mozilla.org/mozilla-central/rev/fc7ce4b5807c
https://hg.mozilla.org/mozilla-central/rev/62e88bb12275

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cbc4f2d34a30255c3a093f6005f87b453491a7a
https://hg.mozilla.org/releases/mozilla-aurora/rev/b487c31f112be41241912d780ca762218b3e663a
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca85cebb809dde03109b92b670a3d34907a24ae4
https://hg.mozilla.org/releases/mozilla-aurora/rev/b9ab03b42c5de945ad520bccc1483e9da0a9d751

credits go to seth for the checkin i just setting the flags :)