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

VERIFIED FIXED in Firefox 47

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: amatcama, Assigned: roc)

Tracking

(6 keywords)

44 Branch
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 affected, firefox47 verified, firefox-esr38 unaffected)

Details

(Whiteboard: [sg:dos][adv-main47+])

Attachments

(4 attachments)

Reporter

Description

3 years ago
Posted file 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
Reporter

Comment 1

3 years ago
[Tracking Requested - why for this release]: UAF
Group: firefox-core-security → core-security
Component: Untriaged → Layout
Product: Firefox → Core
Tracking for all current versions since this looks like a bad crash.
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.
Group: core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:dos]

Updated

3 years ago
Blocks: 1209994
Make sure Roc is aware of this. Prioritize as appropriate.
Flags: needinfo?(roc)
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!!!
Flags: needinfo?(roc) → needinfo?(alice0775)
Posted file crash.html
Improved testcase
Assignee: nobody → roc
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?
Group: layout-core-security
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
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?
Attachment #8717671 - Flags: review?(mats) → review+
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.
Attachment #8717671 - Flags: sec-approval?
Reporter

Comment 15

3 years ago
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
> 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.
Reporter

Comment 17

3 years ago
Thanks.
Keywords: sec-low
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.
Attachment #8717671 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/6ba4a380e902
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: layout-core-security → core-security-release
Whiteboard: [sg:dos] → [sg:dos][adv-main47+]
Alias: CVE-2016-2823
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.
Status: RESOLVED → VERIFIED
Alias: CVE-2016-2823
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.