906219 - Mixed content blocker navigation via target=

Status: NEW

(Core :: DOM: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

In bug 841850 and bug 902350, we handled the target=_blank and target=_top cases.

This bug is to write tests for the remaining targets (see below).  And also investigate how to handle target=_parent from a grandchild frame (we may discover that it doesn't need any additional code changes).

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.  Handled in bug 902350.

_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 bug was initially created as a clone of Bug #902350 +++

Comment 1

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #0) 
> _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.
> 

I wrote some test cases for _parent and tested them with the patch to bug 902350:
https://people.mozilla.com/~tvyas/navigateparent_grandchild.html - Single page (no frames) trying to navigate _parent.  Since it's parent is itself, the page navigates the top as it should.  Mixed Content Blocker is not invoked.

https://people.mozilla.com/~tvyas/navigateparent_child.html - Top page is parent of a child that wants to navigate it's _parent to an http page.  Since the child's parent is the top, the navigation succeeds as it should.  Mixed Content Blocker is not invoked.

https://people.mozilla.com/~tvyas/navigateparent_top.html - Top page is grandparent of a granchild that wants to navigate it's _parent to an http page.  This is blocked by the Mixed Content Blocker, as it it should be.


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

For this scenario, we can use the http version of one of the examples:
http://people.mozilla.com/~tvyas/navigateparent_top.html.  Top/grandparent is HTTP.  Contains an HTTPS parent frame.  The HTTPS parent frames contains an HTTPS grandchild frame.  The HTTPS grandchild frame tries to navigate it's parent (the HTTPS parent frame) to an HTTP site.  What happens?  Mixed Content Blocker blocks the navigation.  Is this the right thing to do or not?  The top is HTTP.  Navigating the HTTPS child to an HTTP page makes the page fully un-encrypted, but we weren't giving any encryption guarantees anyway since the top is HTTP.

Opinions on this are welcome.  Thanks!

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> For this scenario, we can use the http version of one of the examples:
> http://people.mozilla.com/~tvyas/navigateparent_top.html.  Top/grandparent
> is HTTP.  Contains an HTTPS parent frame.  The HTTPS parent frames contains
> an HTTPS grandchild frame.  The HTTPS grandchild frame tries to navigate
> it's parent (the HTTPS parent frame) to an HTTP site.  What happens?  Mixed
> Content Blocker blocks the navigation.  Is this the right thing to do or
> not?  The top is HTTP.  Navigating the HTTPS child to an HTTP page makes the
> page fully un-encrypted, but we weren't giving any encryption guarantees
> anyway since the top is HTTP.

AFAICT, you are right that the mixed content blocker should NOT block the navigation because it does not result in a non-secure iframe inside a secure document.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Tanvi Vyas [:tanvi] from comment #0)
> _content:
> _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

AFAICT, these should work like _top.

> The name of a frame:
> Example: https://people.mozilla.com/~tvyas/navigateframefoo.html.  Not sure
> how the code for this works yet.

I suspect that there may still be a problem here too. As far as I understand the logic after the fix for bug 902350, you special-case the current tab's top-level docshell and docshells that haven't been created yet. But, what about a docshell in another tab or another window that already exists?

There are four things about this code that make me worried: (1) the special-casing noted above, (2) the special logic for CSP in nsDocShell::DoURILoad added in bug 515460,(3) nsIContentPolicy check is in nsDocShell::InternalLoad but the actual channel that gets created is in nsDocShell::DoURILoad, very far away, and (4)  nsDocShell::InternalLoad is recursive, and the recursion happens AFTER the nsIContentPolicy checks. Doesn't #4 mean that in the cases of recursion the nsIContentPolicy checks happen multiple times for the same load? That seems bad, at least for performance, if not correctness.

I know almost nothing about docshell, but my naive opinion is that nsIContentPolicy checks should always be done immediately before the channel is created, whenever possible--i.e. in nsDocShell::DoURILoad in this case. I do not know what kind of problems that type of change would cause, but it would seem like it would have avoided some of these mixed-content-iframe-related issues.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #3)
> 
> > The name of a frame:
> > Example: https://people.mozilla.com/~tvyas/navigateframefoo.html.  Not sure
> > how the code for this works yet.
> 
> I suspect that there may still be a problem here too. As far as I understand
> the logic after the fix for bug 902350, you special-case the current tab's
> top-level docshell and docshells that haven't been created yet. But, what
> about a docshell in another tab or another window that already exists?
> 
What do you mean by docshells that haven't been created yet?  And why to docshells in other tabs or windows matter?  Bug 902350 just ensures that if a frame is trying to navigate the top, it is treated like a TYPE_DOCUMENT load instead of a TYPE_SUBDOCUMENT load in Content Policies.


> There are four things about this code that make me worried: (1) the
> special-casing noted above, (2) the special logic for CSP in
> nsDocShell::DoURILoad added in bug 515460,(3) nsIContentPolicy check is in
> nsDocShell::InternalLoad but the actual channel that gets created is in
> nsDocShell::DoURILoad, very far away, and (4)  nsDocShell::InternalLoad is
> recursive, and the recursion happens AFTER the nsIContentPolicy checks.
> Doesn't #4 mean that in the cases of recursion the nsIContentPolicy checks
> happen multiple times for the same load? That seems bad, at least for
> performance, if not correctness.
> 
> I know almost nothing about docshell, but my naive opinion is that
> nsIContentPolicy checks should always be done immediately before the channel
> is created, whenever possible--i.e. in nsDocShell::DoURILoad in this case. I
> do not know what kind of problems that type of change would cause, but it
> would seem like it would have avoided some of these
> mixed-content-iframe-related issues.

This type of rewrite/refactor is out of scope for this bug.

Comment 5

[Tanvi Vyas[:tanvi]]

The following things should be done to complete this bug

1) Write basic mochitests for the _* options mentioned in comment 0 (_content, _main, _self, _parent) and a test where a frame navigates a frame by name (ex: https://people.mozilla.com/~tvyas/navigateframefoo.html).

2) Fix this scenario so that MCB is NOT invoked when clicking on "Go to http site".
 
> For this scenario, we can use the http version of one of the examples:
> http://people.mozilla.com/~tvyas/navigateparent_top.html.  Top/grandparent
> is HTTP.  Contains an HTTPS parent frame.  The HTTPS parent frames contains
> an HTTPS grandchild frame.  The HTTPS grandchild frame tries to navigate
> it's parent (the HTTPS parent frame) to an HTTP site.  What happens?  Mixed
> Content Blocker blocks the navigation.  Is this the right thing to do or
> not?  The top is HTTP.  Navigating the HTTPS child to an HTTP page makes the
> page fully un-encrypted, but we weren't giving any encryption guarantees
> anyway since the top is HTTP.
> 

3) Investigate cases where a frame with a specific name is navigated.  Are there cases where MCB is invoked when it shouldn't be?  I think the answer to this is yes - we could find a scenario where this happens similar to the example in number 2.  But ideally writing code to fix this for number 2 will fix these cases too.

Comment 6

[Jefferson]

The four standardized keywords are _blank, _self, _parent, and _top per HTML 4.01 and HTML5.

_main (IE) and _content (Netscape) were created to direct links in pages loaded in the sidebar to open in the main content area.

See: /browser/base/content/browser.js contentAreaClick() function

Since they launch outside the sidebar, no mixed content problem.

When encountered in an iframe in a regular tab, these two targets appear to behave like "_top". I haven't traced all the paths to make sure that's always true. (I'm not qualified to do that.)