Closed Bug 1265577 Opened 8 years ago Closed 8 years ago

don't rely on the views we stash when we destroy a subdocument frame to stick around

Categories

(Core :: Web Painting, defect)

17 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 + fixed
firefox48 + fixed
firefox49 + fixed
firefox-esr45 47+ fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main47+][adv-esr45.2+])

Attachments

(4 files, 1 obsolete file)

Nothing forces them to stick around. The only thing that makes it true most of the time is we clear the stashed views on a script runner, limiting the total changes that can happen while we have them stored. But nothing prevents those changes from including destroying the stored views.
Attached patch patch (obsolete) — Splinter Review
I wasn't sure if it was better to call frameloader->Hide() in nsSubDocumentFrame::DestroyFrom in the case that we aren't trying to stash the previous root frame, or to leave it to nsHideViewer to do that.
Attachment #8742549 - Flags: review?(mats)
This could cause us to use a deleted view. We poison views when we delete them, but I don't think we hold that memory in an arena like frames, so it could be re-used right away.
After a chat with :tnikkel it sounds like this patch could fix bug 1261175 and bug 1259572
Keywords: crash, csectype-uaf
Group: core-security → gfx-core-security
Marking sec-critical assuming this fixes bug 1259572 and because of the number of crash stats in bug 1259572 (it is likely reproducible).
Keywords: sec-critical
Comment on attachment 8742549 [details] [diff] [review]
patch

>layout/generic/nsSubDocumentFrame.cpp
>+    if (oldContainerDoc) {

The old code doesn't have this check, so it falls through to
"frameloader->Hide()" AFAICT (since OwnerDoc() is never null).
The new code does nothing in this case.
Why change the behavior for the null oldContainerDoc case?
Are you assuming 'detachedFrame' must be null in this case anyway?
Can we add an else-branch that MOZ_ASSERTs if so?
(I think it should hold)

>     nsView* detachedViews = ::BeginSwapDocShellsForViews(mInnerView->GetFirstChild());
>-    frameloader->SetDetachedSubdocView(detachedViews, mContent->OwnerDoc());
>+    bool tryToStashSubDoc =
>+      detachedViews && detachedViews->GetFrame() && !detachedViews->GetNextSibling();

The "!detachedViews->GetNextSibling()" condition is new here.
Why do we check that exactly?
Is the sibling view associated with the mPreviousViewer perhaps?
If so, could we simply destroy that viewer instead, so that we
can stash the frame here?

Also, in the !detachedViews->GetFrame() case, isn't there a perf loss
because we will recreate the sub doc pres shell/context when we reframe
an iframe that has no root frame yet (and this seems to happen more than
I thought - during startup for example).
Perhaps it's negligible though.
Flags: needinfo?(tnikkel)
Thanks for your careful review of this hairy code!

(In reply to Mats Palmgren (:mats) from comment #5)
> >layout/generic/nsSubDocumentFrame.cpp
> >+    if (oldContainerDoc) {
> 
> The old code doesn't have this check, so it falls through to
> "frameloader->Hide()" AFAICT (since OwnerDoc() is never null).
> The new code does nothing in this case.
> Why change the behavior for the null oldContainerDoc case?

The old code used the stored views being null as a sentinel as to weather we try to use those stashed views, or call Hide on the frameloader. The new code uses a weak frame instead of a view. So there are two cases where we get a null frame: 1) we never stored a frame 2) we stored a frame but it was destroyed. In 1) we do not want to call Hide. For 2) we want to call Hide. So we can't use the presence or not of the stored frame to decide anymore. So instead I use the stored container doc check.

> Are you assuming 'detachedFrame' must be null in this case anyway?
> Can we add an else-branch that MOZ_ASSERTs if so?
> (I think it should hold)

Yes, I believe OwnerDoc can never return null. I can assert.

> >     nsView* detachedViews = ::BeginSwapDocShellsForViews(mInnerView->GetFirstChild());
> >-    frameloader->SetDetachedSubdocView(detachedViews, mContent->OwnerDoc());
> >+    bool tryToStashSubDoc =
> >+      detachedViews && detachedViews->GetFrame() && !detachedViews->GetNextSibling();
> 
> The "!detachedViews->GetNextSibling()" condition is new here.
> Why do we check that exactly?
> Is the sibling view associated with the mPreviousViewer perhaps?
> If so, could we simply destroy that viewer instead, so that we
> can stash the frame here?

Yes, good catch. I wanted to make this change in a separate patch but it turned out to require more work.

