User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11 Many security-sensitive pages, such as login pages, contain inline frames (iframes). For example, the password-entry field on Google AdSense, Hushmail, and several bank web sites are contained in iframes. These frames appear to be part of the parent page and do not have address bars (or any kind of security indicator). Because the user has no visible indication of the source of the content that appears in the iframe, the user implicitly trusts the parent page to fill the iframe with trustworthy content. Protecting the integrity of the frame's contents is critical to the security of these sites. Firefox has a more permissive frame navigation policy than IE and Safari, allowing an active frame to navigate a target frame if (1) the target frame is a top-level frame or (2) the active frame can script any frame in the same window as the target frame. This policy permits frame hijacking attacks: 1. Sites such as iGoogle or Live.com contain a number of iframe "gadgets." A malicious gadget can navigate all the other gadgets in the same window, tricking a user who is expecting to interact with some trusted gadgets (such as a Chat or Email gadget). 2. Sites commonly display third-party advertisements in iframes. A malicious advertisement can navigate the other ad frames to steal their advertising impression and display advertisements of the attacker's choice. Firefox should implement the same policy as IE and Safari: An active frame can navigate a target frame if (1) the target frame is a top-level frame or (2) the active frame can script the target frame or any of its ancestors in the frame hierarchy. A full discussion of frame navigation policies in Firefox, IE, Safari, and Opera is available at <http://crypto.stanford.edu/frames/>. Reproducible: Always Steps to Reproduce: 1. Go to <http://www.google.com/ig>. (Logging in is optional.) 2. Add some non-inline gadgets; for example a Hotmail gadget. (There is currently already non-inline YouTube gadget on the default iGoogle page.) 3. Add the attacker's gadget by visiting <http://www.google.com/ig/adde?synd=open&source=ggyp&moduleurl=http://crypto.stanford.edu/frames/igoogle/attacker.xml> 4. Click the "Navigate my gadgets!" button. Actual Results: The contents of non-inline gadgets are replaced with content of the attacker's choice (in this case Yahoo!'s homepage). Expected Results: The attacker is only able to "hijack" his own gadget. This is the behavior of IE7 and the latest WebKit nightly builds.

Here is a patch that implements the Ancestor frame navigation policy, matching IE7 and Safari.

http://crypto.stanford.edu/frames/ is very detailed and helpful. Thanks! There are two major methods for navigating other frames: named targeting and direct navigation. I'm not sure we currently use the same policy for both. Your page refers to bug 13871 and bug 103638, which were primarily about named targeting, while your testcase uses direct navigation. IIRC, the current behavior for direct navigation is mostly the result of patches in bug 52920 and bug 246448. We adopted the permissive policy for direct navigation in bug 52920 for web compatibility reasons, despite my security concerns (bug 52920 comment 6). But IE7 adopted a more restricted policy and took the brunt of the web-compatibility hit (go Microsoft!), so we should be able to do the same now.

We put together a test case of frame navigation within a single window using window.open(). The test case is intended to cover the essential domain and parent/child combinations that can occur within a single page. Our test case can be found at <http://w3sim.com/frames/>. The attached screenshot shows the behavior of Firefox 2.0.0.11 on the test case.

This screenshot shows IE7's behavior on our test case. The red text indicates frame navigations that failed, creating a popup instead of navigating the targeted iframe.

The WebKit component of Safari has recently adopted the same frame navigation policy as IE7, as shown in this screenshot of a recent nightly build.

Our patch updates Firefox to match the behavior of IE7 and Safari on our test case, as shown in the attached screenshot.

> Firefox has a more permissive frame navigation policy than IE and Safari, > allowing an active frame to navigate a target frame if (1) the target frame is > a top-level frame or (2) the active frame can script any frame in the same > window as the target frame. By "frame navigation" do you mean named window targeting? Or something else? That's the first place to start. Different policies are applied to named windows and to other navigation methods.

