634778 - CSP should warn and skip duplicates of directives while parsing a policy

Status: RESOLVED FIXED

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

Description

[Brandon Sterne (:bsterne)]

When a CSP contains more than one instance of a directive, e.g.
default-src 'self'; img-src *; default-src 'self' example.com

we should, at a minimum, log a warning on the Error Console.  This is currently unspecified behavior.  Our implementation takes the last directive value we see as the value of record.

Comment 1

[Lucas Adamski [:ladamski]]

Making progress here on a patch.  Notes on my general approach:

Warn on duplicates by checking (within CSPRep.fromString()) if aCSPR._directives[dirname] is already defined; CSPWarning() if so.   Exceptions to this:

report-uri: don't warn on duplicates since this is supported
options: warn if(aCSPR._allowInlineScripts || aCSPR._allowEval)
allow: warn if aCSPR._directives[SD.DEFAULT_SRC] is defined

More info on allow vs default-src.  The approach behaves as follows:
allow + allow: warn on duplicate allow
allow + default-src: warn on duplicate default-src
default-src + allow: warn on duplicate allow
default-src + default-src: warn on duplicate default-src
Could be more OCD here but that would complicate the patch, requiring tracking more state.  Not sure if its worth it given the deprecation.

This seems to cover the existing directives from my manual testing.

If that approach seems reasonable I can submit a patch for feedback.

Speaking of testing, not sure how much coverage we need.  In theory we could just test almost endless permutations of directives.  I'm thinking instead of just testing one instance of each of the main directives, then going a little deeper on the above exceptions.  Thoughts?

Comment 2

[Lucas Adamski [:ladamski]]

Attachment: Proposed patch for feedback

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Lucas Adamski from comment #1)
> Making progress here on a patch.  Notes on my general approach:
> [...snip...]
> More info on allow vs default-src.  The approach behaves as follows:
> allow + allow: warn on duplicate allow
> allow + default-src: warn on duplicate default-src
> default-src + allow: warn on duplicate allow

Since we're deprecating "allow", should we warn "both allow and default-src were specified; allow is deprecated, please use only default-src"?

> Speaking of testing, not sure how much coverage we need.  In theory we could
> just test almost endless permutations of directives.  I'm thinking instead
> of just testing one instance of each of the main directives, then going a
> little deeper on the above exceptions.  Thoughts?

That works.  Don't need to be too exhaustive.

Moar in my feedback of the patch.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 601538 [details] [diff] [review]
Proposed patch for feedback

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

Thanks for the patch!  Good start.  Lets widen the scope of the bug a bit to skip duplicate directives (instead of overwrite them).

In the warnings, instead of saying "x will be honored", I recommend saying "x will be ignored".  The effect is clearer from the policy writer's point of view.

Also, we should spit out more information when CSP debug is enabled.  Please add CSPdebug() calls that report the directive name and value for the directives that are skipped.  For example:

 CSPdebug("Skipping parse of duplicate directive: \"" + dir + "\"");

::: content/base/src/CSPUtils.jsm
@@ +246,5 @@
>  
> +    if(typeof(aCSPR._directives[dirname]) !== "undefined" && dirname !== UD.REPORT_URI) {  // Check for (most) duplicate directives
> +      CSPError("Duplicate " + dirname + " directives detected.  Only the last instance will be honored.");
> +    }
> +    

The W3C working spec says: "If the set of directives already contains a directive with name directive name, ignore this instance of the directive and continue to the next token."

This suggests we should only include the *first* directive.  That's not how the current parser behaves, but this should be an easy fix (add a "continue" inside the if block).  The parser should not skip future report-URI directives, but the rest should be skipped.  Maybe we need to move the REPORT URI section up to the top of this parser routine and immediately after that add a gate for "warn and skip duplicates".

