909920 - Mixed content warning should not show on a HTTP site

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(credit goes to mcoates for finding this)

At:

http://www.darkreading.com/management/incentives-and-organizational-alignment/240160465

It is not a HTTP site. Clicking the mixed content icon shows "Interactive content (such as script) that isn't encrypted has been blocked for your protection".

The information conveyed to the user is not clear - the main page is not HTTPS so the user can get confused about "mixed content".

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot of dialog in question

Comment 2

[Tanvi Vyas[:tanvi]]

Thanks Gark and mcoates for finding this!  This looks like a pretty bad bug where the wrong text is showing up for the case where a page has Mixed Display Content loaded.

Testing in Firefox 26.0a1 -
1) Go to https://people.mozilla.com/~tvyas/mixeddisplay.html
2) You see a grey triangle as you should (from bug 865352).  And a message in the webconsole indicating that mixed display content was loaded:
Loading mixed (insecure) display content on a secure page "http://cisforcookie.org/CookieMonster.jpg" @ https://people.mozilla.com/~tvyas/mixeddisplay.html
3) Click on the grey triangle and you see the following message:
Interactive content (such as script) that isn't encrypted has been blocked for your protection.

What you should see is the following:
The connection to this website is not fully secure because it contains unencrypted elements (such as images).

The wrong identity string is being used - http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#261

I am going to investigate why.

There is a second issue in this bug where http://darkreading is getting the grey triangle and the wrong text.  I still have to hunt that down.

Comment 3

[Tanvi Vyas[:tanvi]]

On http://www.darkreading.com/management/incentives-and-organizational-alignment/240160465

I get the following webconsole messages:

Loading mixed (insecure) display content on a secure page "http://twimgs.com/custom/csb/images/nextgen/nextgen_container_header_bg.gif" @ http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js:117
Loading mixed (insecure) display content on a secure page "http://twimgs.com/custom/csb/images/nextgen/nextgen_close_btn.gif" @ http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js:117
Loading mixed (insecure) display content on a secure page "http://twimgs.com/custom/csb/images/nextgen/nextgen_container_mid_bg.gif" @ http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js:117
Loading mixed (insecure) display content on a secure page "http://twimgs.com/custom/csb/images/nextgen/nextgen_container_footer_bg.gif" @ http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js:117

The only reason an HTTP page would show Mixed Content warnings is if it contained an HTTPS iframe with mixed content (frame desecendants rule [1]).  But Here is says that HTTP jquery is including HTTP images.  The jquery itself should have been flagged as Mixed Active and blocked if it was running in an HTTPS frame.  Unless HTTP jquery on the HTTP page was injecting HTTP images into an HTTPS frame?  Very odd; still digging into this.


[1] http://blogs.msdn.com/b/ie/archive/2011/06/23/internet-explorer-9-security-part-4-protecting-consumers-from-malicious-mixed-content.aspx
http://ie.microsoft.com/testdrive/ieblog/2011/Jun/23_InternetExplorer9SecurityPart4ProtectingConsumersfromMaliciousMixedContent_5.png

Comment 4

[Tanvi Vyas[:tanvi]]

I filed bug 912817 for the issue in comment 2.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

The page http://www.darkreading.com/ loads its css file using https:
  https://i.cmpnet.com/informationweek/whitepaper/v3/common/css/twlightbox.css
Within that css file we load the image:
  http://twimgs.com/custom/csb/images/nextgen/nextgen_container_header_bg.gif
which causes MCB to report. I assume this is similar behavior to an https iframe loaded in an http site.

The interesting thing is, that none of the reporting calls to ReportToConsole from MixedContentBlocker.cpp include a URI or also a linenumber.
How come, that in this specific case we receive a JSContext and also a CallingLocation in
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3058
which causes the wrong value for the variable spec to be set.

If we would not get the JSContext and the LineNumber, we would correctly set the spec to the *.css file instead of the reported *.js file like in all the other cases MixedContentBlocker reports a MCB warning to the console.

Comment 6

[Tanvi Vyas[:tanvi]]

http://people.mozilla.org/~tvyas/darkreading.html

I created a test case that shows the following:
HTTP top level page
Includes HTTPS CSS
HTTPS CSS tries to include HTTP image

We get a mixed display warning in the webconsole.  And a grey triangle icon in the url bar.  This is not expected behavior.  There are no HTTPS iframes on this page and hence the concept of Mixed Content makes no sense on this HTTP page.

Debug messages show that aContentLocation is the twimgs.com gif and that aRequestingLocation is the https://people.mozilla.org/~tvyas/style.css.  They also show that aRequestingLocation is passed into shouldLoad and not computed from aRequestingPrincipal: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#345

This means that aRequestingLocation is not necessarily the location of the top level page or a frame, but could also be the location of a css file, script, object, etc that is trying to include other content.  We need to rewrite some of the Mixed Content Blocking code to account for this.

What are the current ramifications of this?
* We may incorrectly show mixed display content warnings on HTTP pages.  This means different things on FF 23, 24, 25, and 26 as far as what the user might see.  I will detail that in a later comment.
* We may incorrectly block active content on HTTP pages.  This is the biggest issue.
* For HTTPS pages, there are no ramifications that I can think of.

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> * We may incorrectly block active content on HTTP pages.  This is the
> biggest issue.

We have not been able to produce a test case that does this.

http://people.mozilla.org/~tvyas/darkreading-script.html

This is an HTTP page that includes HTTPS script.  The HTTPS script attempts to load HTTP javascript and css, and does so without any issues.  MCB identifies the aRequestingLocation to be the top level page and not the script.

Why is this different than the test case in the previous example?  Why is the css coming up in aRequestingLocation and not the javascript?

Christoph and I are still investigating.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking based on the impact to users of HTTP pages - let's get a Test Plan around how to verify this in Beta against top sites and to gauge how commonly this might be being encountered with our current release.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

A stacktrace from nsMixedContentBlocker::ShouldLoad reveals that loading an image follows two different paths in the code, depending on whether the image is loaded from a css file (case 1) or from a js-script (case 2). 

1) http://people.mozilla.org/~tvyas/darkreading.html

> nsMixedContentBlocker::ShouldLoad { 
>   aContentLocation: http://twimgs.com/custom/csb/images/nextgen/nextgen_container_header_bg.gif
>   aRequestingContext: https://people.mozilla.org/~tvyas/style.css
> }
> #0  nsMixedContentBlocker::ShouldLoad at content/base/src/nsMixedContentBlocker.cpp:222
> #1  nsContentPolicy::CheckPolicy at /content/base/src/nsContentPolicy.cpp:127
> #2  nsContentPolicy::ShouldLoad /content/base/src/nsContentPolicy.cpp:190
> #3  NS_CheckContentLoadPolicy /dist/include/nsContentPolicyUtils.h:213
> #4  nsContentUtils::CanLoadImage /content/base/src/nsContentUtils.cpp:2637
> #5  mozilla::css::ImageLoader::LoadImage /layout/style/ImageLoader.cpp:261
> #6  mozilla::css::ImageValue::ImageValue /layout/style/nsCSSValue.cpp:1816
> #7  nsCSSValue::StartImageLoad /layout/style/nsCSSValue.cpp:567

---------------------------------------------------------------------

2) http://people.mozilla.org/~tvyas/darkreading-script.html

> nsMixedContentBlocker::ShouldLoad { 
>   aContentLocation: http://cisforcookie.org/CookieMonster.jpg
>   aRequestingContext: http://people.mozilla.org/~tvyas/darkreading-script.html
> }
> #0  nsMixedContentBlocker::ShouldLoad /content/base/src/nsMixedContentBlocker.cpp:222
> #1  nsContentPolicy::CheckPolicy /content/base/src/nsContentPolicy.cpp:127
> #2  nsContentPolicy::ShouldLoad /content/base/src/nsContentPolicy.cpp:190
> #3  NS_CheckContentLoadPolicy /dist/include/nsContentPolicyUtils.h:213
> #4  nsContentUtils::CanLoadImage /content/base/src/nsContentUtils.cpp:2637
> #5  nsImageLoadingContent::LoadImage /content/base/src/nsImageLoadingContent.cpp:792
> #6  nsImageLoadingContent::LoadImage /content/base/src/nsImageLoadingContent.cpp:728
> #7  mozilla::dom::HTMLImageElement::SetAttr /content/html/content/src/HTMLImageElement.cpp:411

