Bugzilla

Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]

Assignee

Updated

•

11 years ago

Assignee: nobody → matspal

Updated

•

11 years ago

Flags: sec-bounty?

Assignee

Updated

•

11 years ago

Severity: normal → critical

Keywords: crash, sec-critical, testcase

Hardware: x86_64 → All

Whiteboard: [asan]

Assignee

Comment 1

•

11 years ago

Attached file stack + frame tree — Details

The OverflowContainersList containing the red and blue frames will be
destroyed (because it becomes empty) when the green frame calls
DeleteNextInFlowChild.

We crash because there's a nsOverflowContinuationTracker with
mOverflowContList pointing to that deleted nsFrameList.
We do call nsOverflowContinuationTracker::Finish() though, which
is supposed to take care of nulling out the pointer:
http://hg.mozilla.org/mozilla-central/annotate/c444f2bfadc4/layout/generic/nsContainerFrame.cpp#l1747

The problem is "nif->GetNextSibling() == nif->GetNextInFlow()"
doesn't catch it because the list is not ordered properly. (There's plenty
of "ASSERTION: overflow containers out of order" leading up to the crash.)

Assignee

Comment 2

•

11 years ago

Attached file stack + frame dump for the assertion — Details

Assignee

Comment 3

•

11 years ago

Probably exploitable since nsFrameList is allocated from the general heap.
I should go and fix bug 729519 now so that frame poisoning will protect us
from bugs like this.

Assignee

Comment 4

•

11 years ago

Attached file stack + frame dump when the out-of-order overflow list occurs — Details

The first frame dump is at the start of nsBlockFrame::ReflowBlockFrame.

While reflowing the green block, the ColumnSet child 0x7fffdfc36810 (pink)
decides it's complete, but not fully complete, so we insert its next-in-flow
(red) into the tracker.  The tracker tracks the OverflowContainersList of the
next-in-flow block (cyan) and it already contains the next-in-flow (blue)
of the red frame from a previous reflow.  mPrevOverflowCont is that nif, and
nsOverflowContinuationTracker::Insert happily inserts the red frame *after* it,
resulting in the frame tree at the end.

This looks like a perfectly valid reflow scenario to me, and something that
nsOverflowContinuationTracker::Insert must be able to deal with.

Assignee

Comment 5

•

11 years ago

Attached file stack + frame dump for "ASSERTION: overflow containers out of order" — Details

So, after fixing Insert to maintain order amongst the next-in-flows
on the same list, the next problem is an assertion:
###!!! ASSERTION: overflow containers out of order: '!(mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont))', file layout/generic/nsContainerFrame.cpp, line 1788
http://hg.mozilla.org/mozilla-central/annotate/c444f2bfadc4/layout/generic/nsContainerFrame.cpp#l1677

In this case we're in ReflowOverflowContainerChildren and the tracker list
is the ExcessOverflowContainersList of the block we're reflowing (lime).
After reflowing the pink child frame (complete but not fully complete)
we Insert its next-in-flow (red) into the tracker.  But it's already there so
it triggers the assert.  We StealFrame it though, so the "only" problem is that
we would normally insert after the blue frame which seems misplaced.

Still, what if it was the *only* frame on the excess list?  then StealFrame
would remove it and *delete* the property, leaving mOverflowContList pointing
at freed memory.

Assignee

Comment 6

•

11 years ago

Attached file stack #2 + frame dump for "ASSERTION: overflow containers out of order" — Details

For completeness, that assertion can also occur when the tracker inserts
to the next-in-flow's OverflowContainersList.

Assignee

Comment 7

•

11 years ago

Attached patch fix — Details — Splinter Review

Description of changes in the order they appear:

In ReflowOverflowContainerChildren, if the frame has its own
ExcessOverflowContainersProperty, then append those frames to the list of
frames to reflow.  This avoids the problem of Inserting a next-in-flow
that is already in the tracked list.  It's rare that this occurs.
See comment 5 and its attachment.

In nsOverflowContinuationTracker::Insert, check if the frame to insert
is already in the mOverflowContList.  The ContainsFrame() call there is
unfortunately O(n).  See comment 6.  Given the first change above, it might
be safe to restrict this check to OverflowContainers insertions only.
I think we should fix bug 729519 first though, before making optimizations
that could lead to exploitable bugs.

Last hunk checks if mOverflowContList already has a prev/next-in-flow
for the inserted frame and if so adjusts the insertion point so that the
next-in-flow chain within the list remains ordered.

Fwiw, reftest/crashtests pass in a local ASan debug build on Linux64.

