902350 - Mixed content block navigating from secure frameset to insecure page

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Jefferson]

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.

Comment 1

[Tanvi Vyas[:tanvi]]

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.

Comment 2

[Tanvi Vyas[:tanvi]]

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.

Comment 3

[Tanvi Vyas[:tanvi]]

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.

Comment 4

[Tanvi Vyas[:tanvi]]

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

Comment 5

[Tanvi Vyas[:tanvi]]

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.

Comment 6

[Tanvi Vyas[:tanvi]]

Attachment: Set correct content type for top level navigations v1

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 787240 [details] [diff] [review]
Set correct content type for top level navigations v1

mochitests please :)

Comment 8

[Tanvi Vyas[:tanvi]]

Attachment: WIP Mochitest

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 9

[Tanvi Vyas[:tanvi]]

Attachment: Bug902350-mochitest2-08-15-13.patch

Mochitest

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

Sorry Olli!  Looks like I uploaded a WIP patch instead of the actual working patch.

Comment 13

[Tanvi Vyas[:tanvi]]

Setting tracking flags since we should nominate this patch for uplift to FF 24.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 791349 [details] [diff] [review]
Mochitest

ok, but at some point we should add tests for all the special targets.

Comment 15

[Tanvi Vyas[:tanvi]]

(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

Comment 16

[Tanvi Vyas[:tanvi]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cc93d3d72306

Comment 17

[Tanvi Vyas[:tanvi]]

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.

Comment 18

[Tanvi Vyas[:tanvi]]

(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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75a18dd2b73b
https://hg.mozilla.org/mozilla-central/rev/8329300901f8

Comment 20

[bhavana bajaj [:bajaj]]

(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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62f4f576fe9b
https://hg.mozilla.org/releases/mozilla-aurora/rev/660eabee7811

https://hg.mozilla.org/releases/mozilla-beta/rev/8959a853c1f0
https://hg.mozilla.org/releases/mozilla-beta/rev/209b28130fd1

Comment 22

[Tanvi Vyas[:tanvi]]

(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

Comment 24

[Mihai Morar, (:MihaiMorar)]

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