314549 - Various bugs involving containers not actually fixed for subframes

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It looks like we never unset document and prescontext containers, prescontext link handlers, etc for subframes when their parent goes into bfcache.  This means that bug 292960 and possibly bug 292961 are not fixed for subframes.

While this is not quite as bad as the original bug 292960 and bug 292961, it still has the basic problem that code (extensions, content code, session history code, web pages, etc) doesn't expect that sort of thing to be happening, so allowing it to happen rather scares me security-wise.  Once the page a frame is in has been unloaded, there should be no more traversals in that frame.

To fix this, I think we need to walk the docshell tree like we do the presshell tree when putting a page in bfache and unset the containers and link handler on everything in that subtree.  Restoring needs to restore the pointers.  It should be pretty safe to do this, I would think.

Comment 1

[Andreas Goetz]

This impacts bug 314288, which effectively renders FastFind useless- imho critical.

Thanks,
Andi

Comment 2

[Scott MacGregor]

We're not going to be able to get to this for the 1.5 release. After discussing this with bryner, we'd like to pursue this for the next follow on release so we can address the remaining issues with subframes whose parent goes into bfcache.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we don't fix this in 1.5 release, we have major bug(bug 314288).
That has low risk workaround patch. We should use it!!

Comment 4

[Boris Zbarsky [:bzbarsky]]

For what it's worth, and this is the last time I'm going to worry about any bugs related to any of this, I think it would be worth considering preffing bfcache off by default and preffing it back on at the same time as we fix this bug, assuming we fix it in branch.  I do think this is that serious.

Then again, given the amount of hoopla we've generated about bfcache in the last few months, I can see how it would be rather politically inexpedient to ship 1.5 without it.  So I rather doubt the above course of action will be adopted.

Comment 5

[Scott MacGregor]

There seems to be a disconnect as too how serious this issue is. Bryner and others don't think this is that bad and is best addressed in the follow up release. It would have to be a  pretty serious show stopper for us to slip the release for this and I'm not seeing enough evidence that we should based on the feedback we've gotten.

Comment 6

[Hixie (not reading bugmail)]

Clearly I'm missing some context here, I assume stuff happened in private e-mail or something. Is this a security bug or not? bz's comments regarding the seriousness of this bug seem way out of alignment with the description.

What are the steps to reproduce?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Quote from a mail I sent earlier today (and really my last advocacy-like commment on this bug):

If anything demonstrates a problem with this code, it'll be a security exploit, not a site misbehaving.

And I can't even state categorically that there _is_ a way to exploit it, without doing a lot more legwork than I really have time or desire for.  All I can say for sure is that we're now allowing certain actions on the part of script where we've never allowed them before.  And that scares me.  But maybe that's just me being paranoid.

Comment 8

[Brian Ryner (not reading)]

Attachment: fix

This should take care of the problem, I think.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

>Index: layout/base/nsDocumentViewer.cpp
>@@ -1396,6 +1483,7 @@ DocumentViewerImpl::Destroy()
>+    nsISHEntry *shEntry = mSHEntry; // save a weak reference, see below

I don't really see what ensures that the shEntry stays alive. Please make that an nsCOMPtr, ok?

r=bzbarsky with that change.  Thanks for fixing this, Brian!

Comment 10

[Brian Ryner (not reading)]

checked in on the trunk.

Comment 11

[Brian Ryner (not reading)]

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

I think we can at least evaluate this for branch landing.

Comment 12

[Asa Dotzler [:asa]]

we've already got final bits in and are running our tests now. this has just landed on the trunk and not yet been verified there. can one of you tell us what the specific impact of this fix is and what kind of risk it poses? 

Comment 13

[Brian Ryner (not reading)]

I think Boris has already stated the impact/risk of this bug to the extent that we know.

My feeling is that exploiting this as a security bug would be difficult.  As long as we leave bfcache disabled for subframe navigations, I can't see a way that page content could retain a reference to one of these subframes that's in bfcache but isn't completely unhooked.  That leaves us with the possibility of built-in or privileged extension code getting confused.  We don't touch content viewers in session history except to evict or restore them, so I'd consider that fairly unlikely as well... a malicious extension could do something here, but that's nothing new.

Unless anyone sees other avenues to explot this, I'd be inclined to let this bake on the trunk and pull it into the first 1.5.x bugfix release.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> As long as we leave bfcache disabled for subframe navigations

That doesn't affect this bug, since this bug pops up when we bfcache a toplevel page that has subframes.

Content could trivially retain a reference to one of these subframes -- just take a reference to a subframe and then load something in the toplevel page.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Impact of this fix:

When a page which has subframes goes into bfcache, in addition to setting pointers to the docshell to null on the toplevel page we also do so on all subframes.  When we restore from bfcache, we go through and set all the pointers correctly again.

There is no impact on pages that have no subframes.

For pages with subframes the change should be very safe.  It makes subframes of a page in bfcache pretty much identical in terms of behavior to the page itself; without this patch they behave a lot more like subframes of a page that is _not_ in bfcache.  So the risk is minimal.

