Closed
Bug 1262338
Opened 9 years ago
Closed 8 years ago
heap-buffer-overflow write in [@mozilla::image::nsGIFDecoder2::DoLzw]
Categories
(Core :: Graphics: ImageLib, defect)
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)
21.42 KB,
text/plain
|
Details | |
49 bytes,
image/gif
|
Details | |
14.93 KB,
patch
|
n.nethercote
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
n.nethercote
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
n.nethercote
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
5.85 KB,
patch
|
n.nethercote
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
Regressed between the 2016-03-09 and 2016-03-10 nightlies, so most likely a regression from bug 1247152.
Blocks: 1247152
Comment 3•9 years ago
|
||
[Tracking Requested - why for this release]: regression, likely sec-crit
Updated•9 years ago
|
Keywords: sec-critical
Updated•9 years ago
|
Assignee: nobody → seth
Comment 5•9 years ago
|
||
[Tracking Requested - why for this release]: sec-critical regression
status-firefox49:
--- → affected
tracking-firefox49:
--- → ?
Comment 6•9 years ago
|
||
Tracking since it is sec critical and recent regression.
Updated•8 years ago
|
Flags: needinfo?(bugs)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db8c13ef7a27
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(seth)
Flags: needinfo?(bugs)
Assignee | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8756269 -
Flags: review?(n.nethercote) → review+
Updated•8 years ago
|
Attachment #8756271 -
Flags: review?(n.nethercote) → review+
Comment 14•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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?
Assignee | ||
Comment 17•8 years ago
|
||
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?
Updated•8 years ago
|
Attachment #8756271 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Attachment #8756267 -
Flags: sec-approval? → sec-approval+
Comment 18•8 years ago
|
||
We'll want this nominate for Aurora too so it is fixed everywhere.
Assignee | ||
Comment 19•8 years ago
|
||
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?
Assignee | ||
Comment 20•8 years ago
|
||
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?
Assignee | ||
Comment 21•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Attachment #8756269 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8756271 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8756272 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Attachment #8756267 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8756269 -
Flags: sec-approval?
Attachment #8756269 -
Flags: sec-approval+
Attachment #8756269 -
Flags: approval-mozilla-aurora?
Attachment #8756269 -
Flags: approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8756271 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8756272 -
Flags: sec-approval?
Attachment #8756272 -
Flags: sec-approval+
Attachment #8756272 -
Flags: approval-mozilla-aurora?
Attachment #8756272 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 22•8 years ago
|
||
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
Assignee | ||
Comment 23•8 years ago
|
||
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•8 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 25•8 years ago
|
||
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 :)
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Updated•8 years ago
|
status-firefox-esr45:
--- → unaffected
Updated•8 years ago
|
Group: core-security-release
Updated•8 years ago
|
Version: unspecified → 48 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•