Case 2) works as expected where aRequestingContext holds the *.html file.

A possible fix might be to replace http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#566 with aDocument->NodePrincipal(), which is the *.html file and not the css file. I am not sure what further implications this change might have.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

In addition to the previous comment. The value for mValue.mURL->mOriginPrincipal
  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#566
is assigned earlier. Probably, this should be the principal of the loading *.html instead of the *.css, see: 
  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#5463

I am not sure if we can just replace the css principal with the principal of the html file, but that is the root cause of getting the MCB warning. If we want to change that, we could set the principal when initing the CSSParser in
  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#821
This principal is then propagated all the way to the ShouldLoad in MixedContentBlocker.cpp.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Another interesting testcase is loading a font from inside an https css file.

3) http://people.mozilla.org/~tvyas/darkreading-font.html

> nsMixedContentBlocker::ShouldLoad {
>  aContentLocation: http://fonts.googleapis.com/css?family=Inconsolata
>  aRequestingLocation: https://people.mozilla.org/~tvyas/style2.css
> }
>
> #0  nsContentPolicy::ShouldLoad /mc/content/base/src/nsContentPolicy.cpp:198
> #1  NS_CheckContentLoadPolicy /dist/include/nsContentPolicyUtils.h:238
> #2  mozilla::css::Loader::CheckLoadAllowed /layout/style/Loader.cpp:1047
> #3  mozilla::css::Loader::LoadChildSheet mc/layout/style/Loader.cpp:2072
> #4  CSSParserImpl::ProcessImport /layout/style/nsCSSParser.cpp:2011
> #5  CSSParserImpl::ParseImportRule /layout/style/nsCSSParser.cpp:1982

Case 1) and Case 3), the two cases where we load content from inside a https-css file both use the *.css file as the aRequestingLocation.
It seems to me that we have to provide a special case handling inside MixedContentBlocker::ShouldLoad where we account for http content loaded from a https-css file. Similar to the special handling of https-iframes.

Comment 12

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> Case 1) and Case 3), the two cases where we load content from inside a
> https-css file both use the *.css file as the aRequestingLocation.
> It seems to me that we have to provide a special case handling inside
> MixedContentBlocker::ShouldLoad where we account for http content loaded
> from a https-css file. Similar to the special handling of https-iframes.

In these cases, what is aRequestPrincipal? Perhaps the solution is to always use aRequestPrincipal whenever it is provided and only use aRequestingLocation when aRequestPrincipal is missing?

Comment 13

[Tanvi Vyas[:tanvi]]

In order to fix this we need to know which of these 2 issues is the root cause:

1) layout/style is setting the wrong aRequestingLocation and aRequestingLocation is supposed to be the top level page or containing frame.
2) Content Policies should not depend on aRequestingLocation being a top level page or containing frame.  aRequestingLocation could be css, script, objects, etc and content policies (like MCB) need to account for that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

>205    * @param aRequestOrigin    OPTIONAL. the location of the resource that
>206    *                          initiated this load request; can be null if
>207    *                          inapplicable

So for loads from a stylesheet it should be the stylesheet.

For loads from a stylesheet, furthermore, aRequestPrincipal will be the principal of the sheet.

If you're looking for the DOM tree that matters, that's what aContext is for.

Comment 15

[Tanvi Vyas[:tanvi]]

Thanks BZ for chiming in!

(In reply to Boris Zbarsky [:bz] from comment #14)
> >205    * @param aRequestOrigin    OPTIONAL. the location of the resource that
> >206    *                          initiated this load request; can be null if
> >207    *                          inapplicable
> 
> So for loads from a stylesheet it should be the stylesheet.
> 
Is that true for only stylesheet loads?  scripts, objects, etc will never be the aRequestingLocation?


> For loads from a stylesheet, furthermore, aRequestPrincipal will be the
> principal of the sheet.
> 
> If you're looking for the DOM tree that matters, that's what aContext is for.

This sounds like it is opposite of what we are using in nsMixedContentBlocker.cpp.  First we use aRequestingLocation if it is provided.  If not, we try aRequestPrincipal.  If both are missing, then (and only then) do we use the Context:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#345

Should we:
1) Use aRequestingContext to get the principal and location and ignore the aRequestingLocation and aRequestingPrincipal that are passed in?

2) Special case loads coming from CSS?  If we wanted to go down this route, how would we do this?  There is no content type associated with the requesting location.  Looking for ".css" in the uri is not a good idea.  How do we know this load was initiated from a stylesheet?

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #12)
> In these cases, what is aRequestPrincipal? Perhaps the solution is to always
> use aRequestPrincipal whenever it is provided and only use
> aRequestingLocation when aRequestPrincipal is missing?

As BZ mentioned in comment 14, aRequestingPrincipal yields the same results.  I also verified this.

Comment 17

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

Wasn't there something odd going on with principals too... can't recall, that was so long ago.
But if principal works, using it sounds the right thing to do.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> scripts, objects, etc will never be the aRequestingLocation?

Scripts don't ever do any loads themselves.

<object> can do loads, for sure.  Just like a subframe (in fact, mostly via the same codepaths).

SVG external resources can do loads too.

> 1) Use aRequestingContext to get the principal and location and ignore the
> aRequestingLocation and aRequestingPrincipal that are passed in?

What information are you actually looking for?

Comment 19

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> > 1) Use aRequestingContext to get the principal and location and ignore the
> > aRequestingLocation and aRequestingPrincipal that are passed in?
> 
> What information are you actually looking for?

We need to know the scheme of the document or subdocument that the resource is being loaded into.  We assumed that aRequestingLocation was the top level document or the frame and hence we got the scheme from there.  Since aRequestingLocation might be a css file, we can no longer depend on its scheme to tell us whether the content that we are about to load is on an HTTP page/frame or an HTTPS page/frame.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Ah.  Then yeah, you want aContext, I think...

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Values of all the relevant arguments to nsMixedContentBlocker::ShouldLoad for the three test cases:

a) http://people.mozilla.org/~tvyas/darkreading.html
>  aContentLocation:    http://twimgs.com/custom/csb/images/nextgen/nextgen_container_header_bg.gif
>  aRequestingLocation: https://people.mozilla.org/~tvyas/style.css
>  aRequestingContext:  http://people.mozilla.org/~tvyas/darkreading.html
>  aRequestPrincipal:   https://people.mozilla.org/~tvyas/style.css

b) http://people.mozilla.org/~tvyas/darkreading-font.html
>  aContentLocation:    http://fonts.googleapis.com/css?family=Inconsolata
>  aRequestingLocation: https://people.mozilla.org/~tvyas/style2.css
>  aRequestingContext:  http://people.mozilla.org/~tvyas/darkreading-font.html
>  aRequestPrincipal:   https://people.mozilla.org/~tvyas/style2.css

c) http://people.mozilla.org/~tvyas/darkreading-script.html
>  aContentLocation:    http://cisforcookie.org/CookieMonster.jpg
>  aRequestingLocation: http://people.mozilla.org/~tvyas/darkreading-script.html
>  aRequestingContext:  http://people.mozilla.org/~tvyas/darkreading-script.html
>  aRequestPrincipal:   http://people.mozilla.org/~tvyas/darkreading-script.html

Please note that we extract the uri for aRequestingContext using the following (simplified) code: 
> nsCOMPtr<nsINode> node = do_QueryInterface(aRequestingContext);
> nsCOMPtr<nsIPrincipal> tmpPrincipal = node->NodePrincipal();
> tmpPrincipal->GetURI(getter_AddRefs(tmpURI));
> printf("%s", tmpURI.get());

Comment 22