IE7 uses the same policy for window.open, hyperlinks with targets, and setting the location property: Test case for hyperlinks with targets: http://xenon.stanford.edu/~abarth/research/nav/frame1.html Test case for window.open: http://xenon.stanford.edu/~abarth/research/nav-open/frame1.html Test case for setting location property: http://xenon.stanford.edu/~abarth/research/nav-pointer/frame1.html Although Firefox uses different policies for different types of navigation, it makes sense to enforce a consistent security policy across all of them. As Jesse pointed out, the patch does not yet address navigation via setting the location property, but we think that doing so would be a good idea.

It's possible that we'll want targeted loads to be "stricter" than setting the location property, not for security reasons but to match the HTML5 spec or expected web site behavior.

We have brought up the issue in the HTML5 working group and it seems likely that if Firefox synchronizes its policy with the IE7 and Safari policy, the HTML5 spec will adopt this policy as well.

The policy for targeted loads is somewhat "looser" in that you can target windows that are not reachable though JS (e.g. windows opened via window.open() that you then didn't hold a reference to). > IE7 uses the same policy for window.open, hyperlinks with targets, and setting > the location property: Is this documented somewhere (same question as what I asked in comment ? Or are the testcases cited in comment 8 exhaustive? Basically, we need to have a clear spec for this stuff before we start changing it; bit-by-bit changes will just cause pain for testers and web developers and lead to confusing code. I see no reason to treat window.location and named targets identically (since per above they are not in fact identical, and since in practice the use cases are quite different). If we're just talking named targets and there is a proposed patch, could someone attach a readable (-up12 is good) version of it?

The rules for window.open() and hyperlinks with targets in HTML5 attempt to resolve the security issues raised in this bug: http://www.whatwg.org/specs/web-apps/current-work/#the-rules I would be interested in knowing what should change in those rules if they are not adequate (i.e. do you think you could implement them? If not, what should I change so that you would be willing to implement them?).

The policy described in the HTML5 spec is the Parent policy described in our white paper: <http://crypto.stanford.edu/frames/> From a security standpoint, the Parent policy is fine with us, because it is more restrictive than Ancestor. We consider the Ancestor policy to be more convenient for two reasons: 1) Ancestor already has adoption in two browsers. Any web sites that are not compatible with Ancestor have already had a year to fix their sites for IE7. Opera attempts to implement Parent, but does not do so correctly for window.open(). Because no major browser implements Parent according to the HTML5 spec, it is not known whether the Parent would break functionality in a large number of legitimate sites. 2) Ancestor permits more functionality while admitting no known attacks. For example, Ancestor permits more flexible fragment identifier messaging.

> I see no reason to treat window.location and named targets identically Currently, nsDocShell::CheckLoadingPermissions (which checks window.location) and nsDocShell::CanAccessItem (which checks named targets) enforce the same navigation policy, except that nsDocShell::CanAccessItem is restricts by opener. However, the opener restriction is easily circumvented unless the web site takes preventative measures that only work in Firefox. The attached patch changes the security checks for both nsDocShell::CheckLoadingPermissions and nsDocShell::CanAccessItem to enforce the Ancestor policy.

The "Ancestor policy" is vulnerable to the attack vector I described in my e-mail reply to your e-mail: if a site acts differently based on what page (from its own site) is loaded in an iframe in its page, then it would be bad if an evil site could load the whole thing in an iframe itself and then manipulate the inner iframe to trick the site into doing something different. I really see no good reason to allow a third-party domain to affect iframes that it neither created nor was loaded in.

It's clear that either Ancestor or Parent are better than the current (Window) policy. Both the Ancestor and Parent policies prevent real attacks on real sites that are currently allowed by the Window policy, e.g. iGoogle gadget hijacking. Both the Ancestor and Parent policies allow a parent frame to overwrite some or all of the child frame's screen area with malicious content -- these attacks are trivial under either policy, but the child can always detect whether it's in an iframe by checking if top === self and framebusting if necessary. Advantages of Parent policy: Stricter, simpler, provides more isolation to frames that don't trust their parent but aren't willing to framebust. Advantages of Ancestor policy: Because a frame's ancestor can overwrite the frame's screen real estate (under both Parent and Ancestor policies), the Ancestor policy is more consistent with the "don't prohibit what you can't prevent" philosophy, avoiding a false sense of security among developers who build iframes that don't trust the parent page. It may be more compatible with existing web site designs since the stricter Parent policy has never been deployed on a large scale. Either policy seems secure enough, but we should be sure to choose something that all browser vendors can agree on so we don't have settle for the worst of both worlds.

+'ing this and making it a P2. Let's decide if we should take this.

Apple deployed the Ancestor policy today in Safari as part of Security Update 2007-009 <http://docs.info.apple.com/article.html?artnum=307179>.

We added a mochitest that checks the browser's frame navigation policy for three navigation methods: setting window.location, using window.open, and submitting a form that targets a named frame. We don't expect any major compatibility issues because this patch changes Firefox to match IE7, but we still think it's a good idea to give it some time to bake in the nightly just in case.

There was a bug in the earlier mochitest that I've now corrected. If the first navigation attempt results in an exception being thrown, the exception will now be caught and the remaining attacks are also tested.

That patch regresses one of the bugs that got fixed in this code at some point. In particular, targeting all toplevel windows is not acceptable. Sadly, we haven't gotten all the tests for the various bugs we fixed here into mochitest yet; we really need to. In fact, I'd call getting regression tests for all the bugs this code has historically fixed in a prerequisite for changing the code; we really don't want to regress those bugs.

Though I guess Adam mentioned this in comment 14 when he said "However, the opener restriction is easily circumvented unless the web site takes preventative measures that only work in Firefox." I'm not sure I follow that. The opener restriction is so that you can't target random windows opened by other sites (e.g. banks' login popups). Now that we always show the URL bar it's somewhat mitigated, but we should still prevent this behavior. There is no special effort on the bank's part that's needed here.

We removed the opener restriction on purpose because an active frame can bypass this restriction and navigate any top-level frame in the same unit of related browsing contexts: 1. An active frame can invade the root frame of a related browsing context by 1. dereferencing window.top to get a reference to a top-level frame, 2. dereferencing the opener property of that frame and its openers until it reaches the root, and 3. setting the root's location property. 2. The root frame of the browsing context can navigate any open windows by recursively using window.open to navigate windows to its own origin. (Using history.go(-1), any of these windows can be returned to their previous state.) Sites can sever ties with related browsing contexts by opening only anonymous windows and by setting their window.opener to null. However, these countermeasures are not widely used today and may impair legitimate web functionality. We think it make sense to remove the opener restriction (matching IE7 and Safari), but we think it's more important to tighten down frame navigation within a window, allowing people to build gadget aggregators like iGoogle.

> in the same unit of related browsing contexts This is important. Your change allows a frame to also target unrelated browsing contexts. Specifically, with your change if I as a user open a new window from the file menu, then click on a bookmark link to load my bank site in the window, then click a link on the resulting page that brings up a login window, then any other window I ever opened can override the contents of that login window. That's not acceptable.

> 2. dereferencing the opener property of that frame and its openers until it > reaches the root, and Oh, and as far as this goes, you can only do that if you're same-origin with everything you traverse as you do this. I agree that it's important to protect against frame navigation abuses within a window, but I don't think doing so requires removing the cross-window protection that's already in place.

> Oh, and as far as this goes, you can only do that if you're same-origin with > everything you traverse as you do this. The opener property isn't constrained by the same-origin policy, so it's possible to traverse the opener hierarchy (e.g. opener.opener) without being in the same origin as another page. You can also temporarily coerce the page into your own origin by setting its location and then navigating it back to its old one with history.go(-1). Here's an demonstration of the opener circumvention attack technique that Adam described: http://crypto.stanford.edu/~collinj/research/firefox/opener/ I've attached the source code.

> The opener property isn't constrained by the same-origin policy Ah, indeed. Perhaps that needs to change, or perhaps we the interaction of opener and origin changes needs rethinking (in a separate bug). In any case, this is minor compared to the unrelated browsing context issue.

Here is a version of the patch that leaves the opener restriction in place. It is true that the opener restriction may have some use as a defense in depth measure, although I do worry that it gives a false sense of security. However, our most important concern should fixing the vulnerabilities in frame navigation within a single window, so let's focus on the main issue and we can postpone discussion of the opener restriction for a future bug.

Just to make sure we're on the same page, which bugs' testcases have you tested with this patch to make sure you're not regressing them?

Comment on attachment 295997 [details] [diff] [review] Updated patch that leaves opener restriction intact The bugs we've seen that have test cases available are bug 13871, bug 20682, bug 246448 (including the Secunia advisory), and bug 273699. We can convert these into mochitests to help identify regressions. Bug 246923 don't have any attached test cases but we'll try to cover the issues discussed there. Bug 103638 focuses on the frame search process rather than the frame navigation policy. I'll clear the review flag while we work on this.

I think bug 103638 is what I was thinking of with the opener thing. The frame search process aand navigation policy are one and the same: if you can find it, you can navigate it. If you're willing to write tests for those, that would be absolutely marvelous. If you are also willing to do the various regressions those changes caused (bug 270414, bug 278143, bug 278916 [might be hard to test in Firefox], bug 279495, etc) that would be even lovelier, but I can understand if you have finite amounts of time on your hands. ;) Please do make sure you don't regress those, though.

Adam and I developed a set of mochitests for identifying frame navigation regression issues. test_bug13871.html - tests classic frame hijacking: bug 13871, bug 20682, and bug 273699 test_bug270414.html - tests bug 270414 test_bug278916.html - tests bug 278916 test_bug278143.html - tests bug 278143, bug 279495 test_child.html - tests basic functionality of navigating child iframes test_grandchild.html - distinguishes between ancestor and parent policy test_not-opener.html - tests opener restriction test_opener.html - tests basic functionality of navigating popup windows test_reserved.html - tests frame hijacking of parents, reserved names, parts of bug 246448 test_sibling-matching-parent.html - tests parts of bug 246448 test_sibling-off-domain.html - tests for frame hijacking of siblings, parts of bug 246448 For each arrangement of frames, we test navigation using window.location, window.open, form submission, and targeted hyperlinks. In the process of developing test_reserved.html we found a bug in our original patch and fixed it. The current build fails test_sibling-off-domain.html and half of test_reserved.html -- these are the attacks that our patch fixes.

Cleaned up some unused code in the mochitests.

Cleaned up the mochitest code a bit more, and fixed a Makefile bug.

Comment on attachment 296463 [details] [diff] [review] Updated patch with mochitests Jesse, are you available to review this?

For what it's worth, the changes will need review from one of bsmebdberg, jst, or me (or darin, but he's not really active) per <http://www.mozilla.org/owners.html>. This is apart from the security review, which should be dveditz or Jesse. I do have this on my list of things to review, but likely won't get to it until next weekend.

Comment on attachment 296463 [details] [diff] [review] Updated patch with mochitests jst, dveditz said that you'll be a better reviewer, here

Comment on attachment 296463 [details] [diff] [review] Updated patch with mochitests Boris, any chance you could glance over the docshell changes here too? They're not big changes (the majority of the patch is tests), so shouldn't take you long...

The tests are the part that needs review (and comparison to the old bugs in this code)... I'm still hoping to get to this before the freeze.

Comment on attachment 296463 [details] [diff] [review] Updated patch with mochitests >Index: docshell/base/nsDocShell.cpp >+ // 2) aTargetItem is a top-level frame and we are its descendant or can >+ // access its opener. "we" should be "aAccessingItem", right? Note that this doesn't quite describe what you implemented. What's implemented is more like: 2) aTargetItem is a top-level frame and we are its descendant 3) aTargetItem is a top-level frame and we can target its opener per rule (1) or (2). I guess we can do either one, but I would be fine with the more restrictive version described in your comment, I think. In particular, that would prevent off-site frames targeting on-site popups opened from toplevel, which is something I do think we want to prevent (and test for). >- if (ValidateOrigin(aAccessingItem, target)) { >+ if (ValidateOrigin(aAccessingItem, target)) > return PR_TRUE; >- } Please leave the curlies there? Similar for the other one-line if bodies in this patch. >Index: docshell/test/Makefile.in > DIRS += chrome \ > browser \ >+ navigation \ Why not just put the new files directly in docshell/test? It probably doesn't matter all that much, but just wondering... >Index: docshell/test/navigation/NavigationUtils.js >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation Just making sure that this is correct... if you guys just wrote this on your own without the Foundation paying you for it, chances are you're the Initial Developers. Again, not a big deal either way, just making sure. >+function navigateByLocation(wnd) { >+ window.open(target_url, "_blank"); Could you toss on a "width=10,height=10" feature string on there? That helps with running the tests when the default for new DOMWindows is new window, not new tabs (which is something we do need to test every so often). Similar for the other window.open calls in the tests. >+function isNavigated(wnd, message) { >+ } catch(ex) { >+ result = ex; I assume that you want to press on here on purpose (e.g. because you want to clean up the windows) instead of just relying on the exception to trigger a mochitest failure? Similar in isBlank, isAccessible, isInaccessible. >+function xpcGetFramesByName(name) { >+ var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"] >+ .getService(Components.interfaces.nsIWindowMediator); Is there a reason to not use nsIWindowWatcher's window enumerator here instead? That would be a little more robust, I think, esp. in terms of not relying on the exact structure of the Firefox chrome. Or did you try it and it failed for some reason? This applies to all the xpc* functions. This is not recursing into subframes on purpose, I assume? Please document that (and perhaps the reason why it doesn't descend if there is a short explanation past "that's what the test expects"). >Index: docshell/test/navigation/test_bug270414.html It's good to have this test (and we should keep it), but this doesn't test bug 270414. To test that bug, the window in which the navigation originates needs to be generated completely via document.write(). >Index: docshell/test/navigation/test_bug278143.html This looks like it tests bug 279495, but not so much bug 278143. Testing bug 278143 might be kind of tough now that I look at it, and shouldn't be affected much by this change, so we can just let it be. Just rename this test so it's clear what it's doing? >+function countAndClose(name, expected_count) { >+ is(array_of_frames.length, expected_count, "Should only open " + expected_count + " window using a fancy hyperlink."); Maybe ... "window with name " + name + " using .... ? >Index: docshell/test/navigation/test_bug278916.html This test will definitely need to use windowwatcher to be meaningful, since bug 278916 only showed up in Gecko embeddings that don't have a window mediator... This looks really really good in general. Thank you very much for writing up the tests; they made it a lot easier to both review the patch and convince myself that the patch is correct! If you want to land the tests without doing the window mediator to window watcher switch and do that switch as a followup bug, that would be fine by me. That's not an issue that should hold up landing. Same for the bug270414 issue. I'd appreciate resolving the other test comments before checkin; they're pretty minor. And most importantly, let's sort out the (2) thing from nsDocShell.cpp?

> "we" should be "aAccessingItem", right? Yep. Fixed. > Note that this doesn't quite describe what you implemented. What's > implemented is more like: > > 2) aTargetItem is a top-level frame and we are its descendant > 3) aTargetItem is a top-level frame and we can target its opener > per rule (1) or (2). Yep. This comment was sloppily written. Fixed. > Please leave the curlies there? Fixed for the C++ code. > Why not just put the new files directly in docshell/test? It probably > doesn't matter all that much, but just wondering... We left these in their own directory. The tests have a bunch of auxiliary files that only seem useful in navigation tests. > chances are you're the Initial Developers. Ok. Updated to say "Stanford University" since it writes our paychecks. > Could you toss on a "width=10,height=10" feature string on there? Done. Not all the windows are opened via window.open, but the ones that are now all open in small popup windows. > I assume that you want to press on here on purpose Yes. We don't want these APIs to throw exceptions. In fact, some times the exceptional behavior is correct (e.g. in isInaccessible). > Is there a reason to not use nsIWindowWatcher's window enumerator > here instead? Changed to using nsIWindowWatcher's enumerator, but that seems to still give us back a chrome window (so we have to reach inside to find the content window). The getWindowByName isn't very helpful because we need to enumerate multiple frames with the same name, for example when a navigation fails and spawns a new window. > This is not recursing into subframes on purpose, I assume? Yes, we're just looking for top-level frames here. We could recurse here, but it doesn't affect the test. > Please document that Fixed. > It's good to have this test (and we should keep it), but this doesn't test > bug 270414. To test that bug, the window in which the navigation originates > needs to be generated completely via document.write(). Fixed. The old test has been renamed test_popup_navigates_children.html > This looks like it tests bug 279495, but not so much bug 278143. > [...] > Just rename this test so it's clear what it's doing? Done. > Maybe ... "window with name " + name + " using .... ? Done. > This test will definitely need to use windowwatcher to be meaningful, > since bug 278916 only showed up in Gecko embeddings that don't have a > window mediator... Ok. We're using window watcher now. > If you want to land the tests without doing the window mediator to window > watcher switch and do that switch as a followup bug, that would be fine by > me. That's not an issue that should hold up landing. Same for the > bug270414 issue. I'd appreciate resolving the other test comments before > checkin; they're pretty minor. We've tried to resolve all the issues you brought up. I'm not sure we're using WindowWatcher in the way you intend (we still seem to be relying on the structure of chrome windows), but maybe that's an issue to address in a separate patch. > And most importantly, let's sort out the (2) thing from nsDocShell.cpp? We left this as coded to most-closely match current Firefox behavior.