We have more than one view/frame here during document navigation; where there is a non-zero time with two "active" documents. This seemed like a bad time to do these kinds of acrobatics. The bad things that could happen though seem like they would be solved by storing weak frames. So I could create a patch that stores multiple weak frames and tries to restore them. It creates some more "interesting" questions though like what to do if one of two weak frames are destroyed? Call Hide? Just restore the one frame?

It seems more risky to introduce destroying the previous viewer, so I'd like to avoid that if possible (or proven to be the less risky option :) ).

> Also, in the !detachedViews->GetFrame() case, isn't there a perf loss
> because we will recreate the sub doc pres shell/context when we reframe
> an iframe that has no root frame yet (and this seems to happen more than
> I thought - during startup for example).
> Perhaps it's negligible though.

Only way I can think of to avoid that is to re-introduce weak views and store weak views. Worth it?
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> > The "!detachedViews->GetNextSibling()" condition is new here.
> [...]
> It creates some more "interesting" questions though like what to do if one
> of two weak frames are destroyed? Call Hide? Just restore the one frame?

Why can't we just drop that "!detachedViews->GetNextSibling()" check?
If one of these old navigations is destroyed it will destroy its frame
tree and its root view will be unhooked from the detachedFrame's view,
just like it's currently unhooked (as a sibling) from it.
The bug here is just that the (leading) stored detached view might
become stale, right?

Passing detachedFrame->GetView() to InsertViewsInReverseOrder should work
the same as it currently does if that view happens to have siblings.

Or do you have reason to believe the view sibling list is screwed up too?

> > Also, in the !detachedViews->GetFrame() case, isn't there a perf loss
> [...]
> Only way I can think of to avoid that is to re-introduce weak views and
> store weak views. Worth it?