[Boris Zbarsky [:bzbarsky]]

aRequestingContext can be a Window too, not just a Node.

Comment 23

[Jesse Ruderman]

> * We may incorrectly block active content on HTTP pages.  This is the biggest issue.

What are the cases you consider incorrect blocking?

  HTTP page -> HTTPS stylesheet -> HTTP image  (with pref to block mixed display content)?
  HTTP page -> HTTPS iframe -> HTTP script     ?

Comment 24

[Tanvi Vyas[:tanvi]]

(In reply to Jesse Ruderman from comment #23)
> > * We may incorrectly block active content on HTTP pages.  This is the biggest issue.
> 
> What are the cases you consider incorrect blocking?
> 
>   HTTP page -> HTTPS stylesheet -> HTTP image  (with pref to block mixed
> display content)?

Yes, that is incorrect blocking.  The page is HTTP.

>   HTTP page -> HTTPS iframe -> HTTP script     ?

This would be correct blocking because the frame is HTTPS and hence we should not allow Mixed Script to compromise the integrity of the HTTPS iframe.  We have special code in nsMixedContentBlocker.cpp that handles this already.  This bug is not about frames, but about other content types.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site_tests.patch

Since we are already adding an image file, we probably could also add a font for a second test.

Comment 27

[Tanvi Vyas[:tanvi]]

Comment on attachment 808860 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site.patch

>Bug 909920 - Mixed content warning should not show on a HTTP site
>
>diff --git a/content/base/src/nsMixedContentBlocker.cpp b/content/base/src/nsMixedContentBlocker.cpp
>--- a/content/base/src/nsMixedContentBlocker.cpp
>+++ b/content/base/src/nsMixedContentBlocker.cpp
>@@ -337,64 +337,73 @@ nsMixedContentBlocker::ShouldLoad(uint32
>       NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT, &schemeSecure))) {
>     return NS_ERROR_FAILURE;
>   }
> 
>   if (schemeLocal || schemeNoReturnData || schemeInherits || schemeSecure) {
>      return NS_OK;
>   }
> 
>-  // We need aRequestingLocation to pull out the scheme. If it isn't passed
>-  // in, get it from the aRequestingPricipal
>-  if (!aRequestingLocation) {
>-    if (!aRequestPrincipal) {
>-      // If we don't have aRequestPrincipal, try getting it from the
>-      // DOM node using aRequestingContext
>-      nsCOMPtr<nsINode> node = do_QueryInterface(aRequestingContext);
>-      if (node) {
>-        aRequestPrincipal = node->NodePrincipal();
>-      } else {
>-        // Try using the window's script object principal if it's not a node.
>-        nsCOMPtr<nsIScriptObjectPrincipal> scriptObjPrin = do_QueryInterface(aRequestingContext);
>-        if (scriptObjPrin) {
>-          aRequestPrincipal = scriptObjPrin->GetPrincipal();
>-        }
>-      }
>+  /*
>+   * We process the reuesting Location in the following order:
requesting
>+   *   1 - aRequestingContext
>+   *   2 - aRequestPrincipal
>+   *   3 - aRequestingLocation (@TANVI: do we even need this one?)
Depends if we have callers of Content Policy that do not set aRequestingContext or aRequestPrincipal.  Check the callers in mxr (ignoring addons) and see what you find.

>+   */
>+
>+  nsCOMPtr<nsIPrincipal> principal;
>+  // 1) Try to pull out the scheme of aRequestingContext
>+  nsCOMPtr<nsINode> node = do_QueryInterface(aRequestingContext);
>+  if (node) {
>+    principal = node->NodePrincipal();
>+  }
>+
>+  // If we still do not have a principal, try getting it from the DOM node
>+  if (!principal) {
>+    nsCOMPtr<nsIScriptObjectPrincipal> scriptObjPrin = do_QueryInterface(aRequestingContext);
>+    if (scriptObjPrin) {
>+      principal = scriptObjPrin->GetPrincipal();

Per BZ, in comment 22, we have to look for a window as well.

>     }
>-    if (aRequestPrincipal) {
>-      nsCOMPtr<nsIURI> principalUri;
>-      nsresult rvalue = aRequestPrincipal->GetURI(getter_AddRefs(principalUri));
>-      if (NS_SUCCEEDED(rvalue)) {
>-        aRequestingLocation = principalUri;
>-      }
>+  }
>+
>+  // 2) Try getting the principal from the aRequestPrincipal
If we still do not have a principal, try getting the principal from aRequestPrincipal
>+  if (!principal) {
>+    principal = aRequestPrincipal;
>+  }

Comment 28

[Tanvi Vyas[:tanvi]]

Comment on attachment 808862 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site_tests.patch


>diff --git a/browser/base/content/test/general/browser_bug909920.js b/browser/base/content/test/general/browser_bug909920.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/general/browser_bug909920.js
>@@ -0,0 +1,78 @@
>+/*
>+ * Description of the Tests for
>+ *  - Bug 909920 - Mixed content warning should not show on a HTTP site
>+ *
>+ * Description of the test:
>+ *   1) Load an http page
>+ *   2) The page includes a css file using https
>+ *   3) The css file loads an image over http
>+ *
>+ * Since the top-domain is >> NOT << served using https, the MCB
>+ * should >> NOT << trigger a warning.
>+ */
>+
>+const PREF_ACTIVE = "security.mixed_content.block_active_content";
Since this test depends on an image load and the associated warning, we should store, set and reset the mixed display flag instead of the active one (or in addition to the active one if/when we add fonts to the test).

For fonts, in a previous patch I have commented that "Load events for external fonts are not detectable by javascript."  In this case we don't need to actually check if the font loaded; just that the MCB was or was not invoked / security state did or did not change.  I would ask around or look in mxr for mochitests involving external fonts.
https://mxr.mozilla.org/mozilla-central/source/content/base/test/file_mixed_content_main.html?force=1#24


>diff --git a/browser/base/content/test/general/file_bug909920.css b/browser/base/content/test/general/file_bug909920.css
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/general/file_bug909920.css
>@@ -0,0 +1,4 @@
>+#testDiv {
>+  background: url(http://example.com/browser/browser/base/content/test/general/file_bug909920.gif)
You could use another gif that is already in the test suite instead of adding a new one.  In a previous mixed content test, I used
tests/image/test/mochitest/blue.png
https://mxr.mozilla.org/mozilla-central/source/image/test/mochitest/blue.png
https://mxr.mozilla.org/mozilla-central/source/content/base/test/file_mixed_content_main.html?force=1#184


>diff --git a/browser/base/content/test/general/file_bug909920.html b/browser/base/content/test/general/file_bug909920.html
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/general/file_bug909920.html
>@@ -0,0 +1,46 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+  Test 1 for Bug 909920 - See file browser_bug909920.js for description.
>+  https://bugzilla.mozilla.org/show_bug.cgi?id=909920
>+-->
>+<head>
>+  <meta charset="utf-8">
>+  <title>Test 1 for Bug 902156</title>
Bug 909920

>+  <link rel="stylesheet" type="text/css" href="https://example.com/browser/browser/base/content/test/general/file_bug909920.css" />
>+<script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+<script type="text/javascript">
>+  function checkLoadStates() {
>+  	var ui = SpecialPowers.wrap(window)
>+    		 .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
>+             .getInterface(SpecialPowers.Ci.nsIWebNavigation)
>+             .QueryInterface(SpecialPowers.Ci.nsIDocShell)
>+             .securityUI;
>+
>+  	var loadedMixedActive = ui &&
>+    	(ui.state & SpecialPowers.Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT);
>+  	is(loadedMixedActive, false, "OK: Should not load mixed active content!");
>+
>+  	var blockedMixedActive = ui &&
>+       (ui.state & SpecialPowers.Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT);
>+  	is(blockedMixedActive, false, "OK: Should not block mixed active content!");
>+
>+  	var loadedMixedDisplay = ui &&
>+    	(ui.state & SpecialPowers.Ci.nsIWebProgressListener.STATE_LOADED_MIXED_DISPLAY_CONTENT);
>+  	is(loadedMixedDisplay, false, "OK: Should not load mixed display content!");
>+
>+  	var blockedMixedDisplay = ui &&
>+    	(ui.state & SpecialPowers.Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT);
>+  	is(blockedMixedDisplay, false, "OK: Should not block mixed display content!");
>+
>+  	var newValue = "Verifying MCB for http page - https css - http image";
Verifying MCB does not trigger warning/error for http page - https css - http image
>+  	document.getElementById("testDiv").innerHTML = newValue;
Is this just to ensure that all the above tests are completed before test1() is completed?

>+  }
>+</script>
>+</head>
>+<body onload="checkLoadStates()">
>+  <div class="testDiv" id="testDiv">
>+    Testing MCB for http page - https css - http image
Testing MCB on http page...

>+  </div>
>+</body>
>+</html>

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

I guess what bz meant in Comment 22 is that we should also account for the window's script object principal if it's not a node.
I guess we already did that in the previous version, which I updated to follow the new processing hierarchy in this patch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yes, that's what I meant.  The patch does that fine, as far as it goes.

Comment 31

[Tanvi Vyas[:tanvi]]

Comment on attachment 809477 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site.patch

For MCB we need the uri of the owning document.

In the current code, we try get the uri in the following order:
1) from the aRequestingLocation
2) from the aRequestPrincipal
3) from the aRequestingContext
4) if we still don't have a uri, we check to see if aRequestPrincipal is system principal or an expanded principal (if it is, we accept the load).
If we don't get a uri at this point, we reject the load.

