If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Heap-use-after-free in UnhookTextRunFromFrames

RESOLVED FIXED in Firefox 38, Firefox OS v1.4

Status

()

Core
Layout
--
critical
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(4 keywords)

unspecified
mozilla40
crash, csectype-uaf, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38+ fixed, firefox39+ fixed, firefox40+ fixed, firefox-esr3138+ 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)

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
+++ 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

3 years ago
Created attachment 8577611 [details]
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.
(Assignee)

Comment 2

3 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

3 years ago
Created attachment 8577657 [details] [diff] [review]
wip

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.
(Assignee)

Comment 5

3 years ago
That's a good suggestion, thanks.
(Assignee)

Comment 6

3 years ago
Created attachment 8577807 [details] [diff] [review]
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.
Assignee: nobody → mats
Attachment #8577657 - Attachment is obsolete: true
status-firefox36: affected → wontfix
status-firefox37: affected → wontfix
status-firefox40: --- → affected
Any updates on a final patch so we can get this into the 38 release?
Flags: needinfo?(mats)
(Assignee)

Comment 8

3 years ago
Created attachment 8590476 [details] [diff] [review]
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.
Attachment #8577807 - Attachment is obsolete: true
Flags: needinfo?(mats)
Attachment #8590476 - Flags: review?(roc)
Attachment #8590476 - Flags: review?(roc) → review+
(Assignee)

Comment 9

3 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 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+
tracking-firefox38: --- → +
tracking-firefox39: --- → +
tracking-firefox40: --- → +
tracking-firefox-esr31: --- → 38+
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91047ecbd9f
Flags: in-testsuite-
(Assignee)

Comment 12

3 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

3 years ago
Created attachment 8591160 [details] [diff] [review]
ESR31 patch

[Approval Request Comment]
see above
Attachment #8591160 - Flags: approval-mozilla-esr31?
https://hg.mozilla.org/mozilla-central/rev/d91047ecbd9f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/790150e0ea11
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/2659ba26dcf2
status-firefox38: affected → fixed
status-firefox-esr38: affected → fixed
Attachment #8591160 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
https://hg.mozilla.org/releases/mozilla-esr31/rev/ce44e02200c4
status-firefox-esr31: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7fa668810d43
status-b2g-v2.2: affected → fixed
status-b2g-master: affected → fixed
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
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: affected → fixed
status-b2g-v2.0M: affected → fixed
status-b2g-v2.1: affected → fixed
status-b2g-v2.1S: affected → fixed
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

2 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)
Whiteboard: [asan] → [asan][adv-main38+][adv-main31.7+]
Whiteboard: [asan][adv-main38+][adv-main31.7+] → [asan][adv-main38+][adv-esr31.7+]

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release

Updated

10 months ago
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.