Attachment #726324 - Flags: review?(roc)

Assignee

Updated

•

11 years ago

Keywords: csec-uaf

Robert O'Callahan (:roc) (email my personal email if necessary)

Assignee

Comment 8

•

11 years ago

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

fantasai, I'd appreciate if you could take a look at this patch and
the bug comments / attachments as well.  Thanks.

Attachment #726324 - Flags: feedback?(fantasai.bugs)

Daniel Veditz [:dveditz]

Updated

•

11 years ago

Flags: sec-bounty? → sec-bounty+

Daniel Veditz [:dveditz]

Comment 10

•

11 years ago

Is this a new bug (regression) or has it been around for a while?

status-firefox22: --- → affected

tracking-firefox22: --- → +

Updated

•

11 years ago

Attachment #726324 - Flags: review?(roc) → review+

Assignee

Comment 11

•

11 years ago

(In reply to Daniel Veditz [:dveditz] from comment #10)
> Is this a new bug (regression) or has it been around for a while?

I suspect all supported branches are affected.

status-b2g18: --- → affected

status-b2g18-v1.0.0: --- → affected

status-b2g18-v1.0.1: --- → affected

status-firefox19: --- → affected

status-firefox20: --- → affected

status-firefox21: --- → affected

status-firefox-esr17: --- → affected

Assignee

Comment 12

•

11 years ago

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

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Extremely 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 the same patch should apply. If not, backporting should be easy.

How likely is this patch to cause regressions; how much testing does it need?
Medium risk.

Attachment #726324 - Flags: sec-approval?

Updated

•

11 years ago

Attachment #726324 - Flags: sec-approval? → sec-approval+

Lukas Blakk [:lsblakk] use ?needinfo

Updated

•

11 years ago

tracking-firefox20: --- → ?

tracking-firefox21: --- → ?

Comment 13

•

11 years ago

Given that this is extremely hard to create an exploit for, it's been in the product for a while (though unclear for how long) and risk of regression from taking this in our final beta (going to build today) I'd like to hold off on landing this until FF21 is in beta.

status-firefox20: affected → wontfix

tracking-firefox20: ? → -

tracking-firefox21: ? → +

Assignee

Comment 14

•

11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4992c8267847

Assignee

Updated

•

11 years ago

Flags: in-testsuite?

Ryan VanderMeulen [:RyanVM][PTO June 24-28]

Assignee

Comment 15

•

11 years ago

(In reply to lsblakk@mozilla.com from comment #13)
> Given that this is extremely hard to create an exploit for

Maybe we mean the same thing, but I want to clarify that I didn't say
that the crash condition is hard to exploit (it's not).  I said it
would be extremely hard to figure out an exploit *based on the
information revealed by the patch*.

Anyway, I agree that the regression risk is too high to take this
for beta without baking.

Comment 16

•

11 years ago

https://hg.mozilla.org/mozilla-central/rev/4992c8267847

Status: NEW → RESOLVED

Closed: 11 years ago

status-b2g18-v1.0.0: affected → wontfix

status-firefox19: affected → wontfix

status-firefox22: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla22

Alex Keybl [:akeybl]

Comment 17

•

11 years ago

(In reply to Mats Palmgren [:mats] from comment #12)
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> I expect the same patch should apply. If not, backporting should be easy.

ESR/B2G:21+ in that case.

tracking-b2g18: --- → 21+

tracking-firefox-esr17: --- → 21+

Assignee

Comment 18

•

11 years ago

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): on m-c since 2013-03-26
Risk to taking this patch (and alternatives if risky): medium risk
String or IDL/UUID changes made by this patch: none

Attachment #726324 - Flags: approval-mozilla-aurora?

Comment 19

•

11 years ago

(In reply to Mats Palmgren [:mats] from comment #18)
> Comment on attachment 726324 [details] [diff] [review]
> fix
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: sec-critical crash
> Testing completed (on m-c, etc.): on m-c since 2013-03-26

Did we verify with the testcase once the patch landed on m-c or we need to pull in QA here for verification  ? 

> Risk to taking this patch (and alternatives if risky): medium risk

Any obvious areas of regression to lookout for or additional testing we need qa help here based on the risk evaluation ?

> String or IDL/UUID changes made by this patch: none

Assignee

Comment 20

•

11 years ago

(In reply to bhavana bajaj [:bajaj] from comment #19)
> Did we verify with the testcase once the patch landed on m-c or we need to
> pull in QA here for verification  ? 

The crash was quite hard to reproduce (on Linux64).  I could only do it in an
ASan build, loading 20 or so tabs with the testcase file from the command line
then resizing the window and doing an occasional "Reload All Tabs" from the
tab context menu.  That STR should crash an unfixed ASan build in less than
a minute of trying.  I haven't tried regular builds on other platforms though
so maybe it's easier there.

> Any obvious areas of regression to lookout for or additional testing we need
> qa help here based on the risk evaluation ?

Any regression is probably going to be a crash and most likely in pagination
contexts, such as content styled with -moz-columns or Print/Print Preview.

Comment 21

•

11 years ago

(In reply to Mats Palmgren [:mats] from comment #20)
> (In reply to bhavana bajaj [:bajaj] from comment #19)
> > Did we verify with the testcase once the patch landed on m-c or we need to
> > pull in QA here for verification  ? 
> 
> The crash was quite hard to reproduce (on Linux64).  I could only do it in an
> ASan build, loading 20 or so tabs with the testcase file from the command
> line
> then resizing the window and doing an occasional "Reload All Tabs" from the
> tab context menu.  That STR should crash an unfixed ASan build in less than
> a minute of trying.  I haven't tried regular builds on other platforms though
> so maybe it's easier there.

Adding Matt as QA contact here to see he if can help verify this on other platforms based on above 

> 
> > Any obvious areas of regression to lookout for or additional testing we need
> > qa help here based on the risk evaluation ?
> 
> Any regression is probably going to be a crash and most likely in pagination
> contexts, such as content styled with -moz-columns or Print/Print Preview.

QA Contact: mwobensmith

Updated

•

11 years ago

Attachment #726324 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Assignee

Comment 22

•

11 years ago

https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=974d7b407fd2

status-firefox21: affected → fixed

Assignee

Comment 23

•

11 years ago

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

See comment 12 and forward.

Attachment #726324 - Flags: approval-mozilla-esr17?

Attachment #726324 - Flags: approval-mozilla-b2g18?

Ryan VanderMeulen [:RyanVM][PTO June 24-28]

Updated

•

11 years ago

Attachment #726324 - Flags: approval-mozilla-esr17?

Attachment #726324 - Flags: approval-mozilla-esr17+

Attachment #726324 - Flags: approval-mozilla-b2g18?

Attachment #726324 - Flags: approval-mozilla-b2g18+

Comment 24

•

11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f7efbec12999
https://hg.mozilla.org/releases/mozilla-esr17/rev/ad2a567bfa94

status-b2g18: affected → fixed

status-firefox-esr17: affected → fixed

Matt Wobensmith [:mwobensmith][:matt:]

Comment 25

•

11 years ago

Since we don't have ASan builds available for every date/branch/platform, I'm not able to run a 100% consistent verification. However, I can reproduce the crash in non-ASan builds from the affected period, so I feel it's safe to assume that we can verify this issue despite lack of ASan build availability.


Confirmed crash in 2013-03-13 FF21, both ASan and release
Confirmed no crash in 2013-04-01 FF20, release
Confirmed no crash in 2013-04-01 FF21, release
Confirmed no crash in 2013-04-06 FF21, ASan
Confirmed no crash in 2013-04-06 17esr, release

status-firefox21: fixed → verified

status-firefox22: fixed → verified

status-firefox-esr17: fixed → verified

fantasai

Comment 26

•

11 years ago

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

Sorry for not responding earlier. I kept trying to understand the patch and failing, so I kindof gave up... trying again tonight, and I think maybe it's just wrong.

This bit --
+  // Our own excess overflow containers from a previous reflow can still be
+  // present if our next-in-flow hasn't been reflown yet.
+  nsFrameList* selfExcessOCFrames =
+    RemovePropTableFrames(aPresContext, ExcessOverflowContainersProperty());
+  if (selfExcessOCFrames) {
+    if (overflowContainers) {
+      overflowContainers->AppendFrames(nullptr, *selfExcessOCFrames);
+      delete selfExcessOCFrames;
+    } else {
+      overflowContainers = selfExcessOCFrames;
+      SetPropTableFrames(aPresContext, overflowContainers,
+                         OverflowContainersProperty());
+    }
+  }

This does not make sense. Our excessOC list is a list of frames that are *not ours*. Trying to merge it with our OC list is like trying to grab our next-in-flow's OC list and merge it with ours. Why are we doing this? Or if this is the right thing to do (for some reason), why are we not grabbing our NIF's OC list when our excess list is missing?

I can't think of a reason why ReflowOverflowContainerChildren should be reflowing an overflow continuation *and* its continuation both as children of the current nsContainerFrame. But that seems to be will happen with this patch in place, because the excess list, in theory, consists of continuations of this frame's children (either normal children or OC children).

+    // If aOverflowCont has a prev/next-in-flow that might be in
+    // mOverflowContList we need to find it and insert after/before it to
+    // maintain the order amongst next-in-flows in this list.

My expectation was that aOverflowCont's prev-in-flow would never be in this list, because if we're calling Insert() on aOverflowCont, its prev-in-flow must be the child of this nsContainerFrame that we just reflowed, not a frame that we plan to pass on to our NIF. So this looks wrong as well.

It is possible for aOverflowCont's next-in-flow to be in this list. But we shouldn't have encountered it yet. It should just sit at the end of this list until our NIF reflows this list, at which point it should be stolen from this list and put into our NIF's excessOC list for reflow by *its* NIF.

So this code also looks wrong.

+  // If we have a list and aOverflowCont is already in it then don't try to
+  // add it again.
+  if (addToList && aOverflowCont->GetParent() == mParent &&
+      (aOverflowCont->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) &&
+      mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont)) {
+    addToList = false;
+    mPrevOverflowCont = aOverflowCont->GetPrevSibling();
+  }

If aOverflowCont is already in the list, it should be the very next frame in it. In which case we will step over it further down in this function. If it's not the very next frame in the list, then this code steps over some arbitrary number of frames. What are these frames, and why are we stepping over them? The overflow container lists are supposed to strictly maintain the frame order of their prev-in-flows, so if we are reflowing those frames in the correct order, we should not encounter an overflow container until we have encountered each of its previous-siblings.

----------------

In Comment 4 you wrote that mPrevOverflowCont is the NIF of the aOverflowCont we're trying to insert. That's clearly broken, and is what should be fixed.

It looks like mPrevOveflowCont became that NIF when we were setting up the list walker (walking over the OC continuations, one of which was aOverflowCont's NIF). What we probably should be doing is checking that the OC continuations that we're skipping are the ones we want to keep in this list, and we can check that by checking that their prev-in-flow's parent is our prev-in-flow.

Any OC continuations whose prev-in-flow is not parented by our prev-in-flow don't belong on this list. They have to be kept somewhere, but that should be at the end of the list (because that maintains continuation order) and should not be encountered by our walker or reflowed by ReflowOverflowContainerChildren. (They'll eventually be pushed out to another frame further down the continuation chain, and should be placed correctly once their immediate prev-in-flow is reflowed. We just need to own them until that happens.)

So, r- fantasai. I think the patch is totally wrong. Even though it apparently works fine. :/

I'll look at 863935 next.

Attachment #726324 - Flags: feedback?(fantasai.bugs) → feedback-

Updated

•

11 years ago

Whiteboard: [asan] → [asan][adv-main21+][adv-esr1706+]

Updated

•

11 years ago

Alias: CVE-2013-1680

Assignee

Comment 27

•

11 years ago

(In reply to fantasai from comment #26)
> This does not make sense. Our excessOC list is a list of frames that are
> *not ours*. 

If by "ours" you mean a parent/child relationship then you're mistaken.
I guess you mean they were destined to be picked up by our NIF and
thus should not contribute to our overflow areas.

> Why are we doing this?

The overflow tracker needs to deal with an EOC list from a *previous*
reflow somehow.  As the comment says, this happens when a frame is
reflowed for the second time before its NIF is reflowed so the EOC list
is obsolete anyway.

But you do have a point that reflowing the EOC children here is
unnecessary.  I suspect that it doesn't really matter in practice
since if the reflow conditions are the same they will have been moved
into a new EOC list again before we reach them.

> why are we not grabbing our
> NIF's OC list when our excess list is missing?

Yeah, we should probably drain those for the same reason.

> It is possible for aOverflowCont's next-in-flow to be in this list. But we
> shouldn't have encountered it yet. It should just sit at the end of this
> list until our NIF reflows this list, at which point it should be stolen
> from this list and put into our NIF's excessOC list for reflow by *its* NIF.

You seem to assume that given a NIF chain, A B C, that first A is reflowed
then B, then C.  That's a bogus assumption.  These are all valid reflows:

A B C A ...
A A B C ...
A B A B C ...

> In Comment 4 you wrote that mPrevOverflowCont is the NIF of the
> aOverflowCont we're trying to insert. That's clearly broken, and is what
> should be fixed.

Filed bug 871163.

Daniel Veditz [:dveditz]

Updated

•

11 years ago

Group: core-security