This patch tries to get the uri in the following order
1) from the aRequetingContext
2) from the aRequestingLocation
3) if we still don't have a uri, we check to see if aRequestPrincipal is system principal or an expanded principle (if it is, we accept the load).
If we don't get a uri at this point, we reject the load.

It should have a step between 2 and 3 that tries to get the uri from aRequestPrincipal.  Something like:

    if (!requestingLocation && aRequestPrincipal) {
      nsCOMPtr<nsIURI> principalUri;
      nsresult rvalue = aRequestPrincipal->GetURI(getter_AddRefs(principalUri));
      if (NS_SUCCEEDED(rvalue)) {
        requestingLocation = principalUri;
      }
    }

Comment 32

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #31)
> This patch tries to get the uri in the following order
> 1) from the aRequetingContext
> 2) from the aRequestingLocation
> 3) if we still don't have a uri, we check to see if aRequestPrincipal is
> system principal or an expanded principle (if it is, we accept the load).
> If we don't get a uri at this point, we reject the load.
> 
> It should have a step between 2 and 3 that tries to get the uri from
> aRequestPrincipal.

Thinking about this further... what if RequestingContext yields a principal that is a system principal or an expanded principal.  If we use aRequestingLocation in step 2 (or if add a step between 2 and 3 that uses aRequestingPrincipal), we may end up getting the uri of a subresource (not the owning document) and using that, without realizing that the owning document has system principal or an expanded principal.

Hence, maybe we need to rethink this order again.

Get the uri of the owning document:
1) from the aRequestingContext
2) If aRequestingContext yields a principal but no location, check if its system principal or an expanded principal.  If it isn't, continue
3) from the aRequestingLocation
4) from the aRequestPrincipal
5) if we still don't have a uri, we check to see again if the principal is system principal or an expanded principle (if it is, we accept the load).
If we don't get a uri at this point, we reject the load.


OR since there are cases where aRequestingLocation and aRequestPrincipal are definitely not the owning document, maybe we should ignore them altogether and simplify the code:
1) from the aRequestingContext
2) If aRequestingContext yields a principal but no location, check if its system principal or an expanded principal.  If it isn't, reject the load.

Can we try the second approach, push it to try and see what happens?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Actually, I seem to recall aRequestingContext not being all that useful for loads in subframes: it's the window the load is happening in, not what started the load, iirc.

We should really just pass through the thing CSP needs here, which is none of the current things being passed in.

Comment 34

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

(In reply to Boris Zbarsky [:bz] from comment #33)
> We should really just pass through the thing CSP needs here, which is none
> of the current things being passed in.

I strongly agree. Mixed content blocker is not the only thing that has to deal with this issue and we might as well fix it properly instead of trying to hack in a bunch of fallbacks.

I think the only thing I disagree with bz on is about extensions. I think it is totally reasonably to require the necessary principals to be passed in, even though we know some extensions don't. Being optimistic about missing information to support extensions is at odds with secure programming practice where being pessimistic is much more safe.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I could certainly be convinced to make the principal argument here required.

Comment 36

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky [:bz] from comment #35)
> I could certainly be convinced to make the principal argument here required.

Note that even if it was required, it still wouldn't always be the owning documents principal, so we would have to add another parameter regardless.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Yes, I meant the new parameter, not the existing principal arg (which we would also make required).

Comment 38

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

Could we stop adding more parameters and making the API more and more confusing, but add a new
method which would be possibly opt-in when registering a contentpolicy - or perhaps a new
API?
(Though, it is not clear to me what a sane contentpolicy API should look like.)

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

We performed a test where we use an HTTP page with and HTTPS iframe. When the script loads in the HTTPS iframe, aRequestingContext gives us the HTTPS iframe as the principal, not the HTTP page. See output:

http://people.mozilla.org/~tvyas/mixediframe2.html
> 1a) aRequestingContext is a node
> aContentLocation: http://people.mozilla.com/~tvyas/script.js
> requestingLocation: https://people.mozilla.org/~tvyas/mixedcontent.html

For the second test we load an HTTP page with an HTTPS css file which loads an HTTP image.

http://people.mozilla.org/~tvyas/darkreading.html
> 1a) aRequestingContext is a node
> aContentLocation: http://twimgs.com/custom/csb/images/nextgen/nextgen_container_header_bg.gif
> requestingLocation: http://people.mozilla.org/~tvyas/darkreading.html

Bz, don't these results imply that the context is not the window the load is happening in?
Note, that in all cases the node's principal and the window scripts object principal are identical (basically cases 1a and 1b in the patch code would return the same result!)

Thanks,
  Christoph

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site_comment_32.patch

Pushed the idea from comment 32 to try:
  https://tbpl.mozilla.org/?tree=Try&rev=7c1d83842950

Comment 41

[Boris Zbarsky [:bzbarsky]]

> When the script loads in the HTTPS iframe, aRequestingContext gives us the HTTPS iframe
> as the principal, not the HTTP page.

Yes, of course.  Why were you expecting anything other than the window the script is going to run in here, in terms of principals?

> Bz, don't these results imply that the context is not the window the load is happening
> in?

The context is a node or a window, depending on what's being loaded, as documented in the IDL.  Of course you can always reach a window from the node, if the context is a node.

Comment 42

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Actually, I seem to recall aRequestingContext not being all that useful for
> loads in subframes: it's the window the load is happening in, not what
> started the load, iirc.
> 
> We should really just pass through the thing CSP needs here, which is none
> of the current things being passed in.


Hi BZ,

In comment 33, you seemed to imply that we could not depend on aRequestingContext for subframe loads.  For resources loaded in frames, we want/expect the URI of the requesting frame.  From there we have code that traverses up the frames to the root document already built into nsMixedContentBlocker.

We did some testing[1], and it seems to be behaving the way we want and expect.  Is there something we are missing here?  Can you clarify your comment 33?  I think there is some confusion on the meaning of "the window the load is happening in".

[1] If we have an HTTP page A with an HTTPS iframe B that loads an HTTP script C, the load associated with the script C has an aRequestingContext of the HTTPS iframe B.  nsMixedContentBlocker uses the scheme from the HTTPS iframe B to determine that the script C is mixed content and reject it.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> For resources loaded in frames, we want/expect the URI of the requesting frame. 

That's not the issue.

The issue is what happens for loads of the subframe document itself (or for that matter in a toplevel document).

There are multiple ways this can happen:

1)  Change of the @src attribute of the <iframe> element.
2)  <a target="whatever"> or <form target="whatever"> targeting the iframe.
3)  A direct window.location set on the relevant DOM Window.

I believe aRequestingContext will currently be the <iframe> element in question for all of these loads.  For the toplevel document case it will be the <xul:browser> for desktop firefox; probably the Window inside the <iframe> in e10s situations.

On the other hand, aRequestingPrincipal in the above three cases is the principal of the <iframe> element, the principal of the document containing the <a> or <form>, and the principal of the script that set window.location, respectively....

Comment 44

[Tanvi Vyas[:tanvi]]

In comment 6 I started talking about the ramifications of this bug.  Here I will attempt to lay out the consequences of this bug again and in more detail - 

* We may incorrectly show mixed display content warnings on HTTP pages. This means different things on FF 23, 24, 25, and 26 as far as what the user might see.

- In Firefox 24, the user's site identity box will say:
Your connection to this website is only partially encrypted, and does not prevent eavesdropping.
instead of:
Your connection to this website is not encrypted.

- In Firefox 25, the user's site identity box will say:
The connection to this website is not fully secure because it contains unencrypted elements (such as images).
instead of:
Your connection to this website is not encrypted.

- In Firefox 26 and 27, the user will see a grey warning triangle instead of a globe!  The site identity box will say:
The connection to this website is not fully secure because it contains unencrypted elements (such as images).
instead of:
Your connection to this website is not encrypted.

Note that the consequences to Firefox 24 and 25 are not a big deal since it is only shown when the user clicks on the globe.  In Firefox 26 and 27 this is a bigger problem because we have changed the globe to a grey triangle, which is more alarming.  The triangle could cause users to click on the identity box and read this inaccurate message.



* In Firefox 23+, we may incorrectly block active content on HTTP pages and show the shield to the user.

There are a few situations where this can occur:
1) HTTP page has HTTPS css that tries to load external HTTP font.  The HTTP font will be blocked by MCB.  Since most sites still work properly without external fonts, this won't be a usability issue to users but may confuse them.  Example: 
http://people.mozilla.org/~tvyas/darkreading-font.html

2) An HTTPS web worker running on an HTTP page that tries to load an HTTP active resource.  The HTTP active resource might will be blocked by MCB (Have not tested to confirm).

3) An HTTPS object running in an HTTP page that tries to create a document inside itself.  HTTP active resource loads from the document the object created might be blocked by MCB (Have not tested to confirm).

4) An HTTP page with an SVG that loads HTTP external resources.  In this case, we will look at the location of the SVG to determine the scheme.  I'm not quite sure how that works and how we would end up with a blocked HTTP external resource.  Again, I haven't attempted to test this.

More analysis into BZ's comment 43 may yield a few more situations where we have an HTTP page incorrectly invoke the MCB.



* For HTTPS pages, there are no ramifications that I can think of.

Comment 45

[Tanvi Vyas[:tanvi]]

Hi BZ,

I tried to validate what you said in comments 33 and 43.  The comments implied that aRequestingContext would not be useful for MCB in various situations with iframes.  After some testing, I don't think that is true.  Here is what I found.

For case 1) Change of the @src attribute of the <iframe> element.  You hypothesized that the uri associated with aRequestingContext and aRequestPrincipal would be the frame who's location was being changed.  Testing concluded that the uri associated with aRequestingContext, aRequestPrincipal, and aRequestingLocation is that of the parent page containing the frame (and not the frame itself).  See http://pastebin.mozilla.org/3190630 for the urls of the testcases, a summary of the tescases' behavior and the debug output.  For MCB, this means that any three of parameters passed it would work in this situation.

For case 2) <a target="whatever"> or <form target="whatever"> targeting the iframe.  You hypothesized that the uri associated with aRequestingContext would be the iframe who's been targeted and that the uri associated with aRequestPrincipal would be the document that contained the <form target>.  Testing concluded that the uri associated with aRequestingContext was the parent of the targeted iframe (not the frame itself).  The uri associated with aRequestPrincipal and aRequestingLocation is the document that contained the <form target> (as you guessed).  See http://pastebin.mozilla.org/3190818 for the urls of the testcases, a summary of the testcases' behavior and the debug output.  For MCB, this means that aRequestingContext is the right parameter to use in this case.

For case 3)  A direct window.location set on the relevant DOM Window.  You hypothesized that the uri associated with aRequestingContext would be the iframe/window who's location is changing and that the uri associated with aRequestPrincipal would be the script that set the window.location.  Testing concluded that the uri associated with aRequestingContext is that of the direct parent (not the frame itself) and the uri associated with aRequestingLocation and aRequestPrincipal is the original page/frame (not the script that initiated the window.location change). See http://pastebin.mozilla.org/3190805 for the urls of the testcases, a summary of the testcases' behavior and the debug output.  For MCB, this means that aRequestingContext  is the right parameter to use in this case.
Note that in the case where the window.location is set on the toplevel document, there is no uri associated with aRequestingContext (probably xul browser as you suggested).  For MCB, that is fine since it is TYPE_DOCUMENT and hence we accept the load and return before we even check the context.

Given the above, we have not identified a case where using the URI associated with aRequestingContext in MCB would provide a false positive or false negative (BZ, is there a test case I am missing?).  With the current implementation (using aRequestingLocation and aRequestPrincipal) we do have cases where the MCB behaves incorrectly.

The long term solution is to really rewrite Content Policy, but this not something that can be done in any reasonable amount of time.  Another shorter term but still multi-week solution would be to hack up Content Policy by adding another parameter.  I think the right immediate solution is to rely solely on aRequestingContext to provide Mixed Content Blocker with the owning documents URI, and ignore aRequestPrincipal and aRequestingLocation all together (since we have documented cases where aRequestPrincipal and aRequestingLocation are unreliable).

Comment 46

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #40)
> Created attachment 811446 [details] [diff] [review]
> bug_909920_mcb_warning_on_http_site_comment_32.patch
> 
> Pushed the idea from comment 32 to try:
>   https://tbpl.mozilla.org/?tree=Try&rev=7c1d83842950

Christoph, you have implemented the suggestion to just use aRequestingContext in the above patch and pushed it to try.  As we've discussed, try shows a number of failures.  Can you dig into these a bit and figure out why try is failing?  A number of CSP tests are failing (for example) but this patch doesn't change the CSP code so I'm curious as to why these would be failing.  Maybe there is a check we are missing in the code.

Thanks!

Comment 47

[Boris Zbarsky [:bzbarsky]]

> For case 1) Change of the @src attribute of the <iframe> element.  You hypothesized that
> the uri associated with aRequestingContext and aRequestPrincipal would be the frame
> who's location was being changed. 

No, I said it would be the <iframe> element whose src attribute got set.

> For case 2) <a target="whatever"> or <form target="whatever"> targeting the iframe.
> You hypothesized that the uri associated with aRequestingContext would be the iframe
> who's been targeted 

No, I said it would be the <iframe> element in question.

> You hypothesized that the uri associated with aRequestingContext would be the
> iframe/window who's location is changing 

Again, no.

> and the uri associated with aRequestingLocation and aRequestPrincipal is the original
> page/frame (not the script that initiated the window.location change)

I believe it's actually the URI of the script entry point on the stack, which you're right is not the same as the script that did the location change.

For purposes of mixed-content detection, I think you're right: using aRequestingContext will do the right thing here.  For CSP, it's unclear.

Comment 48

[Alex Keybl [:akeybl]]

After this week, it'll be difficult to make the case for uplift to FF25 (fyi).

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Since there are cases where aRequestingLocation and aRequestPrincipal are definitely not the owning document, we tried to completely ignore them by extracting the requestingLocation in the following order:
[1] from the aRequestingContext, either extracting 
    a) the node's principal, or the
    b) script object's principal
[2] If aRequestingContext yields a principal but no location, we check if its
    a) the system principal, or
    b) an expanded principal (for content scripts from addons), and
    if it isn't, we reject the load.