@@ +272,5 @@
>      // stabilizes, at which time we can stop parsing "allow"
>      if (dirname === CSPRep.ALLOW_DIRECTIVE) {
> +      if (typeof(aCSPR._directives[SD.DEFAULT_SRC]) !== "undefined") {  // Check for duplicate default-src and allow directives         
> +        CSPError("Duplicate allow directives detected.  Only the last instance will be honored.");
> +      }

I think it makes sense to put something about deprecation in here too.  Warn once when we get inside the if block for the allow directive ("Allow directive is deprecated, use the equivalent default-source directive instead").  Warn again when we see multiple default-src directives, but it should be clear to the reader that the duplicate could have been either "allow" or "default-src".  Maybe:

"Duplicate allow/default-src directive detected.  All but the first instance will be skipped."

::: toolkit/crashreporter/google-breakpad/src/client/mac/Breakpad.xcodeproj/project.pbxproj
@@ +1337,4 @@
>  			isa = PBXProject;
>  			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "Breakpad" */;
>  			compatibilityVersion = "Xcode 3.1";
> +			developmentRegion = English;

Please don't include this file in your patch.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Widening the scope of the bug title to "warn about and skip any duplicates".

And ignore the bit of my feedback about not skipping duplicate report-uri directives.  Warn and skip those too.

Comment 6

[Marshall Moutenot]

(In reply to Sid Stamm [:geekboy] from comment #5)
> Widening the scope of the bug title to "warn about and skip any duplicates".
> 
> And ignore the bit of my feedback about not skipping duplicate report-uri
> directives.  Warn and skip those too.

I am now working on this bug.

Sid: report-uri is where the report is sent, correct? Was it not initially in the spec that they should be skipped? I'm just trying to understand the correction made in your last comment.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

I mistakenly suggested we treat report-uri different than the rest of the directives.  Just keep 'em all the same (first one rules, the rest trigger warnings).

Comment 8

[Marshall Moutenot]

Sorry Lucas!

@@ -212,6 +212,14 @@
     var dirname = dir.split(/\s+/)[0];
     var dirvalue = dir.substring(dirname.length).trim();
 
+    // Check for and skip duplicate directives
+    if(typeof(aCSPR._directives[dirname]) !== "undefined") {
+        CSPError("Duplicate " + dirname + " directives detected. "
+                              + dirname + ":" + dirvalue + " will be ignored.");
+        CSPdebug("Skipping parse of duplicate directive: \"" + dir + "\"");
+        continue;
+    }
+

Comment 9

[Lucas Adamski [:ladamski]]

Attachment: Patch rev 2

Sadly I'd actually updated this some time ago (on a long flight) and forgot about it. :/

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 642155 [details] [diff] [review]
Patch rev 2

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

I'd like to see some tests for duplicates, probably at least:

* one test of a policy with two of the same directive
* one test of a policy with three directives, one that's a duplicate
* one test similar to the second test in a different order
* one test with triplicates of a directive intermixed with other directives
* one test with duplicate allow
* one test with duplicate default-src
* one test with one of each allow and default-src.

The tests just need to verify that the policy is parsed as expected (not necessary to check console output).  You can just add a section to test_csputils.js.  A good starting point is the "test_CSP_ReportURI_parsing" test.

Please make your lines < 80 cols when possible, and watch for trailing whitespace.  Also, if blocks should be formatted consistently with the rest of the file (space after the word 'if')

::: content/base/src/CSPUtils.jsm
@@ +223,5 @@
> +    if(typeof(aCSPR._directives[dirname]) !== "undefined") {  // Check for (most) duplicate directives
> +      if(dirname === CSPRep.DEFAULT_SRC)
> +        CSPError("Duplicate allow/default-src directive detected.  All but the first instance will be ignored.");
> +      else
> +        CSPError("Duplicate " + dirname + " directive detected.  All but the first instance will be ignored.");

We'll probably want to localize the strings in CSPError and CSPWarning.  Please pull them out and put them in /dom/locales/en-US/chrome/security/csp.properties (see other calls to CSPError and CSPWarning in CSPUtils.jsm).

@@ +232,3 @@
>      // OPTIONS DIRECTIVE ////////////////////////////////////////////////
>      if (dirname === CSPRep.OPTIONS_DIRECTIVE) {
> +      if (aCSPR._allowInlineScripts || aCSPR._allowEval) {  // Check for duplicate options directives         

Please remove trailing whitespace here and elsewhere in the file (you also may want to delete/change the comment to its own line)

@@ +256,5 @@
>      if (dirname === CSPRep.ALLOW_DIRECTIVE) {
> +      CSPWarning("Allow directive is deprecated, use the equivalent default-source directive instead");
> +      if (typeof(aCSPR._directives[SD.DEFAULT_SRC]) !== "undefined") {  // Check for duplicate default-src and allow directives 
> +        CSPError("Duplicate allow/default-src directive detected.  All but the first instance will be ignored.");
> +        CSPdebug("Skipping parse of duplicate directive: \"" + dir + "\"");

This is a little awkward... maybe "Skipping duplicate directive:" or "Not parsing repeated directive:"

Comment 11

[Lucas Adamski [:ladamski]]

Quick question: CSPdebug strings don't seem to be localized.  Is that the norm?

Comment 12

[Ian Melven :imelven]

(In reply to Lucas Adamski from comment #11)
> Quick question: CSPdebug strings don't seem to be localized.  Is that the
> norm?

adding mgoodwin so he can comment.

Comment 13

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

If the debug messages end up to web or error console, they should be localizable, I believe.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

A user must change the value of a pref in about:config to get these messages to show up on the error/web console.  Should we still localize them?

Comment 15

[Axel Hecht [:Pike]]

I'd continue to do it like the file does it, CSPError gets localized, CSPdebug does not. The prefixing inside those functions with hardocded strings isn't ideal, but that's a different bug, filed bug 778520.

Comment 16

[Lucas Adamski [:ladamski]]

Attachment: Patch rev 3

Addressed comments and added tests.  Passes all CSP tests.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 652263 [details] [diff] [review]
Patch rev 3

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

Really close... After thinking about this a bit, I would like you to address my comments and post another iteration.

Also, please set up your .hgrc to add you as author to the patches you create and add a commit message to the patch that describes the work.  ( https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in ).

::: content/base/src/CSPUtils.jsm
@@ +219,5 @@
>  
>      var dirname = dir.split(/\s+/)[0];
>      var dirvalue = dir.substring(dirname.length).trim();
>  
> +    if (typeof(aCSPR._directives[dirname]) !== "undefined") {  

Nit: Extra whitespace after {, please remove it.

Maybe instead of using !== "undefined" to check for a parsed directive, use:
  aCSPR._directives.hasOwnProperty(dirname)
This seems a bit easier to read, and if you're ambitious, you can change it elsewhere in the file too.

@@ +221,5 @@
>      var dirvalue = dir.substring(dirname.length).trim();
>  
> +    if (typeof(aCSPR._directives[dirname]) !== "undefined") {  
> +      // Check for (most) duplicate directives
> +      if (dirname === CSPRep.DEFAULT_SRC)

You should use SD.DEFAULT_SRC here instead of CSPRep.DEFAULT_SRC which is undefined.  You might also want to just skip special-casing DEFAULT_SRC since it's a SRC_DIRECTIVE.  Sure, it's nice to have the same error message for DEFAULT_SRC and ALLOW, but I'm not convinced it matters enough that we need the extra if statement here.

@@ +255,5 @@
>      // ALLOW DIRECTIVE //////////////////////////////////////////////////
>      // parse "allow" as equivalent to "default-src", at least until the spec
>      // stabilizes, at which time we can stop parsing "allow"
>      if (dirname === CSPRep.ALLOW_DIRECTIVE) {
> +      CSPWarning("allowDirectiveDeprecated");

I think you want to call CSPLocalizer.getFormatStr() here with the string label.

::: content/base/test/unit/test_csputils.js
@@ +567,5 @@
> +      var secondDomain = "http://second.com";
> +      var thirdDomain = "http://third.com";
> +
> +      // check for duplicate "default-src" directives
> +      cspr = CSPRep.fromString("default-src " + self + "; default-src " + 

Moar trailing whitespace here.

@@ +590,5 @@
> +                              + firstDomain + "/report.py", URI(self));
> +      parsedURIs = cspr.getReportURIs().split(/\s+/);
> +      do_check_in_array(parsedURIs, self + "/report.py");
> +      do_check_eq(parsedURIs.length, 1);
> +     

And moar whitespace.  Check this whole file for extra whitespace, please.

@@ +595,5 @@
> +      // check for three directives with duplicates
> +      cspr = CSPRep.fromString("img-src " + firstDomain + "; default-src " + self
> +                               + "; img-src " + secondDomain, URI(self));
> +      do_check_true(cspr.permits(firstDomain, SD.IMG_SRC));
> +      do_check_false(cspr.permits(secondDomain, SD.IMG_SRC));

Consider adding another check here for the effect of default-src ... want to make sure it still works.

@@ +615,5 @@
> +                              + "; img-src " + secondDomain + "; img-src "
> +                              + thirdDomain, URI(self));
> +     do_check_true(cspr.permits(firstDomain, SD.IMG_SRC));
> +     do_check_false(cspr.permits(secondDomain, SD.IMG_SRC));     
> +     do_check_false(cspr.permits(thirdDomain, SD.IMG_SRC));     

Please add one more test for four directives, two of which are distinct duplicates (two img-src and two default-src or something).

::: dom/locales/en-US/chrome/security/csp.properties
@@ +45,5 @@
>  # LOCALIZATION NOTE (reportPostRedirect):
>  # %1$S is the specified report URI before redirect
>  reportPostRedirect = Post of violation report to %1$S failed, as a redirect occurred
> +# LOCALIZATION NOTE (allowDirectiveDeprecated):
> +allowDirectiveDeprecated = Allow directive is deprecated, use the equivalent default-source directive instead

please don't capitalize Allow in this string (the directive name is not usually capitalized).

Comment 18

[Lucas Adamski [:ladamski]]

Attachment: Rev 4 on patch

I think I addressed all of your comments.

Comment 19

[Lucas Adamski [:ladamski]]

I ended up removing the extra branch for the allow/default-src case.  I already had the author and patch info set but apparently its not included when you do "hg qdiff".

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 662681 [details] [diff] [review]
Rev 4 on patch

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

Looks good!

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

On inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbb115f91b7

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ccbb115f91b7