Tanvi Vyas[:tanvi] Reporter
	Description • 9 years ago

https://w3c.github.io/webappsec/specs/mixedcontent/#strict-checking

Allow a website to opt-in to:
1) blocking all mixed content (active and passive)
2) extending this restriction to all descendant frames on the site
3) providing no user override capabilities (shield icon doesn't show up and user can't disable protection)

The website opts in by setting the strict-mixed-content-checking directive.  Note that associated value is empty.

This should happen regardless of the about:config mixed content prefs the user may have changed.  So if a user disables mixed active content blocking, we won't honor the user in this case.  The website has explicitly told the browser not to allow mixed content of any kind.

As far as implementation to meet requirement 2, we need to add a boolean flag to nsDocument to indicate whether mixed strict mode is enforced.  When CSP see's this directive set on a dcoument, it should set the flag to true.  Then whenever a frame is created, it will first check if its parent is in strict mixed mode, and if it is, it will set that flag on itself as well.

	Daniel Veditz [:dveditz]
	Comment 1 • 8 years ago

The directive is now called "block-all-mixed-content" in the current spec version. Adding that to the bug summary for easier searching

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 2 • 8 years ago

Update: Mozilla decided to implement that directive this quarter!

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 7 • 8 years ago

Potentially we have to update some web-platform tests, let's have a look at TRY:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda141573fdb

	Tanvi Vyas[:tanvi] Reporter
	Comment 8 • 8 years ago

Comment on attachment 8722546 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>--- a/dom/security/nsMixedContentBlocker.cpp
>+++ b/dom/security/nsMixedContentBlocker.cpp
>@@ -627,37 +627,47 @@ nsMixedContentBlocker::ShouldLoad(bool a
>     *aDecision = REJECT_REQUEST;
>     return NS_OK;
>   }
>   if (!parentIsHttps) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
> 
>+  // Determine if the rootDoc is https and if the user decided to allow Mixed Content
>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>+  NS_ENSURE_TRUE(docShell, NS_OK);
>+
>+  // The page might have set the CSP directive 'block-all-mixed-content' which
>+  // should block not only active mixed content loads but in fact all mixed content
>+  // loads, see https://www.w3.org/TR/mixed-content/#strict-checking
>+  // Let's simplify the approach and block all non https loads in case the CSP
>+  // directive is present.
>+  bool isHttpsScheme = false;
>+  rv = aContentLocation->SchemeIs("https", &isHttpsScheme);

At this point, we know the top level page is https and we know the subresource is not "secure" by the protocol flags we checked.  So we shouldn't need another ContentLocation check.  We already returned with an accept decision for secure schemes - http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#535

	Tanvi Vyas[:tanvi] Reporter
	Comment 9 • 8 years ago

Comment on attachment 8722546 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

>diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp
>--- a/dom/base/nsDocument.cpp
>+++ b/dom/base/nsDocument.cpp
>@@ -1408,16 +1408,18 @@ static already_AddRefed<mozilla::dom::No
> 
> // ==================================================================
> // =
> // ==================================================================
> nsIDocument::nsIDocument()
>   : nsINode(nullNodeInfo),
>     mReferrerPolicySet(false),
>     mReferrerPolicy(mozilla::net::RP_Default),
>+    mBlockAllMixedContent(false),
>+    mBlockAllMixedContentPreloads(false),
What is a case where one of these would be true but the other would be false?