Comment on attachment 299478 [details] [diff] [review] Updated patch addressing reviewer comments > Changed to using nsIWindowWatcher's enumerator, but that seems to still give > us back a chrome window Ugh. I forgot just how messed-up this stuff is... So these tests still won't work in embedding builds, I guess, unless we add a branch for the case when win.gBrowser is undefined (in which case, we want to look at win.location instead of digging for tabs). Perhaps we should do that, in fact: if (win.gBrowser) { // The code you have now } else if (win.location.protocol == "data:") { // Embedding case win.close(); } with similar changes to the other window watcher consumers here. That should probably be a separate patch, since I'd like to get this change in soon so we can get a bit of bake before the beta. r=bzbarsky. Thanks again for doing this!

I've checked the patch in on trunk. Adam, can you get a followup filed for the minor windowwatcher tweak? As written, the patch caused build bustage; to fix that I removed the "XPCSHELL_TESTS = unit" line from the makefile in docshell/test/navigation This patch also caused a test failure for test_bug326337.html. I fixed that by moving the test code around so that the location is being set on a toplevel window, not a subframe of a toplevel window.

I filed bug 414303 regarding the nsIWindowWatcher issue.

It looks like this caused bug 418559.

Even though this is sort of a web-compat change it seems like it would still be good to land on the 1.8 branch so we get consistent behavior. There are currently significant numbers of browsers in common use with this behavior (IE7) and as FF3 releases will soon be tons more.

Given the risk of site regressions we'd like to wait for the Firefox 3 release to see how this goes over before changing the branch behavior.