Closed Bug 902350 Opened 7 years ago Closed 7 years ago
Mixed content block navigating from secure frameset to insecure page
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release) Build ID: 20130730113002 Steps to reproduce: Open a secure frameset, e.g., Craigslist Nonprofits forum - https://forums.craigslist.org/?forumID=501 In the top frame, click the link to CL, which is a link to an insecure page with a target="_top" Actual results: Mixed Content block! Note: This behavior was discovered by a user who mentioned it on SuMo. Expected results: Because the link navigates away from the secure frameset (due to the target attribute), the link should not be blocked.
Hmm, I thought I fixed this bug 841850, but maybe that was only when a frame navigation results in opening a new tab. I will take a look.
Looks like I've only covered target="_blank". I'll go through all the different options for target (https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3101) and decide what to do in each case. For _top, I can check of the targetDocShell is equal to the same type root doc shell. If it is, then I can assume we are navigating _top.
Changing the code to fix this bug will also "fix" this bug for all other nsIContentPolicies. Does this have negative implications to CSP that I can't think of? If a user clicks a link in an iframe that is going to navigate the current tab (or open a new tab), would CSP want to block that? I don't think so, because this is in an <a href>. This could be an issue for iframe sandbox or csp-sandbox, but ensuring that top level navigation does not occur for sandboxed frames is probably handled somewhere else (outside of content policies). Adding a couple people just in case I've missed something.
It looks like the sandboxed frames case is handled here (after the call to the content policies) - https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8754
Options for _target: _self: No changes needed. We assume the page or frame is navigating itself. _blank: A new tab is being opened in a new docShell, so mixed content blocker should treat the load as TYPE_DOCUMENT and hence not block it. Taken care of in bug 841850. _parent: It depends. Is the parent the top level document? If it is, then we should treat the load as TYPE_DOCUMENT. If not, treat it as TYPE_SUBDOCUMENT. But consider this scenario: http page A contains an https iframe B. https iframe B contains an https grandchild iframe C. C includes <a href=http:// target=_parent>. In this case, we will navigate B to an http page. Since A is http already, we aren't introducing mixed content by navigating B (which was https) to an http page. This may end up being a tricky implementation. _top: Navigating the top level page, so we should consider this TYPE_DOCUMENT. Need to change the code to handle this case. _content: I'm not sure what this is yet. But a test case shows that mixed content blocker blocks the target=_content: https://people.mozilla.com/~tvyas/navigateframecontent.html _main: I'm not sure what this is yet. But a test case shows that mixed content blocker blocks the target=_main: https://people.mozilla.com/~tvyas/navigateframemain.html The name of a frame: Example: https://people.mozilla.com/~tvyas/navigateframefoo.html. Not sure how the code for this works yet.
This patch checks if we are navigating the top level document, and properly categories those type of loads as TYPE_DOCUMENT. Without that patch, they are incorrectly classified as TYPE_SUBDOCUMENT. These 3 test cases are fixed (i.e. Mixed Content Blocker is not invoked) with this patch: _content: https://people.mozilla.com/~tvyas/navigateframecontent.html _main: https://people.mozilla.com/~tvyas/navigateframemain.html _top: https://people.mozilla.com/~tvyas/navigateframetop.html I need to add mochitests.
Attachment #787240 - Flags: review?(bugs)
Comment on attachment 787240 [details] [diff] [review] Set correct content type for top level navigations v1 mochitests please :)
Attachment #787240 - Flags: review?(bugs) → review+
This tests the target=_top case. It adds on to some existing mochitests, but this method isn't going to work since target=_top overwrites the top level page and hence the testing framework. I will have to add a new tab for this test or move it into a stand alone test. More to come later.
Comment on attachment 791041 [details] [diff] [review] Bug902350-mochitest2-08-15-13.patch remove the //alert But I don't understand this. What is testXFOFrameInChrome? It is in browser_bug593387.js, but that isn't related to this bug.
Attachment #791041 - Flags: review?(bugs) → review-
Sorry Olli! Looks like I uploaded a WIP patch instead of the actual working patch.
Setting tracking flags since we should nominate this patch for uplift to FF 24.
Comment on attachment 791349 [details] [diff] [review] Mochitest ok, but at some point we should add tests for all the special targets.
Attachment #791349 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #14) > Comment on attachment 791349 [details] [diff] [review] > Mochitest > > ok, but at some point we should add tests for all the special targets. Thanks Olli! You are right. Specifically, the behavior of _parent will differ depending on whether the link is embedded in an iframe or a grandchild iframe. That edge case is even more edgey than this one though. I filed a followup bug to investigate target="_parent" and add some test cases for other targets - https://bugzilla.mozilla.org/show_bug.cgi?id=906219
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cc93d3d72306
Comment on attachment 787240 [details] [diff] [review] Set correct content type for top level navigations v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): Mixed Content Blocker (master bug 815321) User impact if declined: Some navigations that should not be blocked by MCB will continue to be blocked without this patch. Testing completed (on m-c, etc.): Currently in try - https://tbpl.mozilla.org/?tree=Try&rev=cc93d3d72306 Risk to taking this patch (and alternatives if risky): The code adds logic to nsDocShell that correctly identifies content as TYPE_DOCUMENT vs TYPE_SUBDOCUMENT (i.e. an iframe). Content that was previously miscategorized will be properly categorized. At worst, content that should be treated as frames would get treated as documents or vice versa. String or IDL/UUID changes made by this patch: None.
(In reply to Tanvi Vyas [:tanvi] from comment #16) > Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cc93d3d72306 Try looks good. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/75a18dd2b73b https://hg.mozilla.org/integration/mozilla-inbound/rev/8329300901f8 If this gets into central and we get the mozilla-beta and mozilla-aurora approvals today, we can uplift before Monday's beta. That might be cutting it a little close, but it's a possibility. Otherwise, it will go into Wednesday's beta, which isn't too far off.
(In reply to Tanvi Vyas [:tanvi] from comment #17) > Comment on attachment 787240 [details] [diff] [review] > Set correct content type for top level navigations v1 > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Mixed Content Blocker (master bug > 815321) > User impact if declined: Some navigations that should not be blocked by MCB > will continue to be blocked without this patch. > Testing completed (on m-c, etc.): Currently in try - > https://tbpl.mozilla.org/?tree=Try&rev=cc93d3d72306 > Risk to taking this patch (and alternatives if risky): The code adds logic > to nsDocShell that correctly identifies content as TYPE_DOCUMENT vs > TYPE_SUBDOCUMENT (i.e. an iframe). Content that was previously > miscategorized will be properly categorized. At worst, content that should > be treated as frames would get treated as documents or vice versa. > String or IDL/UUID changes made by this patch: None. Tanvi, does this already have automated tests ? I've added verifyme for QA to help with verification here..will be great if you can let them know of any test cases that you may have in mind.
(In reply to bhavana bajaj [:bajaj] from comment #20) > > Tanvi, does this already have automated tests ? I've added verifyme for QA > to help with verification here..will be great if you can let them know of > any test cases that you may have in mind. There is a mochitest included in this patch. I also have some websites that can be used by QA as testcases: https://people.mozilla.com/~tvyas/navigateframetop.html https://people.mozilla.com/~tvyas/navigateframemain.html https://people.mozilla.com/~tvyas/navigateframecontent.html https://people.mozilla.com/~tvyas/navigateparent_child.html Without the patch in this bug, the above pages will invoke the Mixed Content Doorhanger in Firefox 23+. With the patch in this bug, they should instead just navigate straight to cisforcookie.org without a Mixed Content Blocker doorhanger. These two examples should invoke the Mixed Content Blocker, with or without the patch in this bug: https://people.mozilla.com/~tvyas/navigateparent_top.html http://people.mozilla.com/~tvyas/navigateparent_top.html
Even if there is a mochitest included in the patch I have manually tested this fix using URL's from Comment 22. For all of them Mixed Content Blocker doorhanger works as expected. Tested on Mac OS 10.7.5 and Windows 7 x64 using FF24b9 BuildID: 20130905180733, Latest Aurora 25 BuildID: 20130908004001 and Latest Nightly 26 BuildID: 20130908030205
You need to log in before you can comment on or make changes to this bug.