631040 - Parse default-src as equivalent to allow in CSP header value

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brandon Sterne (:bsterne)]

To make way for the likely (certain?) change to the W3C CSP standard, we should parse the token "default-src" as equivalent to "allow" in a site's policy.

Comment 1

[Brandon Sterne (:bsterne)]

Attachment: fix (with tests)

changes the default directive from "allow" to "default-src" and maps the value of "allow" to "default-src" in the policy parser.  Adds some tests to verify that they are parsed equivalently.

I'm also presently pushing to Try for sanity.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 512909 [details] [diff] [review]
fix (with tests)

I'd like to see Sid review this too.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 512909 [details] [diff] [review]
fix (with tests)

>+++ b/content/base/src/CSPUtils.jsm
>@@ -534,22 +546,22 @@ CSPRep.prototype = {
>   makeExplicit:
>   function cspsd_makeExplicit() {
>     var SD = CSPRep.SRC_DIRECTIVES;
>-    var allowDir = this._directives[SD.ALLOW];
>-    if (!allowDir) {
>-      CSPWarning("'allow' directive required but not present.  Reverting to \"allow 'none'\"");
>+    var defaultSrcDir = this._directives[SD.DEFAULT_SRC];
>+    if (!defaultSrcDir) {
>+      CSPWarning("'allow' or 'default-src' directive required but not present.  Reverting to \"allow 'none'\"");

This CSPWarning should reflect that we are using "default-src 'none'" as the fail-closed policy, instead of using "allow".

>diff --git a/content/base/test/unit/test_csputils.js b/content/base/test/unit/test_csputils.js
>@@ -363,12 +363,12 @@ test(
>       // check default policy "allow *"
>       cspr = CSPRep.fromString("allow *", "http://self.com:80");
>       //"ALLOW directive is missing when specified in fromString"
>-      do_check_has_key(cspr._directives, CSPRep.SRC_DIRECTIVES.ALLOW);
>+      do_check_has_key(cspr._directives, CSPRep.SRC_DIRECTIVES.DEFAULT_SRC);

Nit: Should update the comment above to say "DEFAULT_SRC directive is missing..." ... or drop the comment.

>       // ... and check that the other directives were auto-filled with the
>       // ALLOW one.

Nit: Same here, should say "DEFAULT_SRC" or "DEFAULT".

>@@ -379,6 +379,39 @@ test(
> 
> 
> test(
>+    function test_CSPRep_defaultSrc() {
>+      var cspr, cspr_default_val, cspr_allow;
>+
>+      // apply policy of "default-src *" (e.g. "allow *")
>+      cspr = CSPRep.fromString("default-src *", "http://self.com:80");
>+      // "DEFAULT_SRC directive is missing when specified in fromString"
>+      do_check_has_key(cspr._directives, CSPRep.SRC_DIRECTIVES.DEFAULT_SRC);
>+
>+      // check that the other directives were auto-filled with the
>+      // DEFAULT_SRC one.
>+      var SD = CSPRep.SRC_DIRECTIVES;
>+      cspr_default_val = cspr._directives[SD.DEFAULT_SRC];
>+      for (var d in CSPRep.SRC_DIRECTIVES) {

You should be able to use SD here instead of CSPRep.SRC_DIRECTIVES.  You might want to move the SD definition up to the top so you can use it in the first do_check_has_key() call (cspr._directives, SD.DEFAULT_SRC).

>+        // don't check allow because it won't be present in this CSPRep
>+        if (d === "ALLOW") continue;

"ALLOW" shouldn't be a key in CSPRep.SRC_DIRECTIVES, right?  You can probably drop this line.

>+        do_check_has_key(cspr._directives, SD[d]);
>+        // "Implicit directive " + d + " has non-allow value."

What does "non-allow value" mean in this context?  Shouldn't it be copied from default-src instead of allow?

Also, there are occurrences of policy creations in CSPUtils.jsm and contentSecurityPolicy.js that use "allow".  We need to convert those to default-src.  I'd like to see another patch with those fixed.

Comment 4

[Brandon Sterne (:bsterne)]

Attachment: Final fix

(In reply to comment #3)
> This CSPWarning should reflect that we are using "default-src 'none'" as the
> fail-closed policy, instead of using "allow".

Fixed.

> Nit: Should update the comment above to say "DEFAULT_SRC directive is
> missing..." ... or drop the comment.

Fixed.

> Nit: Same here, should say "DEFAULT_SRC" or "DEFAULT".

Fixed.

> You should be able to use SD here instead of CSPRep.SRC_DIRECTIVES.  You might
> want to move the SD definition up to the top so you can use it in the first
> do_check_has_key() call (cspr._directives, SD.DEFAULT_SRC).

Done and done.

> "ALLOW" shouldn't be a key in CSPRep.SRC_DIRECTIVES, right?  You can probably
> drop this line.

Indeed.  This was needed before I moved ALLOW out of SRC_DIRECTIVES. Fixed.

> What does "non-allow value" mean in this context?  Shouldn't it be copied from
> default-src instead of allow?

Yep, comment was copied from your original test, so it didn't make sense anymore.  Fixed.

> Also, there are occurrences of policy creations in CSPUtils.jsm and
> contentSecurityPolicy.js that use "allow".  We need to convert those to
> default-src.  I'd like to see another patch with those fixed.

These were all fixed as well.  A separate patch with the tests is coming momentarily.

Comment 5

[Brandon Sterne (:bsterne)]

Attachment: Tests

Comment 6

[Brandon Sterne (:bsterne)]

I pushed the final fix and test changes to Try.  Hopefully geekboy can review the new patch ASAP so we can try to land tomorrow.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 516064 [details] [diff] [review]
Final fix

If Try likes it, looks good to me.  I'd like jst to sign off on it too.

Comment 8

[Brandon Sterne (:bsterne)]

Attachment: Final tests

Tryserver caught a missed test change that geekboy's run-all-CSP-tests macro didn't:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8eb7b8a4ec12

This updated patch adds:
diff --git a/content/base/test/test_bug548193.html b/content/base/test/test_bug548193.html
--- a/content/base/test/test_bug548193.html
+++ b/content/base/test/test_bug548193.html
@@ -91,7 +91,7 @@
      "http://example.org/tests/content/base/test/file_CSP.sjs?testid=img_bad&type=img/png",
      "Incorrect blocked uri");
   // correct violated-directive
-  is(cspReport["violated-directive"], "allow http://mochi.test:8888",
+  is(cspReport["violated-directive"], "default-src http://mochi.test:8888",
      "Incorrect violated directive");
   // not practical to test request-headers as header names and values will
   // change with the trunk

Comment 9

[Brandon Sterne (:bsterne)]

pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/90140c158e78 (patch)
http://hg.mozilla.org/mozilla-central/rev/abd037fb8da4 (tests)