Heap-use-after-free [@ nsStyleContext::~nsStyleContext]

VERIFIED FIXED in Firefox 44

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: jruderman, Assigned: dbaron)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44+ verified, firefox45+ verified, firefox46+ verified, firefox-esr3844+ verified, firefox-esr45 verified, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-v2.5 fixed, b2g-v2.2r affected, b2g-master fixed)

Details

(Whiteboard: [adv-main44+][adv-esr38.6+])

Attachments

(4 attachments, 1 obsolete attachment)

No description provided.
Posted file stacks (ASan)
ERROR: AddressSanitizer: heap-use-after-free on address 0x625000c4a480 at pc 0x00010a3c8fbd bp 0x7fff5fbfcfa0 sp 0x7fff5fbfcf98
WRITE of size 8 at 0x625000c4a480 thread T0
    #0 0x10a3c8fbc in nsStyleContext::~nsStyleContext()
###!!! ASSERTION: must be in the same rule tree as parent: 'r1 == r2', file layout/style/nsStyleContext.cpp, line 112

Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at layout/base/nsPresShell.cpp:827
So the second assertion is basically telling the same thing as the use-after-free.

The first assertion is a bit more interesting.  I don't remember exactly what we're supposed to do for win1.getComputedStyle(element in win2), but it's probably not that.

It's not yet clear to me whether the first assertion is related to the second assertion and the use-after-free.
In any case, the second assertion seems related to the fact that when we're resolving the parentContext (i.e., the style for the body), GetPresShellForContent returns null, but nsLayoutUtils::GetStyleFrame returns a frame, so we just return the style context from the frame, which has a style from the document in the opened window.  Then when we resolve the style context for the child (div), GetPresShellFor again returns null, but there's no frame, so we resolve style using the style in the window in which getComputedStyle is running.
Comment on attachment 8696297 [details] [diff] [review]
Don't use frame when not in composed document

I'm curious what you think about this.

This fixes both assertions and the crash (presumably the assertion and crash are related to the owning pointer going from the parent document's style context tree to the opened window's document's style context tree), but it feels like wallpaper.  It might be a decent belt-and-suspenders check, though, but I feel like there should be another underlying bug fix.

It feels very odd that an element has a frame at a point when its GetComposedDoc is null (and presumably the ParentChainChanged notification saying that it's no longer in the document has already fired, although I didn't check that).  Should that surprise us, or not?
Attachment #8696297 - Flags: feedback?(bzbarsky)
Oh, and after looking at the testcase again, I presume this element that has a frame but is no longer in a composed document is actually an element (x in the testcase) that is now in a bfcached document, but that the JS is still holding onto.
Comment on attachment 8696297 [details] [diff] [review]
Don't use frame when not in composed document

> I don't remember exactly what we're supposed to do for 
> win1.getComputedStyle(element in win2)

Spec says to return the style using the rules from win2.  I think we get that wrong right now in cases when the element has a frame.  So an ideal fix here would be to just fix that altogether.

But failing that, this is probably an ok wallpaper.

> It feels very odd that an element has a frame at a point when its
> GetComposedDoc is null 

I just stepped through this, and GetComposedDoc() is not null.  However, GetShell() on that document _is_ null.  And that's because it has a GetBFCacheEntry(), so returns null from GetShell(), even though mPresShell is not null.  Which all makes sense: the element in this case is from a document that has been unloaded but bfcached.  We should _really_ not be using frame information from there.
Attachment #8696297 - Flags: feedback?(bzbarsky) → feedback+
David,

Is this ready to go in? In order to make the 44 release (assuming affected there) this needs to get sec-approval, go into trunk, and then be nominated for branches very soon.
Flags: needinfo?(dbaron)
Attachment #8696297 - Attachment is obsolete: true
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #8704737 - Flags: review?(cam) → review+
Comment on attachment 8704737 [details] [diff] [review]
Don't use frame when not in composed document

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

Not trivially, although it does point in the direction to search for problems.

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

No.

Which older supported branches are affected by this flaw?

All, I think.

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

Backports should be trivial.

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

Reasonably low-risk, I think, although not zero.  It does change behavior -- although I think it's probably only changing behavior in cases where we would have at least seen assertions before, if not crashes.
Attachment #8704737 - Flags: sec-approval?
Comment on attachment 8704737 [details] [diff] [review]
Don't use frame when not in composed document

We'll want to take this on Aurora, Beta, and ESR38 as well.
Attachment #8704737 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/e0603b355f0c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8704737 [details] [diff] [review]
Don't use frame when not in composed document

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: potential sec-critical
[Describe test coverage new/current, TreeHerder]: tested fix locally
[Risks and why]: Reasonably low-risk, I think, although not zero.  It does change behavior in an edge case -- although I think it's probably only changing behavior in cases where we would have at least seen assertions before, if not crashes.
[String/UUID change made/needed]: no
Attachment #8704737 - Flags: approval-mozilla-esr38?
Attachment #8704737 - Flags: approval-mozilla-beta?
Attachment #8704737 - Flags: approval-mozilla-aurora?
Comment on attachment 8704737 [details] [diff] [review]
Don't use frame when not in composed document

Fix a sec-critical and a crash, taking it.
Attachment #8704737 - Flags: approval-mozilla-esr38?
Attachment #8704737 - Flags: approval-mozilla-esr38+
Attachment #8704737 - Flags: approval-mozilla-beta?
Attachment #8704737 - Flags: approval-mozilla-beta+
Attachment #8704737 - Flags: approval-mozilla-aurora?
Attachment #8704737 - Flags: approval-mozilla-aurora+
Group: layout-core-security → core-security-release
Flags: qe-verify+
With the fix, the rebase looks good to me.
Flags: needinfo?(dbaron)
Reproduced with: 
* 44.0b6 asan build (Build ID: 20160105153132), under Ubuntu 12.04 64-bit:
> ==4066==ERROR: AddressSanitizer: heap-use-after-free on address 0x625001c43788 at pc 0x7f99a63942ca bp 0x7fff4043cd30 sp 0x7fff4043cd28
* 44 beta 4 debug build (20151229120438), under Mac OS X 10.11:
>[522] ###!!! ASSERTION: must be in the same rule tree as parent: 'r1 == r2', file /builds/slave/m-beta-m64-d-00000000000000000/build/layout/style/nsStyleContext.cpp, line 112
>Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at /builds/slave/m-beta-m64-d-00000000000000000/build/layout/base/nsPresShell.cpp:839

Verified fixed with:
* 44.0b8 (Build ID: 20160111185352), latest 44 beta asan build (Build ID: 20160112001452) and 38.5.2esrpre (Build ID: 20160111115631), under Ubuntu 12.04 64-bit 
* 44.0b8, latest 44 beta debug build (Build ID: 20160112001452) and 38.5.2esrpre debug build, under Mac OS X 10.10.5.
Whiteboard: [adv-main44+][adv-esr38.6+]
Confirming this fix also with 45 beta 2 (Build ID: 20160201143558) and latest beta-debug build, across platforms [1].

[1] Mac OS X 10.9.5, Ubuntu 14.04 32-bit and Windows 7 64-bit
Confirming this fix also with 46.0b9 (Build ID: 20160407053945) and 45.0.2ESR build 2 (Build ID: 20160408031450), across platforms [1]. 

[1] Mac OS X 10.10.5, Windows 10 64-bit and Ubuntu 14.04 64-bit
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.