>@@ -2543,23 +2545,28 @@ nsDocument::StartDocumentLoad(const char
>   // The CSP directive upgrade-insecure-requests not only applies to the
>   // toplevel document, but also to nested documents. Let's propagate that
>   // flag from the parent to the nested document.
>   nsCOMPtr<nsIDocShellTreeItem> treeItem = this->GetDocShell();
>   if (treeItem) {
>     nsCOMPtr<nsIDocShellTreeItem> sameTypeParent;
>     treeItem->GetSameTypeParent(getter_AddRefs(sameTypeParent));
>     if (sameTypeParent) {
>-      mUpgradeInsecureRequests =
>-        sameTypeParent->GetDocument()->GetUpgradeInsecureRequests(false);
>+      nsIDocument* doc = sameTypeParent->GetDocument();
>+      mBlockAllMixedContent = doc->GetBlockAllMixedContent(false);
>+      // if the parent document makes use of block-all-mixed-content
>+      // then subdocument preloads should always be blocked.
Again, not understanding the difference here.  If the parent document makes use of block-all-mixed-content, then mixed content in subdocuments should always be blocked, whether they are regular loads or preloads.  

>+      mBlockAllMixedContentPreloads =
>+        mBlockAllMixedContent || doc->GetBlockAllMixedContent(true);
>+
>+      mUpgradeInsecureRequests = doc->GetUpgradeInsecureRequests(false);



>diff --git a/dom/interfaces/security/nsIContentSecurityPolicy.idl b/dom/interfaces/security/nsIContentSecurityPolicy.idl
>--- a/dom/interfaces/security/nsIContentSecurityPolicy.idl
>+++ b/dom/interfaces/security/nsIContentSecurityPolicy.idl
>@@ -16,17 +16,17 @@ interface nsIURI;
>  * nsIContentSecurityPolicy
>  * Describes an XPCOM component used to model and enforce CSPs.  Instances of
>  * this class may have multiple policies within them, but there should only be
>  * one of these per document/principal.
>  */
> 
> typedef unsigned short CSPDirective;
> 
>-[scriptable, builtinclass, uuid(b3c4c0ae-bd5e-4cad-87e0-8d210dbb3f9f)]
>+[scriptable, builtinclass, uuid(5b178330-a8a7-4e19-8db3-805150800a6d)]
We no longer need to rev the uuid for idl changes.

	Tanvi Vyas[:tanvi] Reporter
	Comment 10 • 8 years ago

Comment on attachment 8722546 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

See also comments 8 and 9.


>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>--- a/dom/security/nsMixedContentBlocker.cpp
>+++ b/dom/security/nsMixedContentBlocker.cpp
>@@ -627,37 +627,47 @@ nsMixedContentBlocker::ShouldLoad(bool a
>     *aDecision = REJECT_REQUEST;
>     return NS_OK;
>   }
>   if (!parentIsHttps) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
> 
>+  // Determine if the rootDoc is https and if the user decided to allow Mixed Content
This comment should stay where it was originally.

Also consider these two scenarios:
1) Root doc is https, has no csp.  Subdocument is https and has a csp with block all mixed content.  Subdocument tries to load http content.  Is it blocked?  If we only check the root and not the other ancestors, then we may end up allowing the mixed content because we only check the rootdocument policy with your code change.  We should probably go up the ancestor chain.

2) Root doc is http, has no csp.  Subdocument is https and has a csp with block all mixed content.  Root doc navigates subdocument (or user clicks on a link in the subdocument and navigates it) to an http location.  Navigation shouldn't be blocked since root doc is http and hence this isn't mixed content.
This might work out okay with the current code - since it is only checking the rootdocs csp, which doesn't exist.  But it something to be careful of if you go through the ancestor chain as suggested in #1.

You should also add an additional reviewer for the CSP additions - Dan, Kate, or Sid?  I've looked them over, but those folks know that code better.

	Tanvi Vyas[:tanvi] Reporter
	Comment 11 • 8 years ago

Comment on attachment 8722547 [details] [diff] [review]
bug_1122236_block_all_mixed_content_gcli_updates.patch

I'm not the best reviewer for this, though it seems pretty straight forward.

	Tanvi Vyas[:tanvi] Reporter
	Comment 12 • 8 years ago

Comment on attachment 8722548 [details] [diff] [review]
bug_1122236_block_all_mixed_content_tests.patch

Test looks pretty good.  Add a script case as well, since we have different behavior for active and passive content.

>diff --git a/dom/security/test/csp/file_block_all_mcb.sjs b/dom/security/test/csp/file_block_all_mcb.sjs
>+  "</html>";
>+
>+function handleRequest(request, response)
>+{
>+  // avoid confusing cache behaviors
>+  response.setHeader("Cache-Control", "no-cache", false);
>+ 
>+  var queryString = request.queryString;
>+
>+  dump("\n\n HANDLEREQUEST: " + queryString + "\n");
>+
Remove dump once we are closer to a final implementation patch


>diff --git a/dom/security/test/csp/test_block_all_mixed_content.html b/dom/security/test/csp/test_block_all_mixed_content.html
>+
>+/* Description of the tests:
>+ * Test 1:
>+ * We load mixed display content in a frame using the CSP
>+ * directive 'block-all-mixed-content' and observe that the image is blocked.
>+ *
>+ * Test 2:
>+ * We load mixed display content in a frame and observe
>+ * that the image is loaded.
Comment on test 2's csp policy

>+ *
>+ * Test 3:
>+ *
>+ * Test 4:
>+ *
Update Test 3 and 4 comments.


>+var curTest;
>+var counter = -1;
>+
>+function checkResults(result) {
>+  dump ("result: " + result + "\n");
Remove dump once we are closer to a final implementation patch

	Tanvi Vyas[:tanvi] Reporter
	Comment 13 • 8 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #10)
> 2) Root doc is http, has no csp.  Subdocument is https and has a csp with
> block all mixed content.  Root doc navigates subdocument (or user clicks on
> a link in the subdocument and navigates it) to an http location.  Navigation
> shouldn't be blocked since root doc is http and hence this isn't mixed
> content.
> This might work out okay with the current code - since it is only checking
> the rootdocs csp, which doesn't exist.  But it something to be careful of if
> you go through the ancestor chain as suggested in #1.
> 

Looks like you have covered this in the frame navigation test case.  Great!

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 14 • 8 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #8)
> >+  bool isHttpsScheme = false;
> >+  rv = aContentLocation->SchemeIs("https", &isHttpsScheme);
> 
> At this point, we know the top level page is https and we know the
> subresource is not "secure" by the protocol flags we checked.  So we
> shouldn't need another ContentLocation check.

Ah I see, that makes sense, we can remove that additional check then.

(In reply to Tanvi Vyas [:tanvi] from comment #9)
> >+    mBlockAllMixedContent(false),
> >+    mBlockAllMixedContentPreloads(false),
> What is a case where one of these would be true but the other would be false?

Consider the following (pseudo code) scenario of an https site:

<head>
doc.write(“<—“);
<meta csp = “block-all-mixed-content”>
</head>
<body>
<img src=“http://foo.com/bar.jpg”>

In that case the preloader would generate a preloadCSP which would then block the image load within the preloader since it’s mixed content and the CSP directive is set. Once we bind that piece of code to the tree we would figure that the meta-csp does actually *not* apply to the page because of the doc.write() comments out the meta-csp. In that case mBlockAllMixedContentPreloads would be true but mBlockAllMixedContent would still be false because in fact we shouldn’t block the image from loading.

(In reply to Tanvi Vyas [:tanvi] from comment #10)
> Also consider these two scenarios:
> 1) Root doc is https, has no csp.  Subdocument is https and has a csp with
> block all mixed content.  Subdocument tries to load http content.  Is it
> blocked?

Yes, that would be blocked, because aRequestingContext would be the subdocument and |NS_CP_GetDocShellFromContext(aRequestingContext);| would return the dochsell where the document of that docshell has a principal that holds the CSP including the directive ‘block-all-mixed-content’.

> If we only check the root and not the other ancestors, then we may
> end up allowing the mixed content because we only check the rootdocument
> policy with your code change.  We should probably go up the ancestor chain.

We don’t only look at the root. If you look at the changes at nsDocument, you’ll see that we propagate the mBlockAllMixedContent flag all the way down from the outer most document all the way to the most nested iframe. If any document on the way holds the CSP directive we would start blocking nested mixed content subresource loads.

> 2) Root doc is http, has no csp.  Subdocument is https and has a csp with
> block all mixed content.  Root doc navigates subdocument (or user clicks on
> a link in the subdocument and navigates it) to an http location.  Navigation
> shouldn't be blocked since root doc is http and hence this isn't mixed
> content.

Exactly, that is one of the testcases I added, see:
bug_1122236_block_all_mixed_content_test_frame_navigation.patch 

> You should also add an additional reviewer for the CSP additions - Dan,
> Kate, or Sid?  I've looked them over, but those folks know that code better.

Most certainly, let’s ask Kate in addition to your review.

(In reply to Tanvi Vyas [:tanvi] from comment #12)
> Comment on attachment 8722548 [details] [diff] [review]
> bug_1122236_block_all_mixed_content_tests.patch
> 
> Test looks pretty good.  Add a script case as well, since we have different
> behavior for active and passive content.

Wouldn’t the script always be blocked anyway since it’s mixed active content? I don’t think it makes sense to add a testcase which is always blocked even if ‘block-all-mixed-content’ is not used. If you still insist, I can add that test, but I don't think it provides any additional test coverage other than the two tests I already added. If you are fine with the test coverage we have, then please finish the review and I'll remove the |dump| statements once I upload the final patch.

I hope I answered all of your questions, if I missed something or something is unclear please let me know.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 15 • 8 years ago

Comment on attachment 8722547 [details] [diff] [review]
bug_1122236_block_all_mixed_content_gcli_updates.patch

Joe, can you have a look at those updates?

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 16 • 8 years ago

Comment on attachment 8722546 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

Hey Kate, wanna take a look the csp changes within dom/security?

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 17 • 8 years ago

Regarding adding our own tests: I forgot to mention that we are also able to enable web platform tests for 'block-all-mixed-content' which provide additional test coverage.

	Joe Walker [:jwalker]
	Comment 18 • 8 years ago

Comment on attachment 8722547 [details] [diff] [review]
bug_1122236_block_all_mixed_content_gcli_updates.patch

Review of attachment 8722547 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from a GCLI point of view, but I won't pretend to be able to give the security parts a solid review.

Thanks.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 8 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Potentially we have to update some web-platform tests, let's have a look at
> TRY:
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda141573fdb

Is it possible to see a new try run with the changes to the WPT manifests?

	David Burns :automatedtester
	Comment 20 • 8 years ago

Setting this as a blocker for bug 1123506.

Setting n-i to get an answer to comment 19

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 21 • 8 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #19)
> Is it possible to see a new try run with the changes to the WPT manifests?

Huh - I thought I included the wpt-changes patch - my bad - here we go:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24c73a9687bd

	Kate McKinley [:kmckinley, :Kate]
	Comment 22 • 8 years ago

Comment on attachment 8722546 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

Review of attachment 8722546 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ +20,5 @@
>   */
>  
>  typedef unsigned short CSPDirective;
>  
> +[scriptable, builtinclass, uuid(5b178330-a8a7-4e19-8db3-805150800a6d)]

Don't need to rev uuids anymore.

	Tanvi Vyas[:tanvi] Reporter
	Comment 23 • 8 years ago

This bug is pending my review, but I am out next week, so won't be able to look at this until the following week.

	Tanvi Vyas[:tanvi] Reporter
	Comment 24 • 8 years ago

Comment on attachment 8722546 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

> nsCSPParser::nsCSPParser(cspTokens& aTokens,
>                          nsIURI* aSelfURI,
>                          nsCSPContext* aCSPContext,
>                          bool aDeliveredViaMetaTag)
>- : mCurChar(nullptr)
>+ : mCurChar(nullptr)
Is this a white space change?


>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>--- a/dom/security/nsMixedContentBlocker.cpp
>+++ b/dom/security/nsMixedContentBlocker.cpp
>@@ -627,37 +627,47 @@ nsMixedContentBlocker::ShouldLoad(bool a
>     *aDecision = REJECT_REQUEST;
>     return NS_OK;
>   }
>   if (!parentIsHttps) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
> 
>+  // Determine if the rootDoc is https and if the user decided to allow Mixed Content
>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>+  NS_ENSURE_TRUE(docShell, NS_OK);
Leave the docshell part here, but move the comment back to where it was originally.

This makes much more sense after understanding the preload part.  Patch looks good except what's noted here and in comment 8.  Please make those changes and reflag for review.  I'll re-review just the nsMixedContentBlocker.cpp part.

	Tanvi Vyas[:tanvi] Reporter
	Comment 25 • 8 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> (In reply to Tanvi Vyas [:tanvi] from comment #12)
> > Comment on attachment 8722548 [details] [diff] [review]
> > bug_1122236_block_all_mixed_content_tests.patch
> > 
> > Test looks pretty good.  Add a script case as well, since we have different
> > behavior for active and passive content.
> 
> Wouldn’t the script always be blocked anyway since it’s mixed active
> content? I don’t think it makes sense to add a testcase which is always
> blocked even if ‘block-all-mixed-content’ is not used. If you still insist,
> I can add that test, but I don't think it provides any additional test
> coverage other than the two tests I already added. If you are fine with the
> test coverage we have, then please finish the review and I'll remove the
> |dump| statements once I upload the final patch.
>
Blocking of script depends on the mixed active content about:config pref.  You are right that the pref is true by default and hence blocked and so a mochitest plain for them is not that important.  Although, they are generally blocked with a slightly degraded lock and an override UI in the control center.  Which makes me realize that we should check that in the block-all case, the security UI doesn't get degraded and that we always have a green lock. i.e:
* a page that attempts to load mixed active content has a green lock instead of a green lock with a grey triangle
* a page that attempts to load mixed display content has a green lock instead of a grey lock with a yellow triangle
That is kind of the point of block-all-mixed-content (so that a page can embed third party content without worrying that the third party content will degrade their security), so it is important to test.  Especially with nsSecureBrowserUIImpl around with potential to wreak havoc.  This means you will need a browser chrome test though.  You may be able to use the assertMixedContentBlockingState() method with false values for activeLoaded, activeBlocked, and passiveLoaded - https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#789.  This currently won't properly check the passiveBlocked case though.

	Tanvi Vyas[:tanvi] Reporter
	Comment 26 • 8 years ago

Comment on attachment 8722549 [details] [diff] [review]
bug_1122236_block_all_mixed_content_test_frame_navigation.patch

Frame navigation test looks good.  r+ with the changes below.

>diff --git a/dom/security/test/csp/mochitest.ini b/dom/security/test/csp/mochitest.ini
>--- a/dom/security/test/csp/mochitest.ini
>+++ b/dom/security/test/csp/mochitest.ini
>@@ -149,16 +149,18 @@ support-files =
>   file_meta_element.html
>   file_meta_header_dual.sjs
>   file_docwrite_meta.html
>   file_doccomment_meta.html
>   file_docwrite_meta.css
>   file_docwrite_meta.js
>   file_multipart_testserver.sjs
>   file_block_all_mcb.sjs
>+  file_block_all_mixed_content_frame_navigation.html
>+  file_block_all_mixed_content_frame_navigation_inner_frame.html
The second file here has a misleading name.  Both files are actually "inner frames", the first is the original one and the second is the one we navigate to.  So maybe rename these to frame_navigation1 and frame_navigation2?

> [test_base-uri.html]
> [test_blob_data_schemes.html]
> [test_connect-src.html]
> [test_CSP.html]
> [test_allow_https_schemes.html]
> skip-if = buildapp == 'b2g' #no ssl support
> [test_bug663567.html]
>@@ -226,8 +228,9 @@ skip-if = buildapp == 'b2g' #investigate
> [test_child-src_worker_data.html]
> [test_child-src_worker-redirect.html]
> [test_child-src_iframe.html]
> [test_meta_element.html]
> [test_meta_header_dual.html]
> [test_docwrite_meta.html]
> [test_multipartchannel.html]
> [test_block_all_mixed_content.html]
>+[test_block_all_mixed_content_frame_navigation.html]

Please add the mcb tag to both of these mixed content / csp tests with:
tags = mcb

	Tanvi Vyas[:tanvi] Reporter
	Comment 27 • 8 years ago

Comment on attachment 8725089 [details] [diff] [review]
bug_1122236_block_all_mixed_content_wpt_test_updates.patch

I don't know anything about these web platform tests, but if all this is doing is enabling tests, then r+.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 28 • 8 years ago

Already got an r+ from kate on the CSP bits. Here is what I updated based on the comments:

No need to ref the uuid within the *.idl anymore.

>+ : mCurChar(nullptr)
> Is this a white space change?
Yes, something was off with the line endings.

> Leave the docshell part here, but move the comment back to where it was originally.
I did that, but the comment seems a little lost where it is now. Up to you.

Further I updated the comment in nsMixedContentBlocker to explain why it’s safe to bail early at this point.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 29 • 8 years ago

Here is what I updated based on the comments:
> Remove dump once we are closer to a final implementation patch
Done.

> Update Test 3 and 4 comments.
I not only updated the comments for tests 3 and 4, but expanded them overall.

> Please add the mcb tag to both of these mixed content / csp tests with:
> tags = mcb
Done.

In general I suggest, we keep the mochitest here as it is, just relying on the image request (since it’s mixed passive content). I will add a new browser test, loading a script to make sure the UI does not get downgraded when using block-all-mixed-content.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 30 • 8 years ago

Carrying over r+ from Tanvi and here is what I updated based on the comments:

> So maybe rename these to frame_navigation1 and frame_navigation2?
Sure thing.

> Please add the mcb tag to both of these mixed content / csp tests with:
> tags = mcb
Done.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 31 • 8 years ago

Tanvi, while working on the browser mochitests I realized we should also log a CSP error to the web console that we are blocking an insecure request. Here it is.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 32 • 8 years ago

I suppose that test should be enough for what we want to test here.

	Tanvi Vyas[:tanvi] Reporter
	Comment 33 • 8 years ago

Comment on attachment 8729871 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>--- a/dom/security/nsMixedContentBlocker.cpp
>+++ b/dom/security/nsMixedContentBlocker.cpp
>@@ -627,16 +627,32 @@ nsMixedContentBlocker::ShouldLoad(bool a
>     *aDecision = REJECT_REQUEST;
>     return NS_OK;
>   }
>   if (!parentIsHttps) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
> 
>+  // Determine if the rootDoc is https and if the user decided to allow Mixed Content
I see what you are saying about this comment.  Don't add it here; just move it to above this line in the file:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#675

>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>+  NS_ENSURE_TRUE(docShell, NS_OK);
>+
>+  // The page might have set the CSP directive 'block-all-mixed-content' which
>+  // should block not only active mixed content loads but in fact all mixed content
>+  // loads, see https://www.w3.org/TR/mixed-content/#strict-checking
>+  // Let's simplify the approach and block all non secure loads in case the CSP
Remove "Let's simply the approach and".

>+  // directive is present. Please note that at this point we already know,
>+  // based on |schemeSecure| that the load is not secure, so we can bail out
>+  // early at this point.

Do you need a dom peer for the nsDocument changes?  Or is my + Kate's review sufficient since your changes are only related to CSP variables?

	Tanvi Vyas[:tanvi] Reporter
	Comment 34 • 8 years ago

Comment on attachment 8729872 [details] [diff] [review]
bug_1122236_block_all_mixed_content_tests.patch

Test is really clean and easy to add to in the future, so that is great!  r+ with the changes below.


Do a pushprefEnv for security.mixed_content.block_display_content to ensure that it is in fact false for the test.

>diff --git a/dom/security/test/csp/mochitest.ini b/dom/security/test/csp/mochitest.ini
>--- a/dom/security/test/csp/mochitest.ini
>+++ b/dom/security/test/csp/mochitest.ini
>@@ -231,8 +232,10 @@ skip-if = toolkit == 'android' #investig
> [test_child-src_worker_data.html]
> [test_child-src_worker-redirect.html]
> [test_child-src_iframe.html]
> [test_meta_element.html]
> [test_meta_header_dual.html]
> [test_docwrite_meta.html]
> [test_multipartchannel.html]
> [test_fontloader.html]
>+tags = mcb
>+[test_block_all_mixed_content.html]
I believe the tag goes under the test name.

>diff --git a/dom/security/test/csp/test_block_all_mixed_content.html b/dom/security/test/csp/test_block_all_mixed_content.html
>+// a postMessage handler that is used by sandboxed iframes without
>+// 'allow-same-origin' to bubble up results back to this main page.
Is this comment relevant here?  We don't have a sandboxed iframe.

>+window.addEventListener("message", receiveMessage, false);
>+function receiveMessage(event) {
>+  checkResults(event.data.result);
>+}

	Tanvi Vyas[:tanvi] Reporter
	Comment 35 • 8 years ago

Comment on attachment 8729879 [details] [diff] [review]
bug_1122236_block_all_mixed_content_console_logging.patch

>diff --git a/dom/locales/en-US/chrome/security/csp.properties b/dom/locales/en-US/chrome/security/csp.properties
>+# LOCALIZATION NOTE (blockAllMixedContent):
>+# %1$S is the URL of the blocked resource load.
>+blockAllMixedContent = Blocking insecure request '%1$S'.
I assume this is prepended with "CSP: " or something like that.

	Tanvi Vyas[:tanvi] Reporter
	Comment 36 • 8 years ago

Comment on attachment 8729885 [details] [diff] [review]
bug_1122236_browser_ui_test.patch

r+ with these changes.

Add a pushprefenv to ensure security.mixed_content.block_active_content is true.

>diff --git a/browser/base/content/test/general/browser.ini b/browser/base/content/test/general/browser.ini
>--- a/browser/base/content/test/general/browser.ini
>+++ b/browser/base/content/test/general/browser.ini
>@@ -550,8 +552,10 @@ tags = fullscreen
> [browser_aboutTabCrashed.js]
> skip-if = !e10s || !crashreporter
> [browser_aboutTabCrashed_clearEmail.js]
> skip-if = !e10s || !crashreporter
> [browser_aboutTabCrashed_showForm.js]
> skip-if = !e10s || !crashreporter
> [browser_aboutTabCrashed_withoutDump.js]
> skip-if = !e10s
>+tags = mcb
>+[browser_csp_block_all_mixedcontent.js]
tag goes underneath test name.

>diff --git a/browser/base/content/test/general/browser_csp_block_all_mixedcontent.js b/browser/base/content/test/general/browser_csp_block_all_mixedcontent.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/general/browser_csp_block_all_mixedcontent.js
>@@ -0,0 +1,44 @@
>+/*
>+ * Description of the Test:
>+ * We load an https page wich uses a CSP including block-all-mixed-content.
s/wich/which

>+ * The page tries to load a script over http. We make sure the UI is not
>+ * influenced when blocking the mixed content.
Add "i.e. the page should still appear fully encrypted with a green lock."


>+function verifyUInotDegraded() {
>+  // make sure that not mixed content is loaded and also not blocked
Change comment:
make sure that mixed script that is blocked because of the CSP directive doesn't cause the security state to change.  This is to ensure that the security UI is not degraded for sites that enable block-all-mixed-content

>+  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: false, activeBlocked: false, passiveLoaded: false});
>+

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 37 • 8 years ago

Thanks Tanvi, in fact |tags = mcb| belongs underneath the testname. I updated and incorporated all your suggestions from the last round of reviews and I think this code is so similar to upgrade-insecure-requests we are fine with your and Kate's review. Rebased all of the patches, carrying over r+.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 43 • 8 years ago

Comment on attachment 8730963 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

Hey Bobby, any chance you could sign off on the webidl changes? Otherwise I can't push to inbound :-(

	Blake Kaplan (:mrbkap) (inactive)
	Comment 44 • 8 years ago

Comment on attachment 8730963 [details] [diff] [review]
bug_1122236_block_all_mixed_content.patch

Review of attachment 8730963 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the webidl change.

	Pulsebot
	Comment 45 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8374f1f94ea5
https://hg.mozilla.org/integration/mozilla-inbound/rev/35252bf91d92
https://hg.mozilla.org/integration/mozilla-inbound/rev/32a542c19aed
https://hg.mozilla.org/integration/mozilla-inbound/rev/589452f7ab1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/75e0a68c554a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3bff206820

	Carsten Book [:Tomcat]
	Comment 46 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8374f1f94ea5
https://hg.mozilla.org/mozilla-central/rev/35252bf91d92
https://hg.mozilla.org/mozilla-central/rev/32a542c19aed
https://hg.mozilla.org/mozilla-central/rev/589452f7ab1b
https://hg.mozilla.org/mozilla-central/rev/75e0a68c554a
https://hg.mozilla.org/mozilla-central/rev/1a3bff206820

	Florian Scholz (Open Web Docs)
	Comment 47 • 8 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/48#HTTP
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/block-all-mixed-content