use-after-poison (read) at nsIFrame::GetPrevInFlow()

VERIFIED FIXED in Firefox 32

Status

()

defect
P3
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: aki.helin, Assigned: mats)

Tracking

(4 keywords)

Trunk
mozilla32
Points:
---
Bug Flags:
sec-bounty -
in-testsuite +

Firefox Tracking Flags

(firefox30 wontfix, firefox31 wontfix, firefox32 verified, firefox-esr31 wontfix, b2g-v2.0 fixed)

Details

(Whiteboard: [adv-main32-])

Attachments

(5 attachments)

ASan spots a use-after-poison error when the attached page is opened at least in current Aurora builds. Poisoning may make this harmless, but filing as a security bug to be on the safe side.

==888==ERROR: AddressSanitizer: use-after-poison on address 0x62500058adb8 at pc 0x7f5cd76be30b bp 0x7fff5df86370 sp 0x7fff5df86368
READ of size 8 at 0x62500058adb8 thread T0
==888==WARNING: Trying to symbolize code, but external symbolizer is not initialized!
    #0 0x7f5cd76be30a in nsIFrame::GetPrevInFlow() const /home/aki/src/mozilla-aurora/layout/generic/nsIFrame.h:1504
    #1 0x7f5cd766e6de in ~AutoFinish /home/aki/src/mozilla-aurora/layout/generic/nsContainerFrame.h:616
    #2 0x7f5cd76ef8a5 in nsFrame::ReflowAbsoluteFrames(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, bool) /home/aki/src/mozilla-aurora/layout/generic/nsFrame.cpp:4339
    #3 0x7f5cd76ef51d in nsFrame::FinishReflowWithAbsoluteFrames(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, bool) /home/aki/src/mozilla-aurora/layout/generic/nsFrame.cpp:4309
    #4 0x7f5cd76b4ff4 in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/aki/src/mozilla-aurora/layout/generic/nsColumnSetFrame.cpp:1013
    #5 0x7f5cd76991a6 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockReflowContext.cpp:261
    #6 0x7f5cd76829cc in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:3132
    #7 0x7f5cd767a782 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:2076
    #8 0x7f5cd7676253 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:1069
    #9 0x7f5cd76991a6 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockReflowContext.cpp:261
    #10 0x7f5cd76829cc in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:3132
[...]
0x62500058adb8 is located 5304 bytes inside of 8192-byte region [0x625000589900,0x62500058b900)
There are a few of these assertions:
ASSERTION: Column set should be complete if the available height is unconstrained ..., layout/generic/nsColumnSetFrame.cpp, line 1021

and then:
ASSERTION: StealFrame: can't find aChild: 'removed', layout/generic/nsContainerFrame.cpp, line 1279

leading up to the crash.  I suspect the last one is what causes the crash.

Attached is the frame tree + stack when the StealFrame assertion
occurs.  It appears aChild is a NS_FRAME_IS_OVERFLOW_CONTAINER
on an ExcessOverflowContainersList (highlighted in red color),
but nsColumnSetFrame::StealFrame forces the search to the
principal list.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=18214a2cfdb3#1251
Assignee: nobody → matspal
Severity: normal → critical
OS: Linux → All
Priority: -- → P3
Hardware: x86_64 → All
Posted patch part 1Splinter Review
This part fixes the crash.
Attachment #8432091 - Flags: review?(roc)
Posted patch part 2Splinter Review
This part adds:
* MOZ_ASSERT that all frames on the [Excess]OverflowContainersList
lists are NOT true overflow containers, since that's an invariant
we depend on
* add ReflowOverflowContainerChildren to reflow said frames - this is
boiler plate for any abs.pos. container and I don't see any reason
why we shouldn't do it for nsColumnSetFrame
* move the NS_ASSERTION about being fully complete - I think that
only applies to the status from reflowing normal flow children

Note that we still assert for this testcase even after these fixes:

ASSERTION: Column set should be complete if the available height is unconstrained: 'NS_FRAME_IS_FULLY_COMPLETE(aStatus) || aReflowState.AvailableHeight() != NS_UNCONSTRAINEDSIZE', layout/generic/nsColumnSetFrame.cpp, line 1017

ASSERTION: overflow containers must be zero-height: 'aMetrics.Height() == 0', layout/generic/nsBlockFrame.cpp, line 1495

https://tbpl.mozilla.org/?tree=Try&rev=962cb326875d
Attachment #8432092 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/516ad717d7f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0790d28846

(I'll file bugs for the remaining assertions above separately, if needed)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/516ad717d7f4
https://hg.mozilla.org/mozilla-central/rev/fb0790d28846

Is this worth uplifting to Aurora31, being an ESR and all?
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: sec-bounty?
Flags: needinfo?(matspal)
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
I don't think so.  It seems unlikely this crash would occur on
any real web pages so it's not needed for robustness, and frame-
poisoning should make it non-exploitable.
Flags: needinfo?(matspal)
(the remaining assertions are already on file: bug 569193 and bug 574889)
(In reply to Mats Palmgren (:mats) from comment #6)
> and frame-poisoning should make it non-exploitable.

Where is the framepoisoning? The ASAN use-after-poison warning is not the same thing as our release-build framepoisoning, and in fact pretty much can only occur in arenas we didn't framepoison.
Our pres arena frame poisoning is definitely mitigating this crash.
(In reply to Daniel Veditz [:dveditz] from comment #8)
> The ASAN use-after-poison warning is not the
> same thing as our release-build framepoisoning, 

Yes.

> and in fact pretty much can
> only occur in arenas we didn't framepoison.

I don't think that is true.  When Free-ing pres arena objects in 
an ASAN build we do both frame-poisoning and tell ASAN the memory
is "noaccess", so in an ASAN build it will NOT show up as a frame-
poison crash but rather as an ASAN-noaccess crash.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresArena.cpp?rev=d1e2767da13f#109
Marking sec-other per comment 6.
Keywords: sec-other
Comment 9 is useful, thanks. Definitely frame-poisoning.
Group: core-security
Flags: sec-bounty? → sec-bounty-
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49e1d956526
Flags: in-testsuite? → in-testsuite+
Confirmed crash in ASan build of Fx32, 2014-06-03.
Verified fixed in ASan build of Fx32, 2014-08-20.
Verified fixed in release build of Fx32, release candidate from today.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.