This modification causes mochi tests to fail, in particular:
[i] ShouldLoad is sometimes called twice (see [link1]), where we can not extract a node's principal
    and also not the script object's principal [see 1a, 1b] for speculative loads, which causes this
    patch to reject the load. Of course we can not just block speculative loads, therefore we have to
    find an interim solution till we can fix the API. Unfortunately there is no possibility in
    ShouldLoad to distinguish between speculative and normal loads. I would suggest using the argument
    aRequestingLocation, right after case [1b], which allows speculative loads to succeed, in case
    they do not otherwise violate the MCB policy.

[ii] ShouldLoad for the CSP reporting feature is called without a context (see [link 2]).
     I guess we could change the call site, but on the other hand we might need to use the argument
     aReqeustingLocation for case [i] anyway, so we might use it for the reporting feature of CSP
     as well.

To sum it up, I suggest we introduce case:
[1c] If the requestingLocation is still null after [1b], we get it from the argument
     aRequestingLocation as an interim solution. This change would leave the argument
     aRequestingPrincipal unused in shouldLoad in MCB.

[link1] https://bugzilla.mozilla.org/show_bug.cgi?id=839235
[link2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#456

Comment 50

[Tanvi Vyas[:tanvi]]

I'm not seeing the differences here.  I think we are saying the same thing.

(In reply to On vacation Oct 12 - Oct 27 from comment #47)
> > For case 1) Change of the @src attribute of the <iframe> element.  You hypothesized that
> > the uri associated with aRequestingContext and aRequestPrincipal would be the frame
> > who's location was being changed. 
> 
> No, I said it would be the <iframe> element whose src attribute got set.

<iframe> element whose src attribute got set is the iframe who's location is being changed

> 
> > For case 2) <a target="whatever"> or <form target="whatever"> targeting the iframe.
> > You hypothesized that the uri associated with aRequestingContext would be the iframe
> > who's been targeted 
> 
> No, I said it would be the <iframe> element in question.

The <iframe> element in question is the <iframe> who is being targeted.

> 
> > You hypothesized that the uri associated with aRequestingContext would be the
> > iframe/window who's location is changing 
> 
> Again, no.
>

Comment 51

[Tanvi Vyas[:tanvi]]

I talked to Christoph and went over his proposal in comment 49.  I think it is a good idea, as long as we heavily comment the code where we use aRequestingLocation, explaining why we have to use it (speculative loads and TYPE_CSP_REPORT).

I also proposed that we use aRequestingLocation as step [3] instead of after [1b].

So:

[1] from the aRequestingContext, either extracting 
    a) the node's principal, or the
    b) script object's principal
[2] If aRequestingContext yields a principal but no location, we check if its
    a) the system principal, or
    b) an expanded principal (for content scripts from addons), and
    if it isn't, we reject the load.
[3] If aRequestingContext does not yeild a principal or a location, assume we have a speculative load or TYPE_CSP_REPORT and use aRequestingLocation

Comment 52

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #44)
> Note that the consequences to Firefox 24 and 25 are not a big deal since it
> is only shown when the user clicks on the globe.  In Firefox 26 and 27 this
> is a bigger problem because we have changed the globe to a grey triangle,
> which is more alarming.  The triangle could cause users to click on the
> identity box and read this inaccurate message.
> 

Given the above, we should target this for FF 26.  Firefox 26 is currently in aurora.  It will move to beta on 10/28.  From what I've observed in the release process, about 3 weeks later it will probably be too late to uplift and get it into Firefox 26 before it goes to the stable release.

Ideally we should try and fix this bug before 10/28, so we have it in nightly (FF 27) and only need to uplift to aurora (FF 26).

Comment 53

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site_tests.patch

Incorporated Tanvi's suggestions from Comment 28 and did a rebase!

Comment 54

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

(In reply to Tanvi Vyas [:tanvi] from comment #51)
> I talked to Christoph and went over his proposal in comment 49.  I think it
> is a good idea, as long as we heavily comment the code where we use
> aRequestingLocation, explaining why we have to use it (speculative loads and
> TYPE_CSP_REPORT).
> 
> I also proposed that we use aRequestingLocation as step [3] instead of after
> [1b].

I agree, even though it does not actually affect the outcome of shouldLoad, we introduced the special case handling for speculative loads and TYPE_CSP_REPORT as step [3]. I think we are more explicit on how/why/and in what order we process the requesting location by introducing this additional/separate step. Thanks Tanvi, for pointing that out!

Comment 55

[Tanvi Vyas[:tanvi]]

Comment on attachment 817430 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site.patch

Christoph, please push this to try.  If all looks good, then r? to smaug.


>Bug 909920 - Mixed content warning should not show on a HTTP site
>
>diff --git a/content/base/src/nsMixedContentBlocker.cpp b/content/base/src/nsMixedContentBlocker.cpp
>--- a/content/base/src/nsMixedContentBlocker.cpp
>+++ b/content/base/src/nsMixedContentBlocker.cpp
>+  // 3) use aRequestingLocation (for two special cases:
>+  //                             - speculative loads, where shouldLoad is called twice (bug 839235),
 - speculative loads, where shouldLoad is called twice (bug 839235) and the first speculative call does not include a context


>+  nsCOMPtr<nsIURI> requestingLocation;
>+  if (principal) {
>+    principal->GetURI(getter_AddRefs(requestingLocation));
>+  }
>+
>+  // 2a,b) Use the principal to allow pseudo-privileged addon code.
>+  if (!requestingLocation) {
The comment for this case says  "2) if aRequestingContext yields a principal but no location," so perhaps this if should be
if (principal && !requestingLocation)

We get away without doing this because do_QueryInterface is null safe (if I recall correctly).  But in the case that there is no principal, we should just skip looking for an expanded or system principal.
>+    // If content scripts from an addon are causing this load, they have an
>+    // ExpandedPrincipal instead of a Principal. This is pseudo-privileged code, so allow
>+    // the load. Or if this is system principal, allow the load.
>+    nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(principal);

Comment 56

[Tanvi Vyas[:tanvi]]

Comment on attachment 817424 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site_tests.patch

Per comment 28, did you look into mochitests for fonts?  Can we add a font based test?  Even if external fonts are not detectable by javascript, we can check the security state of the page to detect mixed content font loads/blocks.

>Bug 909920 - Mixed content warning should not show on a HTTP site - Tests
>diff --git a/browser/base/content/test/general/test_no_mcb_on_http_site.html b/browser/base/content/test/general/test_no_mcb_on_http_site.html
>+  	var newValue = "Verifying MCB does not trigger warning/error for http page - https css - http image";
>+  	document.getElementById("testDiv").innerHTML = newValue;
Again, what is the purpose of this?  To ensure all tests are completed before test1() is complete?

>+  }
>+</script>
>+</head>
>+<body onload="checkLoadStates()">
>+  <div class="testDiv" id="testDiv">
>+    Testing MCB does not trigger warning/error for http page - https css - http image
Testing MCB does not trigger warning/error for an http page with https css that includes http image

r- until we resolve whether to add a test for fonts / mixed active content.

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

Testcase:
  http://people.mozilla.org/~tvyas/darkreading-font.html

Description:
The |http|-page includes an |https|-css file, which imports (using @import) another |http|-css file.
This imported |http|-css file loads a font (*.woff). For loading the font we do not call shouldLoad().

More precise shouldLoad is called for:
  http://fonts.googleapis.com/css?family=Inconsolata
but, not for:
 (http://themes.googleusercontent.com/static/fonts/inconsolata/v6/BjAYBlHtW3CJxDcjzrnZCIbN6UDyHWBl620a-IRfuBk.woff

Aren't we missing a call to shouldLoad for the *.woff file?

Comment 58

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site_tests.patch

(In reply to Tanvi Vyas [:tanvi] from comment #56)
> Per comment 28, did you look into mochitests for fonts?  Can we add a font
> based test?  Even if external fonts are not detectable by javascript, we can
> check the security state of the page to detect mixed content font
> loads/blocks.

Hi Tanvi, I added testcases for fonts. One that directly uses the font in the css file (Test 2) and another one, that uses |@import| to include another css files which pulls the font.
 
> >Bug 909920 - Mixed content warning should not show on a HTTP site - Tests
> >diff --git a/browser/base/content/test/general/test_no_mcb_on_http_site.html b/browser/base/content/test/general/test_no_mcb_on_http_site.html
> >+  	var newValue = "Verifying MCB does not trigger warning/error for http page - https css - http image";
> >+  	document.getElementById("testDiv").innerHTML = newValue;
> Again, what is the purpose of this?  To ensure all tests are completed
> before test1() is complete?

Sorry for not answering this earlier. You where right, initially we didn't need this additional check, but since I added two more testcases which are loaded in sequence in the same tab, we need this test to make sure that the previous test finished before loading the next.
 
> >+  <div class="testDiv" id="testDiv">
> >+    Testing MCB does not trigger warning/error for http page - https css - http image
> Testing MCB does not trigger warning/error for an http page with https css
> that includes http image

Fixed that of course. Thanks for pointing that out.

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #57)
> Testcase:
>   http://people.mozilla.org/~tvyas/darkreading-font.html
> 
> Description:
> The |http|-page includes an |https|-css file, which imports (using @import)
> another |http|-css file.
> This imported |http|-css file loads a font (*.woff). For loading the font we
> do not call shouldLoad().
> 
> More precise shouldLoad is called for:
>   http://fonts.googleapis.com/css?family=Inconsolata
> but, not for:
>  (http://themes.googleusercontent.com/static/fonts/inconsolata/v6/
> BjAYBlHtW3CJxDcjzrnZCIbN6UDyHWBl620a-IRfuBk.woff
> 
> Aren't we missing a call to shouldLoad for the *.woff file?

Interestingly, it works correctly (shouldLoad is called in both cases) in the attached testcase, but I couldn't figure the difference between my testcase and Tanvi's testpage. Clearing the cache didn't solve the problem either.

Comment 60

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

(In reply to Tanvi Vyas [:tanvi] from comment #55)
> Comment on attachment 817430 [details] [diff] [review]
> bug_909920_mcb_warning_on_http_site.patch
> 
> Christoph, please push this to try.  If all looks good, then r? to smaug.

Tanvi, I did, and it seems we where missing another corner case.
XHR requests do not provide aRequestingContext, and also not a aRequestingLocation.

I updated the patch so that we are extracting the requestingLocation in the following order:
1) from the aRequestingContext, either extracting
   a) the node's principal, or the
   b) script object's principal.
2) if aRequestingContext yields a principal but no location,
   we check if its the system principal.
3) Special case handling for:
   a) speculative loads, where shouldLoad is called twice (bug 839235)
      and the first speculative load does not include a context.
   b) TYPE_CSP_REPORT which does not provide a context.
   c) Content scripts from addons code, and XHR requests, which do not provide
      aRequestingContext, and also no aRequestingLocation. If aRequestPrincipal
      is an expanded principal, we allow the load.
