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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (may crash Firefox)

Comment 1

[Jesse Ruderman]

Attachment: 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()

Comment 2

[Jesse Ruderman]

Attachment: stacks for assertions (debug)

###!!! 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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Don't use frame when not in composed document

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

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

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Don't use frame when not in composed document

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

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

sec-approval+ for trunk.

Comment 13

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

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0603b355f0c98b14c7dc8ca4784c26aca550937
Bug 1230668 - Don't use frame when not in composed document.  r=heycam

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e0603b355f0c

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to do a bit of work to get this to apply to esr38 since the changes from bug 1134280 never made it onto 38. Hopefully it still works...

https://hg.mozilla.org/releases/mozilla-esr38/rev/8c184c30caa6
Aaand, I messed up the rebase on esr38, so a followup bustage fix: https://hg.mozilla.org/releases/mozilla-esr38/rev/fceff80a84a3

https://hg.mozilla.org/releases/mozilla-aurora/rev/bedac201438f
https://hg.mozilla.org/releases/mozilla-beta/rev/ce5d62493589

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

With the fix, the rebase looks good to me.

Comment 20

[Ada [:adalucinet]]

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.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ce5d62493589

Comment 22

[Ada [:adalucinet]]

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

Comment 23

[Ada [:adalucinet]]

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