Closed Bug 841850 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: fryn, Assigned: tanvi)

References

(Blocks 1 open bug, )

Details

(Keywords: dogfood, regression)

Attachments

(2 files, 8 obsolete files)

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
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.
Target Milestone: --- → mozilla21
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!
Dogfood because you can't open links to external websites from within Zimbra and other Mozilla sites due to this bug.
Blocks: 834836
No longer blocks: MixedContentBlocker
Keywords: dogfood, regression
> 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.
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.
Attachment #716376 - Flags: review?(bugs)
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.
Attachment #716376 - Flags: review?(bugs) → review+
Pushed to try and needs tests:
https://tbpl.mozilla.org/?tree=Try&rev=92c54f7a25d6
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.
(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?
> 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.
Attached patch Mochitests v1 (obsolete) — Splinter Review
(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.
Attachment #727484 - Flags: review?(bugs)
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.
Attachment #716376 - Attachment is obsolete: true
Attachment #727992 - Flags: review?(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.
Attachment #727992 - Flags: review?(bzbarsky) → review-
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.)
Attachment #727484 - Flags: review?(bugs)
Updated per bz's comments.
Attachment #727992 - Attachment is obsolete: true
Attachment #728336 - Flags: review?(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?
(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.
Attachment #728336 - Attachment is obsolete: true
Attachment #728336 - Flags: review?(bzbarsky)
Attachment #728424 - Flags: review?(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.
Attachment #728424 - Flags: review?(bzbarsky) → review+
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.
Attachment #728424 - Attachment is obsolete: true
Attachment #728665 - Flags: review+
Attached patch Mochitests v2 (obsolete) — Splinter Review
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.
Attachment #727484 - Attachment is obsolete: true
Attachment #728667 - Flags: review?(bugs)
Attached patch Mochitests v3 (obsolete) — Splinter Review
Updated so the test applies cleanly on top of the tests in bug 840388.
Attachment #728667 - Attachment is obsolete: true
Attachment #728667 - Flags: review?(bugs)
Attachment #729351 - Flags: review?(bugs)
Attached patch Mochitests v4 (obsolete) — Splinter Review
Forgot to remove "type=" from the query string.
Attachment #729351 - Attachment is obsolete: true
Attachment #729351 - Flags: review?(bugs)
Attachment #729354 - Flags: review?(bugs)
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.
Attachment #729354 - Flags: review?(bugs) → review+
Attached patch Mochitests v5Splinter Review
(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+.
Attachment #729354 - Attachment is obsolete: true
Attachment #730347 - Flags: review+
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f46bbb876415
hg.mozilla.org/integration/mozilla-inbound/rev/95274843db65
Assignee: nobody → tanvi
https://hg.mozilla.org/mozilla-central/rev/f46bbb876415
https://hg.mozilla.org/mozilla-central/rev/95274843db65
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla21 → mozilla22
Depends on: 855730
No longer depends on: 855730
You need to log in before you can comment on or make changes to this bug.