576200 - CSP breaks spec, defaults to allow *

Status: RESOLVED FIXED

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

Description

[Devdatta Akhawe [:devd]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.6) Gecko/20100628 Ubuntu/10.04 (lucid) Firefox/3.6.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:2.0b2pre) Gecko/20100630 Minefield/4.0b2pre

CSP is supposed to default deny i.e. allow 'none' as the spec mentions. Currently it defaults to allow * 

Reproducible: Always

Steps to Reproduce:
1. Open any site with CSP and read the console
2.
3.
Actual Results:  
INITED to 'allow *'

Comment 1

[Devdatta Akhawe [:devd]]

Attachment: Patch on 46470:15675470aa2d

quick and dirty patch - I am not familiar with the code, please forgive if its really slows down something. My testing indicates it works fine, but I might be wrong.

Comment 2

[Devdatta Akhawe [:devd]]

the current code also doesn't fail-with deny for sourcelist errors, I am not quite sure what to do in that case -- seems like failing to an empty source list would be a good idea , since the rest of CSP seems to take a very formal/correct approach. I have not changed anything, but only added a comment.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 455386 [details] [diff] [review]
Patch on 46470:15675470aa2d

>diff -r 5295a7cfd05c content/base/src/CSPUtils.jsm
>--- a/content/base/src/CSPUtils.jsm	Wed Mar 10 19:16:44 2010 -0600
>+++ b/content/base/src/CSPUtils.jsm	Wed Jun 30 22:39:50 2010 -0700
>@@ -147,8 +147,10 @@
>           aCSPR._allowInlineScripts = true;
>         else if (opt === "eval-script")
>           aCSPR._allowEval = true;
>-        else
>+        else {
>           CSPWarning("don't understand option '" + opt + "'.  Ignoring it.");
>+	  CSPRep.fromString("allow 'none'");
>+	}

This is not correct, unrecognized "options" tokens MUST be ignored (https://wiki.mozilla.org/Security/CSP/Specification#Directives).  Additionally CSPRep.fromString is a factory method.  Proper use would be:

 aCSPR = CSPRep.fromString(policy);

>@@ -184,10 +186,12 @@
>             } else {
>               CSPWarning("can't use report URI from non-matching eTLD+1: "
>                          + gETLDService.getBaseDomain(uri));
>+	      CSPRep.fromString("allow 'none'");
>             }
>           }
>         } catch(e) {
>           CSPWarning("couldn't parse report URI: " + dirvalue);
>+	  CSPRep.fromString("allow 'none'");
>         }
>       }

This is also not correct.  Unrecognized or invalid report URIs must be ignored.

>@@ -256,6 +260,7 @@
> 
>     // UNIDENTIFIED DIRECTIVE /////////////////////////////////////////////
>     CSPWarning("Couldn't process unknown directive '" + dirname + "'");
>+    return CSPRep.fromString("allow 'none'");

Again, this is not correct.  Unrecognized directives must be ignored (for future compatibility).

