1143299 - Heap-use-after-free in UnhookTextRunFromFrames

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

+++ 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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

Alt. 3 would look something like this.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Mats Palmgren (inactive)]

That's a good suggestion, thanks.

Comment 6

[Mats Palmgren (inactive)]

Attachment: wip, alt 4.

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.

Comment 7

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

Any updates on a final patch so we can get this into the 38 release?

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

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.

Comment 9

[Mats Palmgren (inactive)]

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.

Comment 10

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

Comment on attachment 8590476 [details] [diff] [review]
fix

sec-approval+ for trunk. Please create and nominate Aurora, Beta, and ESR31 patches.

Comment 11

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d91047ecbd9f

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Mats Palmgren (inactive)]

Attachment: ESR31 patch

[Approval Request Comment]
see above

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d91047ecbd9f

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590476 [details] [diff] [review]
fix

Should be in beta 4.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/790150e0ea11

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2659ba26dcf2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/ce44e02200c4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7fa668810d43

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is there a need for manually testing this fix? If yes, could you please provide some detailed steps on how to do it?

Comment 22

[Mats Palmgren (inactive)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #21)
> Is there a need for manually testing this fix?

No.