I agree with bryner that there is no obvious exploit (at least I haven't managed to come up with one yet, though there was the idea of one that I mentioned in some email recently), so if we're really far too late in the game, I suppose I'd be ok with shipping this in a security release (1.8.0.x, not just 1.8.x) down the road.  This doesn't change any APIs, so that should be ok...

Comment 16

[Brian Ryner (not reading)]

(In reply to comment #14)
> > As long as we leave bfcache disabled for subframe navigations
> 
> That doesn't affect this bug, since this bug pops up when we bfcache a toplevel
> page that has subframes.
> 
> Content could trivially retain a reference to one of these subframes -- just
> take a reference to a subframe and then load something in the toplevel page.
> 

And then the script retaining the reference is either unloaded or put into bfcache, which in neither case would allow it to do anything with the reference.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> And then the script retaining the reference is either unloaded or put into
> bfcache

Not if it's in a different window altogether...  As in, window.open a framed page, take a ref to a frame in that page, load something new in that window.

Comment 18

[Scott MacGregor]

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

unfortunately it's too late for the 1.5 train. Lets focus on getting this into the follow up release.

Comment 19

[Brian Ryner (not reading)]

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

Would like to get this in for the 1.8 branch.

Comment 20

[Daniel Veditz [:dveditz]]

Should this also go on the 1.8.0 branch?

Comment 21

[Boris Zbarsky [:bzbarsky]]

This patch involves an API change, so for the 1.8.0 branch it'd need to be done differently, somehow.  :(

Even for the 1.8.1 branch it might be worth looking for a way without the API change, though there it may be less of a problem.

But yes, in general I think this is worth fixing on the 1.8.0 branch.

Comment 22

[Benjamin Smedberg]

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

Can't take this as-is because of interface changes.

Comment 23

[Daniel Veditz [:dveditz]]

It's not looking like this would make 1.8.0.2, feel free to renominate if you can make the case and have a branch-safe patch.

Comment 24

[Darin Fisher]

We're interested in this for the 1.8 branch, but we'd need a version of the patch that does not break APIs.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Based on dveditz's willingness to take this as a security and stability update, it sounds like the sort of thing that we can take in beta2, but we'll need that API safe patch!

Comment 26

[Brian Ryner (not reading)]

Attachment: branch-safe fix

We could do it this way... it's not really ideal since the old method stops working, but there's no way to implement it correctly without that SHEntry parameter

Comment 27

[Brian Ryner (not reading)]

branch patch checked in

Comment 28

[[e-mail]]

The branch-"safe" fix caused SeaMonkey to go yellow on Linux and Windows:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.8-SeaMonkey
which I verified by backing out the patch locally on one build as starred.

Comment 29

[[e-mail]]

Comment on attachment 228327 [details] [diff] [review]
branch-safe fix

>+    nsISHEntry *shEntry = mSHEntry; // save a weak reference, see below

>+    while (NS_SUCCEEDED(shEntry->ChildShellAt(itemIndex++,
>+                                              getter_AddRefs(item))) && item) {
My debug build dies here because shEntry is stale.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Hmm... Why _is_ that a weak ref?  That seems wrong, since we immediately do a release on the shentry, so it could well go away.  The "see below" is not helpful, since nowhere do we explain why that needs to be a weak ref.

Comment 31

[Bob Clary [:bc] (inactive)]

*** Bug 345151 has been marked as a duplicate of this bug. ***

Comment 32

[Bob Clary [:bc] (inactive)]

This crash blocks any of the automated testing of the js test library and related tests. Please fix as soon as possible or back it out. Thanks.

Comment 33

[Bob Clary [:bc] (inactive)]

(In reply to comment #32)
> This crash blocks any of the automated testing of the js test library and
> related tests. Please fix as soon as possible or back it out. Thanks.

To clarify, the crash I am seeing when attempting to run tests is on the 1.8 branch due to the checkin in comment 27. 

Comment 34

[Brian Ryner (not reading)]

Hm, is the crash branch-only, or on trunk too? (the code is the same) And why doesn't it affect Firefox?

Comment 35

[Bob Clary [:bc] (inactive)]

(In reply to comment #34)
> Hm, is the crash branch-only, or on trunk too? (the code is the same) And why
> doesn't it affect Firefox?

1.8 branch only, windows and linux but not Mac OS X ppc or intel. I don't know why it doesn't affect Firefox by itself. The steps to reproduce are in bug 345151 which I've cc'd you on.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Given the way we seem to crash, it could depend on GC timing amongst other things...

Comment 37

[Brian Ryner (not reading)]

Attachment: crash fix

Well that's what I get for merging the patch I posted, rather than the one I checked in (which has this as a strong ref, based on Boris's review comment about this exact situation).  Doh.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 229880 [details] [diff] [review]
crash fix

That should be an nsCOMPtr, right?

Comment 39

[Brian Ryner (not reading)]

Attachment: crash fix

yes.

Comment 40

[Bob Clary [:bc] (inactive)]

(In reply to comment #39)

appears to have fixed my crash on windows xp at least.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 229901 [details] [diff] [review]
crash fix

r+sr=bzbarsky

Comment 42

[Boris Zbarsky [:bzbarsky]]

*** Bug 345226 has been marked as a duplicate of this bug. ***

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This allegedly caused bug 345083.  Does the "crash fix" patch fix that too?

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 229901 [details] [diff] [review]
crash fix

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.

Comment 45

[Brian Ryner (not reading)]

crash fix checked in on branch, was already fine on the trunk

Comment 46

[Bob Clary [:bc] (inactive)]

*** Bug 345306 has been marked as a duplicate of this bug. ***

Comment 47

[aja+bugzilla]

(In reply to comment #43)
> This allegedly caused bug 345083.  Does the "crash fix" patch fix that too?
> 
The "crash fix" has indeed fixed bug 345083.