>@@ -476,6 +481,8 @@
>     var src = CSPSource.create(tokens[i], self, enforceSelfChecks);
>     if (!src) {
>       CSPWarning("Failed to parse unrecoginzied source " + tokens[i]);
>+      //This should ideally fail too - not clear why this isn't , but I will let it be - devdatta
>+      // return new CSPSourceList();

See my previous note: it's for future compatibility.  Unknown tokens in general are ignored.

>diff -r 5295a7cfd05c content/base/src/contentSecurityPolicy.js
>--- a/content/base/src/contentSecurityPolicy.js	Wed Mar 10 19:16:44 2010 -0600
>+++ b/content/base/src/contentSecurityPolicy.js	Wed Jun 30 22:39:50 2010 -0700
>@@ -61,15 +61,15 @@
>   CSPdebug("CSP CREATED");
>   this._isInitialized = false;
>   this._reportOnlyMode = false;
>-  this._policy = CSPRep.fromString("allow *");
>-
>+  this._policy = CSPRep.fromString("allow 'none'");
>+  this._policy._actualSeen = false;

This line of code does not do what you think it does.  Perhaps we should add a comment to explain it.

There's no "setter" for the _policy variable in contentSecurityPolicy.js (http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#181).  As a result, we have to create a new CSP representation and "refine" it.  An empty CSP policy string is never used, it is always refined at least once.  In the case of a policy "a b; c d;" where there are no valid directives, it should be instantiated into a CSPRep as "allow 'none'", and then when intersected with the initial instance, will result in "allow 'none'".  

This default as shown here means "no policy specified" so it should be "allow *".  It is not "policy specified but empty."

>   // default options "wide open" since this policy will be intersected soon
>-  this._policy._allowInlineScripts = true;
>-  this._policy._allowEval = true;
>+  this._policy._allowInlineScripts = false;
>+  this._policy._allowEval = false;

This was that way for the same reason.  There's always at least one intersection when a CSP is specified.  If we set these to false, they'll never be set back to true and no CSP-employing site will get to have inline scripts.

>@@ -221,10 +221,17 @@
>   function csp_refinePolicy(aPolicy, selfURI) {
>     CSPdebug("REFINE POLICY: " + aPolicy);
>     CSPdebug("         SELF: " + selfURI.asciiSpec);
>-
>+    
>     // stay uninitialized until policy merging is done
>     this._isInitialized = false;
>-
>+    if(this._policy._actualSeen === false){
>+      //nothing has been actually seen yet, so set the default to all allow
>+      // so that intersection doesn't fail
>+      this._policy = CSPRep.fromString("allow *");
>+      this._policy._allowInlineScripts = true;
>+      this._policy._allowEval = true;
>+      this._policy._actualSeen = true;
>+    }

Same thing.  If a policy hasn't been seen, we should not enforce one (and instead use "allow *").  When one is seen, it will be intersected with this.

I think maybe the way the code is written it really isn't as clear as it could be what's going on.  Marking this patch r- because it is the wrong way to approach this (this patch will clearly cause all sites to have "allow 'none'", which you may have seen if you tried out the unit tests in a/content/base/test/test_CSP*.

Tempted to mark this as invalid, but before I do I'm going to run a basic test: a site with CSP "foo bar.com", and see what policy is enforced.  Should be "allow 'none'".  A site with policy "allow *" should not have "allow 'none'" enforced.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #3)
> >diff -r 5295a7cfd05c content/base/src/contentSecurityPolicy.js
> >--- a/content/base/src/contentSecurityPolicy.js	Wed Mar 10 19:16:44 2010 -0600
> >+++ b/content/base/src/contentSecurityPolicy.js	Wed Jun 30 22:39:50 2010 -0700
> >@@ -61,15 +61,15 @@
> >   CSPdebug("CSP CREATED");
> >   this._isInitialized = false;
> >   this._reportOnlyMode = false;
> >-  this._policy = CSPRep.fromString("allow *");
> >-
> >+  this._policy = CSPRep.fromString("allow 'none'");
> >+  this._policy._actualSeen = false;
> 
> This line of code does not do what you think it does.  Perhaps we should add a
> comment to explain it.

This weird set-up sequence will be taken care of in bug 552523, and it'll be more clear what's going on ... in fact, "allow *" will completely go away, and the set-up will be more appropriate.

See https://bugzilla.mozilla.org/show_bug.cgi?id=552523#c4

Making bug 552523 block this one (since that fixes this part of the error).

> There's no "setter" for the _policy variable in contentSecurityPolicy.js
> (http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#181).
>  As a result, we have to create a new CSP representation and "refine" it.  An
> empty CSP policy string is never used, it is always refined at least once.  In
> the case of a policy "a b; c d;" where there are no valid directives, it should
> be instantiated into a CSPRep as "allow 'none'", and then when intersected with
> the initial instance, will result in "allow 'none'".  
> 
> This default as shown here means "no policy specified" so it should be "allow
> *".  It is not "policy specified but empty."
> 
> >   // default options "wide open" since this policy will be intersected soon
> >-  this._policy._allowInlineScripts = true;
> >-  this._policy._allowEval = true;
> >+  this._policy._allowInlineScripts = false;
> >+  this._policy._allowEval = false;

This will be taken care of by bug 552523 as well (that actually fixes all the weirdness in setup of a CSP).

> Tempted to mark this as invalid, but before I do I'm going to run a basic test:
> a site with CSP "foo bar.com", and see what policy is enforced.  Should be
> "allow 'none'".  A site with policy "allow *" should not have "allow 'none'"
> enforced.

This does reproduce, but I think the fix is easy.  Will upload a fix patch shortly.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

After a policy is created, it is "made explicit".  During this step, if there's no 'allow' directive, the function makeExplicit() now posts a CSP warning and returns false.  I also had to add a check to see if makeExplicit() succeeded.  If it didn't, the policy fails closed to "allow 'none'".  The only case in which that function fails is currently when there's no allow directive, but potentially we could put some other stuff in there in the future.

bsterne: can you take a peek and sanity check this?

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 455386 [details] [diff] [review]
Patch on 46470:15675470aa2d

>--- a/content/base/src/contentSecurityPolicy.js	Wed Mar 10 19:16:44 2010 -0600
>+++ b/content/base/src/contentSecurityPolicy.js	Wed Jun 30 22:39:50 2010 -0700
>@@ -61,15 +61,15 @@
>   CSPdebug("CSP CREATED");
>   this._isInitialized = false;
>   this._reportOnlyMode = false;
>-  this._policy = CSPRep.fromString("allow *");
>-
>+  this._policy = CSPRep.fromString("allow 'none'");
>+  this._policy._actualSeen = false;
>   // default options "wide open" since this policy will be intersected soon
>-  this._policy._allowInlineScripts = true;
>-  this._policy._allowEval = true;
>+  this._policy._allowInlineScripts = false;
>+  this._policy._allowEval = false;
> 
>   this._requestHeaders = []; 
>   this._request = "";
>-  CSPdebug("CSP POLICY INITED TO 'allow *'");
>+  CSPdebug("CSP POLICY INITED TO 'allow 'none''");
> 
>   this._observerService = Cc['@mozilla.org/observer-service;1']
>                             .getService(Ci.nsIObserverService);

A better patch would be to simply remove the misleading "CSP POLICY INITED" console spew altogether. Also seems a little odd to say CSP CREATED before doing any creating -- move that line to the end of the constructor?

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

That's precisely what's done in bug 552523 (amongst other things that make it clearer what's going on and why).  The "CSP CREATED" thing is spurious, yeah.  We could move it, but after bug 552523, it will matter less.

Comment 8

[Devdatta Akhawe [:devd]]

@sid : From my testing, CSP policy inited was only outputted when a CSP header
was seen. So if a policy isn't seen the function where I changed it to init to
"allow none" won't be called.

ofcourse your patch is better - I didn't know about makeExplicit at all.

Comment 9

[Brandon Sterne (:bsterne)]

Comment on attachment 455566 [details] [diff] [review]
fix

r=bsterne

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 455566 [details] [diff] [review]
fix

r=dveditz

Comment 11

[Johnny Stenback (:jst)]

This is ready to land, right?

Comment 12

[Johnny Stenback (:jst)]

Pushed to mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/903b6e5087a0