Probably not.  Let's not worry about it for now.
(In reply to Mats Palmgren (:mats) from comment #7)
> (In reply to Timothy Nikkel (:tnikkel) from comment #6)
> > > The "!detachedViews->GetNextSibling()" condition is new here.
> > [...]
> > It creates some more "interesting" questions though like what to do if one
> > of two weak frames are destroyed? Call Hide? Just restore the one frame?
> 
> Why can't we just drop that "!detachedViews->GetNextSibling()" check?
> If one of these old navigations is destroyed it will destroy its frame
> tree and its root view will be unhooked from the detachedFrame's view,
> just like it's currently unhooked (as a sibling) from it.
> The bug here is just that the (leading) stored detached view might
> become stale, right?
> 
> Passing detachedFrame->GetView() to InsertViewsInReverseOrder should work
> the same as it currently does if that view happens to have siblings.

Oh, I see what you are saying now. You are saying to just store the (weak) frame of the leading view, and use that to insert the leading view and any trailing siblings. The problem I see with that is that the leading weak frame can die, but one of the trailing views can still be alive, and we won't have any way to get a pointer to the trailing view to insert back into the tree.

The way I was thinking of allowing more than one view was to store multiple weak frames on the frame loader (in an array or similar). That would avoid that problem.
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> but one of the trailing views can still be alive

Can that actually happen?  Don't we always destroy the mPreviousViewer
chain in that case?  So if the leading view goes away so does all
its siblings.

> and we
> won't have any way to get a pointer to the trailing view to insert back into
> the tree.

If so, is that a bad thing?  Sooner or later these DocumentViewers will be
destroyed and associated frames/views destroyed, no?

> The way I was thinking of allowing more than one view was to store multiple
> weak frames on the frame loader (in an array or similar). That would avoid
> that problem.

Sure, an array of weak frames sounds fine in the long run to make this
independent of the view tree.  I think it's probably fine to just ignore
any null pointers in that case because that's how we currently handle it
(i.e. removed views have unhooked themselves so we won't even notice).
(In reply to Mats Palmgren (:mats) from comment #9)
> > and we
> > won't have any way to get a pointer to the trailing view to insert back into
> > the tree.
> 
> If so, is that a bad thing?  Sooner or later these DocumentViewers will be
> destroyed and associated frames/views destroyed, no?

The trailing view might be the view we actually want to display though. Actually, I think this will work fine. If the first weak frame is destroyed we'll call Hide and then Show will recreate any trailing views that should be there.


I think this is the approach I will take then.
Comment on attachment 8742549 [details] [diff] [review]
patch

Ok, thanks for the explanations.  r=mats with an added MOZ_ASSERT about
the frame being null when oldContainerDoc is, as suggested above.
Attachment #8742549 - Flags: review?(mats) → review+
Attached patch patch v2Splinter Review
Attachment #8742549 - Attachment is obsolete: true
Attached patch interdiffSplinter Review
interdiff from v1 to make sure we are on the same page.
Attachment #8743559 - Flags: review?(mats)
Comment on attachment 8743558 [details] [diff] [review]
patch v2

Either one is fine, but I think I slightly prefer this version since it's close
to what we're currently doing for sibling views so it seems less risky.
Attachment #8743558 - Flags: review+
Attachment #8743559 - Flags: review?(mats) → review+
Comment on attachment 8743558 [details] [diff] [review]
patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
this isn't the easiest code to understand, but if an attacker groks the code they would know what general area to look in. I tried to reproduce this problem and I failed. But who knows what a determined attacker with lots of time and a fuzzer could do.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I'll remove the checkin comment before landing. So the answer is no.

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?
bug 775965 (it's on all supported branches)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
easy to make

How likely is this patch to cause regressions; how much testing does it need?
I think it's low risk
Attachment #8743558 - Flags: sec-approval?
This has sec-approval to go in on May 10, two weeks into the release cycle (We're shipping Firefox 46 next week).

We'll want to get Aurora, Beta, and ESR45 patches, I expect. You may wish to prepare and nominate them now in order for them to go in following this landing on trunk.
Whiteboard: [checkin on 5/10]
Attachment #8743558 - Flags: sec-approval? → sec-approval+
Can we allow some bake time for this before uplifting? That would make me more comfortable.
Sure, we can do that normally. How long were you thinking? I can see a week easily. If you want more, maybe we should move up the checkin (with its associated risks) to May 3.
I think that would be a good idea.
Blocks: 1259572
Blocks: 1261175
Tracking so that we remember to land this around May 10th and uplift it.
May 3 hopefully!
Can I land this?
Flags: needinfo?(abillings)
Yes, land it.
Flags: needinfo?(abillings)
Keywords: checkin-needed
Whiteboard: [checkin on 5/10] → [checkin on 5/3]
the v2 patch has conflicts:
applying 1265577.patch
patching file layout/generic/nsSubDocumentFrame.cpp
Hunk #4 FAILED at 982
1 out of 4 hunks FAILED -- saving rejects to file layout/generic/nsSubDocumentFrame.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1265577.patch

does this need to be checkedin with the interdiff ?
Flags: needinfo?(tnikkel)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6eb3ce095a70
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [checkin on 5/3]
Target Milestone: --- → mozilla49
Group: gfx-core-security → core-security-release
Hi Tim, could you please nominate patches for uplift to Beta47, Aurora48, ESR45? Thanks!
Flags: needinfo?(tnikkel)
Attached patch patch for betaSplinter Review
Aurora should be able to the patch from central. Beta had some conflicts.
Flags: needinfo?(tnikkel)
Attached patch patch for esr45Splinter Review
This touches a lot of the same lines as bug 1261230. Rather than re-write the patch we should just uplift bug 1261230, I think that is the least risky option. I will nominate bug 1261230 for uplift.
Comment on attachment 8743558 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 775965
[User impact if declined]: potentially security crashes
[Describe test coverage new/current, TreeHerder]: not specifically for this problem because I haven't been successful reproducing it.
[Risks and why]: this is the least risky way of fixing this problem.
[String/UUID change made/needed]: none
Attachment #8743558 - Flags: approval-mozilla-aurora?
Comment on attachment 8751039 [details] [diff] [review]
patch for beta

Approval Request Comment
[Feature/regressing bug #]: bug 775965
[User impact if declined]: potentially security crashes
[Describe test coverage new/current, TreeHerder]: not specifically for this problem because I haven't been successful reproducing it.
[Risks and why]: this is the least risky way of fixing this problem.
[String/UUID change made/needed]: none
Attachment #8751039 - Flags: approval-mozilla-beta?
Comment on attachment 8751040 [details] [diff] [review]
patch for esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-crit
Fix Landed on Version: 49
Risk to taking this patch (and alternatives if risky): this is the least risky way of fixing this problem.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8751040 - Flags: approval-mozilla-esr45?
Comment on attachment 8743558 [details] [diff] [review]
patch v2

Sec-crit, Aurora48+
Attachment #8743558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8751039 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8751040 [details] [diff] [review]
patch for esr45

Sec-critical, it will ship with 47, let's get this into esr45.
Attachment #8751040 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+][adv-esr45.2+]
Version: unspecified → 17 Branch
Group: core-security-release
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.