4) If we still end up not having a requestingLocation, we reject the load.


Unfortunately it is not possible to ignore aRequestingLocation, and aRequestPrincipal completely, but we can move those special cases further down, trying to extract the requesting location from the context first. Only if cases 1) and 2) do not lead to success, we take the special case handling into account.

I pushed to try to verify that we pass all testcases:
  https://tbpl.mozilla.org/?tree=Try&rev=8cb678081312

Comment 61

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

As discussed on IRC, special case handling in 3c) is for addons code in general, not only for XHR requests. I updated the comments accordingly.

Comment 62

[Tanvi Vyas[:tanvi]]

Comment on attachment 818580 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site.patch

Only comment changes to make things more clear.  Code looks good.


>diff --git a/content/base/src/nsMixedContentBlocker.cpp b/content/base/src/nsMixedContentBlocker.cpp
>--- a/content/base/src/nsMixedContentBlocker.cpp
>+++ b/content/base/src/nsMixedContentBlocker.cpp

>-      }
>+  // Since there are cases where aRequestingLocation and aRequestPrincipal are definitely not the owning document,
>+  // we try to ignore them by extracting the requestingLocation in the following order:
>+  // 1) from the aRequestingContext, either extracting
>+  //    a) the node's principal, or the
>+  //    b) script object's principal.
>+  // 2) if aRequestingContext yields a principal but no location, we check if its the system principal.
Add: If it is, allow the load.

>+  // 3) Special case handling for:
>+  //    a) speculative loads, where shouldLoad is called twice (bug 839235) and the first
>+  //       speculative load does not include a context.
Add: In this case we use aRequestingLocation to set requestingLocation.

>+  //    b) TYPE_CSP_REPORT which does not provide a context.
In this case we use aRequestingLocation to set requestingLocation.
>+  //    c) Content scripts from addons code which do not provide aRequestingContext, and also
>+  //       not aRequestingLocation. If aRequestPrincipal is an expanded principal, we allow the load.
Change to:
Content scripts from addon code that do not provide aRequestingContext or aRequestingLocation, but do provide aRequestPrincipal.  If aRequestPrincipal is an expanded principal, we allow the load.

>+
>+  // 2) if aRequestingContext yields a principal but no location, we check if its a system principal.
>+  if (principal && !requestingLocation) {
>+    if (nsContentUtils::IsSystemPrincipal(principal)) {
>+      *aDecision = ACCEPT;
>+      return NS_OK;
>     }
>+  }

>+  // 3a,b) Special case handling for speculative loads and TYPE_CSP_REPORT.
Add: aRequestingContext doesn't exist, so we use aRequestingLocation.

>+  // Unfortunately we can not distinguish between speculative and normal loads here,
>+  // otherwise we could special case this assignment.
>+  if (!requestingLocation) {
>+    requestingLocation = aRequestingLocation;
>+  }

Comment 63

[Tanvi Vyas[:tanvi]]

Comment on attachment 818065 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site_tests.patch

Two questions about this patch.

1) Is there a timing issue?  The <script> in the html test files runs to check the security state.  Does the css and the font or image load before this happens?  Should we introduce a short setTimeout() to the beginning of the inline script just to make sure that the css/fonts/images have had time to load.  Or will the setTimeout also prevent them from loading?

2) I'm not sure why the test for test_no_mcb_on_http_site_font.html and the test for test_no_mcb_on_http_site_font2.html are behaving differently in the current code (before the patches in this bug).  I have 3 test cases:
http://people.mozilla.org/~tvyas/darkreading-font.html (similar to test_no_mcb_on_http_site_font.html)
http://people.mozilla.org/~tvyas/darkreading-font2.html (similar to test_no_mcb_on_http_site_font2.html)
http://people.mozilla.org/~tvyas/darkreading-font3.html (similar to test_no_mcb_on_http_site_font2.html)

darkreading-font.html shows the MCB sheild because aRequestingLocation is set to the css file.  Why isn't aRequestingLocation set to the css file in darkingreading-font2 or darkreading-font3?

Comment 64

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #63)
> 1) Is there a timing issue?  The <script> in the html test files runs to
> check the security state.  Does the css and the font or image load before
> this happens?  Should we introduce a short setTimeout() to the beginning of
> the inline script just to make sure that the css/fonts/images have had time
> to load.  Or will the setTimeout also prevent them from loading?

The use of:
>  <body onload="checkLoadStates()">
should take care of eventual timing issues, because it should only exeute the script once the web page has completely loaded all content (including images, script files, CSS files, etc.) (see also [1]).

[1] http://www.w3schools.com/jsref/event_onload.asp
 
> 2) I'm not sure why the test for test_no_mcb_on_http_site_font.html and the
> test for test_no_mcb_on_http_site_font2.html are behaving differently in the
> current code (before the patches in this bug).  I have 3 test cases:
> http://people.mozilla.org/~tvyas/darkreading-font.html (similar to
> test_no_mcb_on_http_site_font.html)
> http://people.mozilla.org/~tvyas/darkreading-font2.html (similar to
> test_no_mcb_on_http_site_font2.html)
> http://people.mozilla.org/~tvyas/darkreading-font3.html (similar to
> test_no_mcb_on_http_site_font2.html)
> 
> darkreading-font.html shows the MCB sheild because aRequestingLocation is
> set to the css file.  Why isn't aRequestingLocation set to the css file in
> darkingreading-font2 or darkreading-font3?

