Closed
Bug 1143299
Opened 9 years ago
Closed 9 years ago
Heap-use-after-free in UnhookTextRunFromFrames
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(4 keywords, Whiteboard: [asan][adv-main38+][adv-esr31.7+])
Attachments
(3 files, 2 obsolete files)
15.05 KB,
text/html
|
Details | |
13.83 KB,
patch
|
roc
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
13.58 KB,
patch
|
bkerensa
:
approval-mozilla-esr31+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1141919 +++ Follow-up from bug 1141919 comment 36: It happens here: https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7916 The line: > InsertFrames(insertion.mParentFrame, kPrincipalList, prevSibling, frameItems); tries to insert the newly constructed frames into the principal list while the prevSibling actually referes to a frame in the overflow list.
Assignee | ||
Comment 1•9 years ago
|
||
Here's the frame tree before the InsertFrames() call where the error occurs, the call stack, and the frame tree after the call. Note that both OverflowList and PrincipalList are now bogus.
Assignee | ||
Comment 2•9 years ago
|
||
So, from the top of my head, a few alternative fixes: 1. make the frame constructor check which list 'prevSibling' is really on, and insert accordingly 2. make a convenience function for the frame constructor to use that takes an alias for which frame lists to check, e.g. kNormalFlow for Overflow/Principal lists, and deal with updating the lists there 3. introduce kNormalFlow and other aliases generally that "external" callers should use instead of the exact child list and update all frame methods (Insert/Append/RemoveFrame etc) to deal with it Are there other alternatives? I think 1. is not acceptable b/c it's too slow. 2 is fast because it only needs to check mFirstChild/mLastChild on the aliased lists. 2 and 3 would basically do the same checks although 3 does it more generally, 2 is just fixing the frame constructor. I think I'm clearly in favor of 3, although 2 might be a less risky change for branches. 3 is good from an architectural POV, since it's a step towards removing the aChildList param from frame methods and let frame classes deal with it internally as much as possible (it can deduce the _alias_ from the frame type as needed). BTW, other lists like kFloatList/kPushedFloatsList etc might potentially have the same problem. We need to audit all frame list uses in the frame constructor for this.
Assignee | ||
Comment 3•9 years ago
|
||
Alt. 3 would look something like this.
Comment 4•9 years ago
|
||
Looks good. I guess it'd be better to check whether the prev frame's next sibling is nullptr first, then principal's last, then overflow's last, and trigger a release crash if it doesn't match any of them.
Assignee | ||
Comment 5•9 years ago
|
||
That's a good suggestion, thanks.
Assignee | ||
Comment 6•9 years ago
|
||
Alt 4. DWIM! It seems unnecessary to introduce new aliases when we could just do what the caller intended anyway, i.e. for kPrincipalList, try also kOverflowList. For kFloatList, try also kPushedFloatsList etc. As we already do in RemoveFrame for example. The code in this patch is a bit boilerplate-ish, I should probably move it to nsFrameList.h with an API similar to Start/ContinueRemoveFrame.
Assignee: nobody → mats
Attachment #8577657 -
Attachment is obsolete: true
Updated•9 years ago
|
Updated•9 years ago
|
status-firefox40:
--- → affected
Comment 7•9 years ago
|
||
Any updates on a final patch so we can get this into the 38 release?
Flags: needinfo?(mats)
Assignee | ||
Comment 8•9 years ago
|
||
I implemented a StartInsertFrames/ContinueInsertFrames approach first, but I didn't quite like the complexity, and the fact that for floats/pushedFloats you also need to set frame bits on the child frames etc. So, it's simpler and more robust to just drain the overflow list before doing the insertion, which we already have code for. It's a virtual call and a frame property lookup which I hope overall will be a negligible part of inserting content. It's expected that the overflow list is empty, so the more expensive code (the reparenting that nsInlineFrame does) shouldn't run normally. Patch overview: for nsBlockFrame: * remove the unused aState from nsBlockFrame::DrainPushedFloats * spawn off the first part of that method into DrainSelfPushedFloats * calls DrainSelfPushedFloats before inserting/appending a float (nsBlockFrame already deals with principal/overflow lists for normal flow) for other frame types: * call DrainSelfOverflowList() where needed Not tested on Try yet. I did confirm that it fixes the crash in bug 1141919 *without* Xidorn's fixes there.
Attachment #8577807 -
Attachment is obsolete: true
Flags: needinfo?(mats)
Attachment #8590476 -
Flags: review?(roc)
Attachment #8590476 -
Flags: review?(roc) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8590476 [details] [diff] [review] fix [Security approval request comment] How easily could an exploit be constructed based on the patch? Very hard. 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. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I expect backporting to be easy. How likely is this patch to cause regressions; how much testing does it need? Low risk, this code is well covered by tests so if there's any problem with the patch it should show up in the first test run.
Attachment #8590476 -
Flags: sec-approval?
Comment 10•9 years ago
|
||
Comment on attachment 8590476 [details] [diff] [review] fix sec-approval+ for trunk. Please create and nominate Aurora, Beta, and ESR31 patches.
Attachment #8590476 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
tracking-firefox38:
--- → +
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
tracking-firefox-esr31:
--- → 38+
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91047ecbd9f
Flags: in-testsuite-
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8590476 [details] [diff] [review] fix Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: possible exploitable crashes in rare circumstances, but there are no known ways to trigger it ATM. [Describe test coverage new/current, TreeHerder]: green on Try, this code is well covered by tests [Risks and why]: low-risk [String/UUID change made/needed]: none
Attachment #8590476 -
Flags: approval-mozilla-beta?
Attachment #8590476 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 13•9 years ago
|
||
[Approval Request Comment] see above
Attachment #8591160 -
Flags: approval-mozilla-esr31?
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d91047ecbd9f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 15•9 years ago
|
||
Comment on attachment 8590476 [details] [diff] [review] fix Should be in beta 4.
Attachment #8590476 -
Flags: approval-mozilla-beta?
Attachment #8590476 -
Flags: approval-mozilla-beta+
Attachment #8590476 -
Flags: approval-mozilla-aurora?
Attachment #8590476 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8591160 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Comment 20•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3a999c888481 https://hg.mozilla.org/releases/mozilla-b2g34_v2_12/rev/3a999c888481 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/66b55766c8e4 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/66b55766c8e4 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fff6bfb4963
Comment 21•9 years ago
|
||
Is there a need for manually testing this fix? If yes, could you please provide some detailed steps on how to do it?
Flags: needinfo?(mats)
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #21) > Is there a need for manually testing this fix? No.
Flags: needinfo?(mats)
Updated•9 years ago
|
Whiteboard: [asan] → [asan][adv-main38+][adv-main31.7+]
Updated•9 years ago
|
Whiteboard: [asan][adv-main38+][adv-main31.7+] → [asan][adv-main38+][adv-esr31.7+]
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
Updated•7 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•