Use-after-poison in nsFrameList::UnhookFrameFromSiblings with moz-column

RESOLVED FIXED in Firefox 24

Status

()

Core
Layout: Block and Inline
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla24
assertion, crash, csectype-framepoisoning, sec-other, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox24 fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main24+])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 739896 [details]
testcase

Debug:
Assertion failure: !GetPropTableFrames(aPresContext, aProperty), at layout/generic/nsContainerFrame.cpp:1459

ASan:
Use-after-poison of pres arena memory in nsFrameList::UnhookFrameFromSiblings.

Nightly:
No symptoms.
(Reporter)

Comment 1

5 years ago
Security-sensitive in case this is a long-standing bug revealed by bug 729519.
(Assignee)

Updated

5 years ago
Assignee: nobody → matspal
(Assignee)

Comment 2

5 years ago
Created attachment 740048 [details]
frame dump #1

The check for when to forget about mOverflowContList is wrong.
http://hg.mozilla.org/mozilla-central/annotate/4420f27742c7/layout/generic/nsContainerFrame.cpp#l1770

Here's the stack / frame tree for the call to Finish() when we
shouldn't forget about it, because only the first two children
(ColumnSet frames) will be removed.

It seems we also update mSentry/mPrevOverflowCont wrong -
if I fix the above bug, then the resulting mSentry/mPrevOverflowCont
points to a frame (red) that is going to be destroyed.  I think mSentry
should point to the green block, and mPrevOverflowCont its prev-in-flow
(pink), but I'm not really sure...
(Assignee)

Comment 3

5 years ago
I'm pretty sure this is mitigated by frame poisoning.
After nsOverflowContinuationTracker::Insert creates a new list and overwrites
the ExcessOverflowContainers property (the assertion in comment 0) there's
no way to access the old list anymore.  The crash is either going to be
accessing a destroyed frame through the bogus mSentry/mPrevOverflowCont
or when DeleteNextInFlowChild is trying to unhook one of the frames and finds
a next-sibling that is destroyed because we failed to remove it from the new
frame list earlier so it's still hooked up (the ASan error above).
Keywords: csec-framepoisoning, sec-other
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 4

5 years ago
Created attachment 740062 [details]
frame dump #2

BEFORE: mSentry=0x7f4fe5b03330 mPrevOverflowCont=(nil)
AFTER:  mSentry=0x7f4fe5b03af0 mPrevOverflowCont=(nil)

fantasai, does this makes sense to you for the given frame trees?
Attachment #740062 - Flags: feedback?(fantasai.bugs)
(Assignee)

Comment 5

5 years ago
Created attachment 740090 [details] [diff] [review]
fix

Call BeginFinish() before DeleteNextInFlowChild/StealFrame on the kid's
next-in-flow and EndFinish() after, with the help of the new AutoFinish
class.

The old Finish() method tried to figure out whether the OC frame list
would become empty, and thus deleted.  This is too fragile and never
really worked.  Instead, compare the list pointer with mParent's 
OC lists after the kid's next-in-flows are gone, in EndFinish(),
and re-setup the tracker's list if it was deleted.

Break out the list setup code from the ctor to a new method,
SetupOverflowContList(), so that it can be used for that purpose.

In BeginFinish, only try to detect if mSentry/mPrevOverflowCont
are going stale and repair those later in EndFinish().
(to my surprise "f == mPrevOverflowCont" actually do occur, which seems
to contradict the documentation that "it's a known good point; this
pointer won't be deleted on us")

I'm assuming that if we bump into mSentry without seeing
mPrevOverflowCont then mPrevOverflowCont is still valid - does this
assumption hold?

It would be great if you could elaborate on what mPrevOverflowCont/
mSentry is used for, and the relation between them.  If one is null,
what does that mean? and what values can the other have in that case?

https://tbpl.mozilla.org/?tree=Try&rev=c3cebb6cf160
Attachment #740090 - Flags: review?(fantasai.bugs)
(Reporter)

Comment 6

5 years ago
WFM on mozilla-central??
(Reporter)

Comment 7

5 years ago
The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/52127bafd50b
user:        Scott Johnson
date:        Wed Apr 24 10:02:36 2013 -0500
summary:     Bug 857324: Make column set reflow continue without balancing rather than restarting if computed height is exceeded. [r=mats]
(Assignee)

Comment 8

5 years ago
Comment on attachment 740090 [details] [diff] [review]
fix

I still think we should fix this.
Attachment #740090 - Flags: review?(fantasai.bugs) → review?(roc)
Comment on attachment 740090 [details] [diff] [review]
fix

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

I'm rubber-stamping this.
Attachment #740090 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Blocks: 881090
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/48bbd10759f7
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/48bbd10759f7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Jesse Ruderman from comment #1)
> Security-sensitive in case this is a long-standing bug revealed by bug
> 729519.

This means 22 and 23 are affected by this and would, ideally, have the fix ported to them, right?
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
(Assignee)

Comment 13

5 years ago
Bug 729519 was fixed in 22 so this crash is mitigated by arena poisoning
from that version, so it's not necessary to uplift this IMO.
Same for 23?
(Assignee)

Comment 15

5 years ago
Yes, 22 and all later versions.
status-firefox22: affected → wontfix
status-firefox23: affected → wontfix
tracking-firefox22: ? → ---
tracking-firefox23: ? → ---
Duplicate of this bug: 881090
Whiteboard: [adv-main24+]
status-b2g18: --- → unaffected
(Assignee)

Comment 17

3 years ago
Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e37220caed
Group: core-security
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c4e37220caed
You need to log in before you can comment on or make changes to this bug.