It seems, that the problem is related to the issue I documented in Comment 57.
For darkreading-font.html aContentLocation is:
  http://fonts.googleapis.com/css?family=Inconsolata
and aRequestingLocation is:
  https://people.mozilla.org/~tvyas/style2.css
which triggers a MCB-warning (displays the shield) when we load the additional CSS file (please note, it's the additional css file, not the font, that causes the warning).

For http://people.mozilla.org/~tvyas/darkreading-font2.html, where the font is directly included in the css, we do not actually call shouldLoad for the font itself. Hence no warning.

As questionend in Comment 57, I don't know why we do not call shouldLoad for the font (*.woff file)!

Comment 65

[Christoph Kerschbaumer [:ckerschb]]

> As questionend in Comment 57, I don't know why we do not call shouldLoad for
> the font (*.woff file)!

If we do not actually make use of a font, it's not loaded, hence no call to shouldLoad().
The new testcases:
  - http://people.mozilla.org/~tvyas/darkreading.html
  - http://people.mozilla.org/~tvyas/darkreading-style.html
  - http://people.mozilla.org/~tvyas/darkreading-font2.html

reflect those changes! Thanks Tanvi, for updating.

Comment 66

[Tanvi Vyas[:tanvi]]

Comment on attachment 818065 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site_tests.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #64)
> The use of:
> >  <body onload="checkLoadStates()">
> should take care of eventual timing issues, because it should only exeute
> the script once the web page has completely loaded all content (including
> images, script files, CSS files, etc.) (see also [1]).
> 
> [1] http://www.w3schools.com/jsref/event_onload.asp
>  
Ah, I missed that.  Thanks for pointing it out.  In that case, r+'ing this patch.


> > 2) I'm not sure why the test for test_no_mcb_on_http_site_font.html and the
> > test for test_no_mcb_on_http_site_font2.html are behaving differently in the
> > current code (before the patches in this bug).  ...
Christoph is investigating this inconsistent behavior and it may lead to the discovery of another bug, but the results don't change the implementation or tests in this bug.

Comment 67

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #62)
> Comment on attachment 818580 [details] [diff] [review]
> bug_909920_mcb_warning_on_http_site.patch
> 
> Only comment changes to make things more clear.  Code looks good.

I made those changes and pushed to try:
  https://tbpl.mozilla.org/?tree=Try&rev=fd6aa8ba3122

Once I get feedback from smaug, I will rebase and upload the patch again!

Comment 68

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

Comment on attachment 818580 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site.patch

Good that we get all the time more tests for  this stuff.

Comment 69

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site_tests.patch

Rebased and added reviewers to description. Carrying over r+ from tanvi and smaug.

Comment 70

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_909920_mcb_warning_on_http_site.patch

Rebased, modified the comments in the code following Tanvi's suggestions from Comment 62, and added reviewers to the description. Carrying over r+ from tanvi and smaug.

Comment 71

[Tanvi Vyas[:tanvi]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7dcf8411463
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2ee998c786

Comment 72

[Tanvi Vyas[:tanvi]]

Comment on attachment 822436 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site_tests.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 834836
User impact if declined:  User impacted is described in detail in comment 44.  Specifically for firefox-aurora-26, when a user encounters an HTTP page with HTTPS CSS that includes an HTTP image, the user will see a grey warning triangle instead of a globe!  The site identity box will say:
The connection to this website is not fully secure because it contains unencrypted elements (such as images).
instead of:
Your connection to this website is not encrypted.

Since we have changed the globe to a grey triangle, it is more alarming and hence it is more likely that users will click on it and read this inaccurate message.
Testing completed (on m-c, etc.): try push - https://tbpl.mozilla.org/?tree=Try&rev=fd6aa8ba3122.  Just landed in inbound.
Risk to taking this patch (and alternatives if risky): This changes the way the data source MCB uses to decide whether to block or allow a load.  The risk to taking on this patch is new false positive or a false negative cases that have not been identified through our analysis and testing.  We did try and cover as many cases as we could think of, with the help of BZ.  But Content Policy is messy, and there is potential that there is a case that we did not think of.
String or IDL/UUID changes made by this patch: None

Comment 73

[Tanvi Vyas[:tanvi]]

Comment on attachment 822437 [details] [diff] [review]
bug_909920_mcb_warning_on_http_site.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 834836
User impact if declined:  User impacted is described in detail in comment 44.  Specifically for firefox-aurora-26, when a user encounters an HTTP page with HTTPS CSS that includes an HTTP image, the user will see a grey warning triangle instead of a globe!  The site identity box will say:
The connection to this website is not fully secure because it contains unencrypted elements (such as images).
instead of:
Your connection to this website is not encrypted.

Since we have changed the globe to a grey triangle, it is more alarming and hence it is more likely that users will click on it and read this inaccurate message.
Testing completed (on m-c, etc.): try push - https://tbpl.mozilla.org/?tree=Try&rev=fd6aa8ba3122.  Just landed in inbound.
Risk to taking this patch (and alternatives if risky): This changes the way the data source MCB uses to decide whether to block or allow a load.  The risk to taking on this patch is new false positive or a false negative cases that have not been identified through our analysis and testing.  We did try and cover as many cases as we could think of, with the help of BZ.  But Content Policy is messy, and there is potential that there is a case that we did not think of.
String or IDL/UUID changes made by this patch: None

Comment 74

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #66)
> > > 2) I'm not sure why the test for test_no_mcb_on_http_site_font.html and the
> > > test for test_no_mcb_on_http_site_font2.html are behaving differently in the
> > > current code (before the patches in this bug).  ...
> Christoph is investigating this inconsistent behavior and it may lead to the
> discovery of another bug, but the results don't change the implementation or
> tests in this bug.

I filed a follow up bug for this incosistency issue in shouldLoad:
  https://bugzilla.mozilla.org/show_bug.cgi?id=931107

Comment 75

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f7dcf8411463
https://hg.mozilla.org/mozilla-central/rev/fc2ee998c786

Comment 76

[Ed Morley [:emorley]]

We can land without tests, or else need a branch patch (unfortunately I have too many uplifts to land asap to rebase each by hand)

applying bug_909920_mcb_warning_on_http_site_tests.patch
unable to find 'browser/base/content/test/general/browser.ini' for patching
2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug_909920_mcb_warning_on_http_site_tests.patch

Comment 77

[Tanvi Vyas[:tanvi]]

Attachment: Tests for Aurora - Bug909920-test-final-aurora.patch

Here is a patch of the tests for aurora.

(In reply to Ed Morley [:edmorley UTC+1] from comment #76)
> We can land without tests, or else need a branch patch (unfortunately I have
> too many uplifts to land asap to rebase each by hand)
> 
> applying bug_909920_mcb_warning_on_http_site_tests.patch
> unable to find 'browser/base/content/test/general/browser.ini' for patching
> 2 out of 2 hunks FAILED -- saving rejects to file
> browser/base/content/test/general/browser.ini.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh
> bug_909920_mcb_warning_on_http_site_tests.patch

Comment 78

[Tanvi Vyas[:tanvi]]

Checkin to aurora needed.  Not sure how to specify that using keywords.

Comment 79

[Tanvi Vyas[:tanvi]]

Checked into aurora: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c006c6b7dd7
https://hg.mozilla.org/releases/mozilla-aurora/rev/1015675d91d0

Comment 80

[Ekanan Ketunuti]

Reproduced on:
* nightly 27.0a1 20131024030204 build
* aurora 26.0a2 20131027004004 build

Verified on:
* nightly 27.0a1 20131026030205 build
* aurora 26.0a2 20131028114808 build
* beta 26.0b1 20131028225529 build