841850 - Opening a target=_blank link from an secure (HTTPS) iframe should not cause mixed content warning

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Frank Yan (:fryn)]

Steps to reproduce:
Click a link of the following type in a secure iframe in a secure page: <a href="http://..." target="_blank">example</a>

Expected result:
The link is opened in a new tab.

Actual result:
If security.mixed_content.block_active_content is false, the original page shows the mixed content warning in the URL bar.
If the pref is true, the link is blocked from being opened.

Example:
Links in Zimbra HTML emails

Testcase:
https://people.mozilla.com/~tvyas/navigateframe.html

Comment 1

[Tanvi Vyas[:tanvi]]

This effects FF 21.  When the pref is enabled, you get the shield icon.  When the pref is not enabled, the load is allowed but the original page gets a STATE_IS_BROKEN security state and you see a mixed content icon instead of the lock.

Comment 2

[Tanvi Vyas[:tanvi]]

In the example: https://bugzilla.mozilla.org/show_bug.cgi?id=841850

Assume security.mixed_content.block_active_content is set to false.  Go to the example page and click on "Go to http site".  We are about to initate a load, so the content policies are called.  nsMixedContentBlocker detects http content of type TYPE_SUBDOCUMENT (used for iframes) and hence classifies the content has mixed active content and allows the load.  Next nsMixedContentBlocker is called for a load of TYPE_DOCUMENT, which is always allowed (mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#227).

So the problem is that when nsMixedContentBlocker first learns of the load to the http page, it treats it like an iframe instead of a new document.  In order to fix this, nsMixedContentBlocker has to know that it is actually meant for a new window and it will soon be TYPE_DOCUMENT and not TYPE_SUBDOCUMENT.

It becomes TYPE_DOCUMENT in nsDocShell::InternalLoad a handful of lines after the content policy is checked: https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8665
At line 8711, OpenNoNavigate() is called, which eventually calls the content policies again, but this time with content type TYPE_DOCUMENT.

Can we somehow change nsDocShell::InternalLoad to check the target before calling the content policies?  Or can we pass information target information to nsMixedContentBlocker so that it knows not to block loads that are meant to create a new window?

Adding smaug and BZ for their advice.  Thanks!

Comment 3

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

Dogfood because you can't open links to external websites from within Zimbra and other Mozilla sites due to this bug.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Can we somehow change nsDocShell::InternalLoad to check the target before calling the
> content policies? 

Yes.  That seems like a very sane thing to do to me.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Detect targetDocument before calling content policies in nsDocShell::InternalLoad() v1

I moved https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8607 to line 8783 to right before line 8591.  Now, if a link in an iframe has a new window as it's target, it will be classified as TYPE_DOCUMENT instead of TYPE_SUBDOCUMENT in the call to content policies.  Hence, the mixed content blocker won't block an iframed http link that will open in a new tab on an https page.

The one thing I'm not sure about is whether I should move lines 8631-8658 to back to below the call for content policies, since they don't seem to be related to the target document code.

Comment 6

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

Comment on attachment 716376 [details] [diff] [review]
Detect targetDocument before calling content policies in nsDocShell::InternalLoad() v1

But please test cases when non-_blank is used. And this really needs mochitests.

Comment 7

[Tanvi Vyas[:tanvi]]

Pushed to try and needs tests:
https://tbpl.mozilla.org/?tree=Try&rev=92c54f7a25d6

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't understand that patch.  If conpol is going to block the load we don't want to be opening a new window for it.  But it looks to me like with this patch we will.

Furthermore, we're not going to do the conpol check with the right context, as far as I can tell...

The right thing to do is to look for an existing window, then do the conpol check based on what's found or not, then go ahead with opening windows as needed.

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> I don't understand that patch.  If conpol is going to block the load we
> don't want to be opening a new window for it.  But it looks to me like with
> this patch we will.

If the target calls for a new window, the content type is TYPE_DOCUMENT.  nsMixedCotnetnBlocker won't block TYPE_DOCUMENT (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#226) so we don't have to worry about a window that pops open and then doesn't load content.  Unless another content policy would prevent this for some reason?

> 
> Furthermore, we're not going to do the conpol check with the right context,
> as far as I can tell...
> 

We are moving the code to resolve the window target up before the call to NS_CheckContentLoadPolicy() (https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8726).  There is no change to the "context" parameter that is passed into NS_CheckContentLoadPolicy in that code.

> The right thing to do is to look for an existing window, then do the conpol
> check based on what's found or not, then go ahead with opening windows as
> needed.
Can you elaborate on this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> nsMixedCotnetnBlocker won't block TYPE_DOCUMENT

nsMixedContentBlocker is not the only content policy implementation around, though.

> There is no change to the "context" parameter that is passed into
> NS_CheckContentLoadPolicy in that code.

But there is.  The "context" is the frame element containing the docshell.  But in the "aWindowTarget && *aWindowTarget" case the code across which you moved the content policy check will call InternalLoad on some _other_ docshell and then return from this function.  Which means that when the content policy check is reached it will be with a different docshell, hence a different context.

Now maybe this is a desirable change, but it's not clear to me why.

> Can you elaborate on this?

I think the right behavior is to do the parts of window targeting that do FindItemWithName, and if that finds a docshell, that's where the load will happen and we can set the DOCUMENT vs SUBDOCUMENT based on whether that docshell is a sibframe.  If it finds nothing, we plan to open a new window and should treat this as a DOCUMENT load, not a SUBDOCUMENT one.  The key is to do the content policy check before doing the OpenNoNavigate call, imo.

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v1

(In reply to Olli Pettay [:smaug] from comment #6)
> But please test cases when non-_blank is used. 
I've added a bunch of frame navigation tests in Bug840388 for non-_blank targets.  Note, that these are in review (also by you) and haven't landed.


> And this really needs
> mochitests.


Here is a mochitest that checks that an https iframe can load an http link in a new window when target=_blank.  This is built on top of the mochitests in bug 840388.

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Detect targetDocument before calling content policies in nsDocShell::InternalLoad() v2

Updated the patch per bz's comments.  There is some duplicate code that I comment about in the patch.  Let me know what you want me to do about that (remove it, or leave the duplication).

r? to bz since he had concerns with the first version of the patch (where we could potentially open a window and then never navigate it because a content policy blocked the load after the window open).  Can also pass it back to smaug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 727992 [details] [diff] [review]
Detect targetDocument before calling content policies in nsDocShell::InternalLoad() v2

We should in fact remove the second FindItemWithName call and use the information we found the first time.

Comment 14

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

Comment on attachment 727484 [details] [diff] [review]
Mochitests v1

Could you ask review once the main patch is reviewed.
(I'm just trying to organize my review queue.)

Comment 15

[Tanvi Vyas[:tanvi]]

Attachment: Detect if there is a targetDocShell before calling content policies in nsDocShell::InternalLoad() v3

Updated per bz's comments.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728336 [details] [diff] [review]
Detect if there is a targetDocShell before calling content policies in nsDocShell::InternalLoad() v3

>+    if (IsFrame() && !targetDocShell) {

This seems backwards.  This will use SUBDOCUMENT if we _didn't_ find a target docshell, no?

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Detect if there is a targetDocShell before calling content policies in nsDocShell::InternalLoad() v4

(In reply to Boris Zbarsky (:bz) from comment #16)
> Comment on attachment 728336 [details] [diff] [review]
> Detect if there is a targetDocShell before calling content policies in
> nsDocShell::InternalLoad() v3
> 
> >+    if (IsFrame() && !targetDocShell) {
> 
> This seems backwards.  This will use SUBDOCUMENT if we _didn't_ find a
> target docshell, no?

Yes, you are right.  And actually, the opposite doesn't work quite right anyway.  What we really want is something like:
if (IsFrame() && !(aWindowTarget && *aWindowTarget && !targetDocShell))

If we have aWindowTarget and we have identified a targetDocShell from that Window, then we assume this is a sub-document.  If we have aWindowTarget and we have not identified a targetDocShell, we assume that this is a document.

If we don't have aWindowTarget, targetDocShell will be null and checking if (IsFrame() && targetDocShell) would fail, and hence legitimate frames in the same document would be considered documents instead of sub-documents.

Instead of using that rather ugly if, I put back in the isNewDocShell boolean.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728424 [details] [diff] [review]
Detect if there is a targetDocShell before calling content policies in nsDocShell::InternalLoad() v4

Yeah, much better, thank you!  Two nits:

1)  Just set "isNewDocShell = !targetDocShell;" at the end of the aWindowTarget
    block instead of doing the conditional thing.
2)  This file is 4-space indented, but your new code is two-space indented. 
    Please fix the indentation.

r=me with those fixed.

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Detect if there is a targetDocShell before calling content policies in nsDocShell::InternalLoad() v5

Thanks bz!  Patch updated and r+ carried over.

(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 728424 [details] [diff] [review]
> Detect if there is a targetDocShell before calling content policies in
> nsDocShell::InternalLoad() v4
> 
> Yeah, much better, thank you!  Two nits:
> 
> 1)  Just set "isNewDocShell = !targetDocShell;" at the end of the
> aWindowTarget
>     block instead of doing the conditional thing.
> 2)  This file is 4-space indented, but your new code is two-space indented. 
>     Please fix the indentation.
> 
> r=me with those fixed.

Comment 20

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v2

Updated mochitests. They are on top of the mochitests in bug 840388, and those needed some cleaning up.  Will r? the tests here after the mochitests for bug 840388 are finalized.

Comment 21

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v3

Updated so the test applies cleanly on top of the tests in bug 840388.

Comment 22

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v4

Forgot to remove "type=" from the query string.

Comment 23

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

Comment on attachment 729354 [details] [diff] [review]
Mochitests v4

I don't see the need for setTimeout here. Just wait for the right content-document-global-created and post the message and remove the observer.

Comment 24

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v5

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 729354 [details] [diff] [review]
> Mochitests v4
> 
> I don't see the need for setTimeout here. Just wait for the right
> content-document-global-created and post the message and remove the observer.

Updated the patch so that it applies on top of the new mochitests in bug 840388 and removed the setTimeout.  Carrying over the r+.

Comment 25

[Tanvi Vyas[:tanvi]]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f46bbb876415
hg.mozilla.org/integration/mozilla-inbound/rev/95274843db65

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f46bbb876415
https://hg.mozilla.org/mozilla-central/rev/95274843db65