Mixed content block navigating from secure frameset to insecure page

VERIFIED FIXED in Firefox 24

Status

()

Core
Security
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jefferson, Assigned: tanvi)

Tracking

(Blocks: 2 bugs)

23 Branch
mozilla24
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24+ verified, firefox25+ verified, firefox26+ verified)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Blocks: 834836
Component: Untriaged → Security
(Assignee)

Comment 1

5 years ago
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.
Blocks: 815321
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 787240 [details] [diff] [review]
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.
Attachment #787240 - Flags: review?(bugs)

Comment 7

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

mochitests please :)
Attachment #787240 - Flags: review?(bugs) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 787262 [details] [diff] [review]
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.
QA Contact: mwobensmith
(Assignee)

Comment 9

5 years ago
Created attachment 791041 [details] [diff] [review]
Bug902350-mochitest2-08-15-13.patch

Mochitest
Assignee: nobody → tanvi
Attachment #787262 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #791041 - Flags: review?(bugs)

Updated

5 years ago
Duplicate of this bug: 905666
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-
Component: Security → Security
Product: Firefox → Core
(Assignee)

Comment 12

5 years ago
Created attachment 791349 [details] [diff] [review]
Mochitest

Sorry Olli!  Looks like I uploaded a WIP patch instead of the actual working patch.
Attachment #791041 - Attachment is obsolete: true
Attachment #791349 - Flags: review?(bugs)
(Assignee)

Comment 13

5 years ago
Setting tracking flags since we should nominate this patch for uplift to FF 24.
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
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+
(Assignee)

Updated

5 years ago
Blocks: 906219
(Assignee)

Comment 15

5 years ago
(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
(Assignee)

Comment 17

5 years ago
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.
Attachment #787240 - Flags: approval-mozilla-beta?
Attachment #787240 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

5 years ago
(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.

Updated

5 years ago
tracking-firefox24: ? → +
tracking-firefox25: ? → +
tracking-firefox26: ? → +
https://hg.mozilla.org/mozilla-central/rev/75a18dd2b73b
https://hg.mozilla.org/mozilla-central/rev/8329300901f8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox26: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

5 years ago
Attachment #787240 - Flags: approval-mozilla-beta?
Attachment #787240 - Flags: approval-mozilla-beta+
Attachment #787240 - Flags: approval-mozilla-aurora?
Attachment #787240 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Keywords: verifyme
(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.
(Assignee)

Comment 22

5 years ago
(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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 908571
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
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox26: fixed → verified
Keywords: verifyme
(Assignee)

Updated

5 years ago
Target Milestone: mozilla26 → mozilla24
You need to log in before you can comment on or make changes to this bug.