Closed Bug 1143299 Opened 9 years ago Closed 9 years ago

Heap-use-after-free in UnhookTextRunFromFrames

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed
firefox-esr31 38+ fixed
firefox-esr38 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(4 keywords, Whiteboard: [asan][adv-main38+][adv-esr31.7+])

Attachments

(3 files, 2 obsolete files)

+++ 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.
Attached file stack + frame dumps
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.
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.
Attached patch wip (obsolete) — Splinter Review
Alt. 3 would look something like this.
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.
That's a good suggestion, thanks.
Attached patch wip, alt 4. (obsolete) — Splinter Review
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
Any updates on a final patch so we can get this into the 38 release?
Flags: needinfo?(mats)
Attached patch fixSplinter Review
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)
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 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+
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?
Attached patch ESR31 patchSplinter Review
[Approval Request Comment]
see above
Attachment #8591160 - Flags: approval-mozilla-esr31?
https://hg.mozilla.org/mozilla-central/rev/d91047ecbd9f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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+
Attachment #8591160 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
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)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #21)
> Is there a need for manually testing this fix?

No.
Flags: needinfo?(mats)
Whiteboard: [asan] → [asan][adv-main38+][adv-main31.7+]
Whiteboard: [asan][adv-main38+][adv-main31.7+] → [asan][adv-main38+][adv-esr31.7+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.