1243623 - Use-after-free (poisoned) in nsTableFrame::FixupPositionedTableParts

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[raiju]

Attachment: poc

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.82 Safari/537.36

Steps to reproduce:

firefox uaf.html


Actual results:

firefox crashed


Expected results:

firefox shouldn't have crashed

Comment 1

[raiju]

Attachment: backtrace + instruction + registers

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: UAF

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for all current versions since this looks like a bad crash.

Comment 4

[Daniel Veditz [:dveditz]]

In a Firefox 44 release build I get a null deref: bp-7fb09c44-0944-42b1-97e0-c473c2160130

As in the attached stack trace I see register values with 0x7ffffffff0deaxxx which is our frame-poisoned address. It's a UAF of a nsIFrame object which we've specifically neutered by pointing to unmapped memory to cause guaranteed crashes. We can treat this as a DOS crash.

I do NOT crash in ESR 38.6 so this problem was introduced somewhere between 38 and 44.

Comment 5

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=789a931f4344ad2d36a8e7bb92634b618d4e3b58&tochange=beb4cf7d167970d5c774bfe903211e8d58633829

Suspect: Bug 1209994

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Make sure Roc is aware of this. Prioritize as appropriate.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I do not crash in f53533d9eb77 (Feb 4 Nightly). Alice, does this crash in Nightly for you? If not, can you get a fix range? Thanks!!!

Comment 8

[Alice0775 White]

Fixed range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=94c27b21c934f0cc178e896587532d7cc0b4c60c&tochange=6f4756d4bab615ade4e575b2f3343772a926abd0

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: crash.html

Improved testcase

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that we're creating an nsTableFrame and splitting it in a non-print context. Our table code, in particular our positioned-table-part handling, is crashing on dynamic changes to tables with continuations. We can probably fix the particular crash here but dynamic changes to tables with continuations is generally known to fail.

The patches in bug 1243125 fix the root cause that's allowing non-printed tables to split. We should uplift them everywhere; although bug 1209994 exposed this particular crash, there are almost certainly others that work before bug 1209994 was fixed. Jonathan, can you backport the bug 1243125 fixes?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Don't skip unregistering a table part if we have a split table

MozReview-Commit-ID: 2GHprw8YGsx

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> The patches in bug 1243125 fix the root cause that's allowing non-printed
> tables to split. We should uplift them everywhere; although bug 1209994
> exposed this particular crash, there are almost certainly others that work
> before bug 1209994 was fixed. Jonathan, can you backport the bug 1243125
> fixes?

It looks like they'll uplift easily; I've posted a rollup patch there and requested approval.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8717671 [details] [diff] [review]
Don't skip unregistering a table part if we have a split table

>+    // The table frame will be destroyed, and it's the first im flow (and thus
>+    // owning the PositionedTablePartArray), so we don't need to do
>+    // anything.

s/im/in/ and it looks like "anything." fits on the second line?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8717671 [details] [diff] [review]
Don't skip unregistering a table part if we have a split table

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

Not sure when I should land this ... we believe this particular bug is blocked by frame poisoning, but the general issue of tables being split might lead to other exploitable bugs.

Comment 15

[raiju]

One question I had about frame poisoning, are the frames ever "recycled" ? Suppose I create a large number of the same objects which end up in the same frame. What happens if I free all these objects and the frame is "empty" ? Is this frame ever released and then later used as a frame for other objects ? Because I don't understand how memory leaks could be prevented if this is not the case.
Thanks

Comment 16

[Mats Palmgren (inactive)]

> are the frames ever "recycled" ?

The "frame" in "frame poisoning" refers to the CSS box for a DOM node (roughly).
I.e. subclasses of nsIFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#415

These are allocated from an memory arena per document and not free'd in the normal
malloc/free sense.  Instead they are cached, and re-used for the next element that
needs a frame *of that type*.  So once a memory location has been allocated for
say a table frame it will always be a table frame at that memory location.  Also,
while the frame is unused in the cache it's filled with "poison" which will lead
to a crash if you try to use it as a pointer. Both these properties are good for
security, either you crash, or if a new frame is re-using the memory, it's of
the same type as the last one (which prevents using the vtable of a table frame
with a frame of a different type for example).

After the document is destroyed though the whole memory arena is free'd so the memory
can be reused for arbitrary things.  A use-after-free of a frame after that happens
can be exploitable.  That's very rare though.

You're right that there's sort of a memory leak here, because we will never go under
the high-water mark for the number of frames that was allocated in a document.
For typical web content this spill should be fairly low though.  Frame poisoning
has mitigated a ton of bugs so it's a small price to pay.

Comment 17

[raiju]

Thanks.

Comment 18

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

Comment on attachment 8717671 [details] [diff] [review]
Don't skip unregistering a table part if we have a split table

Making this a sec-low after talking to Dan Veditz. Doesn't need sec-approval to go in unless it is sec-high or sec-critical so clearing that. Check in.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba4a380e9020f75cf016ca358c3982031385434
Bug 1243623. Don't skip unregistering a table part if we have a split table. r=mats

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ba4a380e902

Comment 21

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I've managed to reproduce the crash using poc.html/crash.html on an affected nightly 47.0a1 build(20160204030229)  from 2016-02-04, under Windows 10 x64.
This is no longer reproducible with 47.0b9 build (20160526140250) on the following platforms: Windows  10 x64,  Mac OS X 10.9.5 and Ubuntu 14.04 LTS  x64.