Closed
Bug 1243623
Opened 9 years ago
Closed 9 years ago
Use-after-free (poisoned) in nsTableFrame::FixupPositionedTableParts
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox44 | --- | affected |
firefox45 | --- | affected |
firefox47 | --- | verified |
firefox-esr38 | --- | unaffected |
People
(Reporter: amatcama, Assigned: roc)
References
Details
(6 keywords, Whiteboard: [sg:dos][adv-main47+])
Attachments
(4 files)
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 2•9 years ago
|
||
[Tracking Requested - why for this release]: UAF
Group: firefox-core-security → core-security
tracking-firefox44:
--- → ?
tracking-firefox45:
--- → ?
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
Component: Untriaged → Layout
Product: Firefox → Core
Tracking for all current versions since this looks like a bad crash.
Comment 4•9 years ago
|
||
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
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox-esr38:
--- → unaffected
tracking-firefox44:
+ → ---
tracking-firefox45:
+ → ---
tracking-firefox46:
+ → ---
tracking-firefox47:
+ → ---
Ever confirmed: true
Whiteboard: [sg:dos]
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Make sure Roc is aware of this. Prioritize as appropriate.
Flags: needinfo?(roc)
Keywords: regressionwindow-wanted
Assignee | ||
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
Flags: needinfo?(alice0775)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
MozReview-Commit-ID: 2GHprw8YGsx
Assignee | ||
Updated•9 years ago
|
Attachment #8717671 -
Flags: review?(mats)
Comment 12•9 years ago
|
||
(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 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
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•9 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
Comment 16•9 years ago
|
||
> 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•9 years ago
|
||
Thanks.
Comment 18•9 years ago
|
||
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?
Assignee | ||
Comment 19•9 years ago
|
||
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•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•9 years ago
|
Group: layout-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [sg:dos] → [sg:dos][adv-main47+]
Updated•8 years ago
|
Alias: CVE-2016-2823
Comment 21•8 years ago
|
||
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
Updated•8 years ago
|
Alias: CVE-2016-2823
Updated•8 years ago
|
Group: core-security-release
Updated•8 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•