552523 - Allow CSP to support active and report-only mode simultaneously

Status: RESOLVED DUPLICATE of bug 836922

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

Description

[Brandon Sterne (:bsterne)]

A poster to the mozilla.dev.security newsgroup pointed out the value of supporting separate policies under CSP for enforcement as well as report-only mode.  This will allow a site to leave their existing policy in-place while testing a new policy under the report-only mode.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Proposed Patch

Proposed patch, including mochitests.  I think maybe we should get dveditz to review this too, bsterne.  What do you think?

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 446619 [details] [diff] [review]
Proposed Patch

>+  //this._policy = CSPRep.fromString("allow *");
>+  /*
...
>+  */
>+  //CSPdebug("CSP POLICY INITED TO 'allow *'");

delete rather comment-out

>@@ -128,58 +131,86 @@ ContentSecurityPolicy.prototype = {
>-      var violation = 'violated base restriction: Inline Scripts will not execute';
>-      wrapper.data = violation;
>+    var violationWrapper = Cc["@mozilla.org/supports-cstring;1"]

ObNits: 
  You could probably just call this "violation" and drop "Wrapper".
  The violation string confuses me. It sounds very generic like a status update that might get sent once, but it's really a report that a specific inline script was found and blocked, right? This is already in the "violated-directive" element (or eventually JSON property), and the others all look short. Could it just be "inline-script base restriction" or "base restriction: no inline scripts"?

>+    violationWrapper.data = 'violated base restriction: Code will not be created from strings';

Similar comments here. Also "Code will not be created from strings" is maybe more accurate, but I think we should use the word "eval" since people will understand that better and it's also the term used in the policy language.

>     var newpolicy = CSPRep.fromString(aPolicy,
>                                       selfURI.scheme + "://" + selfURI.hostPort);

Not part of this patch, but I'm confused why you're turning a perfectly good URI into a string (and possibly an invalid URI, not all schemes use "://"). Then inside fromString() you do a lot of turning it back into a URI. I suppose since we're relying solely on HTTP headers at this point it's going to work but it still seems kind of wrong.

Hm, what do we do about nested URI schemes, like jar:http://mysite.com/someapp.jar!/index.html ?

r+ dveditz

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #2)
> >     var newpolicy = CSPRep.fromString(aPolicy,
> >                                       selfURI.scheme + "://" + selfURI.hostPort);
> 
> Not part of this patch, but I'm confused why you're turning a perfectly good
> URI into a string (and possibly an invalid URI, not all schemes use "://").
> Then inside fromString() you do a lot of turning it back into a URI. I suppose
> since we're relying solely on HTTP headers at this point it's going to work but
> it still seems kind of wrong.
> 
> Hm, what do we do about nested URI schemes, like
> jar:http://mysite.com/someapp.jar!/index.html ?

I created bug 570505 to address this.  I'll go through the rest of CSP code and see where URI's are stringified and then uri-ified again.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Proposed Patch

(In reply to comment #2)
> (From update of attachment 446619 [details] [diff] [review])
> >+  //this._policy = CSPRep.fromString("allow *");
> >+  /*
> ...
> >+  */
> >+  //CSPdebug("CSP POLICY INITED TO 'allow *'");
> 
> delete rather comment-out

Yeah, good call.  Done.

> 
> >@@ -128,58 +131,86 @@ ContentSecurityPolicy.prototype = {
> >-      var violation = 'violated base restriction: Inline Scripts will not execute';
> >-      wrapper.data = violation;
> >+    var violationWrapper = Cc["@mozilla.org/supports-cstring;1"]
> 
> ObNits: 
>   You could probably just call this "violation" and drop "Wrapper".
>   The violation string confuses me. It sounds very generic like a status update
> that might get sent once, but it's really a report that a specific inline
> script was found and blocked, right? This is already in the
> "violated-directive" element (or eventually JSON property), and the others all
> look short. Could it just be "inline-script base restriction" or "base
> restriction: no inline scripts"?
> 
> >+    violationWrapper.data = 'violated base restriction: Code will not be created from strings';

> Similar comments here. Also "Code will not be created from strings" is maybe
> more accurate, but I think we should use the word "eval" since people will
> understand that better and it's also the term used in the policy language.

Done.

> Hm, what do we do about nested URI schemes, like
> jar:http://mysite.com/someapp.jar!/index.html ?

I think this'll be addressed by the newly filed bug 570505.

> r+ dveditz

Carried over the r+ since the changes were minor and cosmetic.

Comment 5

[Brandon Sterne (:bsterne)]

http://hg.mozilla.org/mozilla-central/rev/f0d3b958d38d

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like this broke a lot of CSP mochitests; it will be backed out in a few minutes unless a fix appears.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/8c780e5c086d
http://hg.mozilla.org/mozilla-central/rev/6a2aaf221ccf

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Proposed Patch

The CSP tests might have been timing out because of the way results were being posted: they were throwing an assertion because the DOM was being updated from inside a "http-on-modify-request" event, and it was not safe to run scripts there apparently.  Fixed with an instant setTimeout, and seemed fine on try.

dveditz: could you take another peek at this?  I want to make sure I didn't do anything stupid, and should be a quick review since you've seen most of the code before.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

dveditz: any chance you could give this a quick review?

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 457137 [details] [diff] [review]
Proposed Patch

Sorry for the delay, I thought I had done this one
r=dveditz

Comment 11

[Benjamin Smedberg]

Comment on attachment 457137 [details] [diff] [review]
Proposed Patch

This patch changes interfaces, which is generally forbidden after API freeze. If you would like to take this patch, you'll either need to refactor it to avoid the interface change (perhaps by adding nsIContentSecurityPolicy_MOZILLA_2_0, although that looks very difficult in this case), or by mailing [e-mail] and asking for an exemption. Please indicate the seriousness of the bug, and discuss possibility that extensions are using the current interface (from either JS or C++).

approval- for now.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> discuss possibility that extensions are using the current interface

Zero: it's a new feature currently being used only by a few test sites.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 457137 [details] [diff] [review]
Proposed Patch

Sending mail to release-drivers.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

updated for bit-rot and repaired mochitests to separate report-only and enforced-policy reports to make sure the *right* reports come in (not just enough).

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

attaching the *right* version of the patch.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

dveditz: any chance for a review?  This has been sitting around for a while.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Updated for bit rot, and made the unit tests slightly more robust.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 457137 [details] [diff] [review]
Proposed Patch

(bookkeeping)

Comment 19

[Ian Melven :imelven]

are comments #11 and #12 still relevant ? ie is there going to be a problem with landing an updated version of this patch due to frozen interfaces or is it still ok ?

Comment 20

[Brandon Sterne (:bsterne)]

You're free to change interfaces. Those comments were made in the context of the pre-Firefox-4 API freeze.

Comment 21

[Ian Melven :imelven]

Attachment: sid's existing patch updated

sid's existing patch updated for current code - passes current csp tests. if this looks ok i will r? to dveditz

Comment 22

[Ian Melven :imelven]

going to need to update this patch further, the test should be updated to use SpecialPowers instead of enablePrivilege once we figure out how the observer stuff will work with SpecialPowers.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 540929 [details] [diff] [review]
sid's existing patch updated

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

Looks good to me with the minor tweaks below fixed and verification that this doesn't break stuff on try.

::: content/base/src/contentSecurityPolicy.js
@@ +306,5 @@
>      // (2) Intersect the currently installed CSPRep object with the new one
> +    if (!this[policyToRefine])
> +      this[policyToRefine] = newpolicy;
> +    else
> +      this[policyToRefine] = this._policy.intersectWith(newpolicy);

I didn't catch this before:  this._policy should be this[policyToRefine].

::: content/base/test/test_CSP.html
@@ +86,5 @@
> +        netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +        window.testResult(testid,
> +                          /_bad/.test(testid),
> +                          uri.asciiSpec + " blocked by \"" + data + "\"");
> +      }, 0);

This should probably be updated to use SimpleTest.executeSoon()

::: content/base/test/test_CSP_frameancestors.html
@@ +57,5 @@
> +      // defer this since we may be crossing threads...
> +      window.setTimeout(function () {
> +        netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +        window.frameBlocked(uri.asciiSpec, data);
> +      }, 0);

Same here, this should probably be updated to use SimpleTest.executeSoon()

Comment 24

[Ian Melven :imelven]

Attachment: patch that passes all csp tests including the ones this bug adds

Sid, i had to change a few minor things so asking for feedback if you have time and get to it before dveditz gets to the review.

This depends on the patch in bug 674255 since i updated the CSP tests in this bug to use SpecialPowers, it's also updated for bitrot since the last patch. 

Passes all existing CSP tests and the tests added in this bug.

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 558003 [details] [diff] [review]
patch that passes all csp tests including the ones this bug adds

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

f- until the IDL and violation logging stuff is fixed.

::: content/base/src/contentSecurityPolicy.js
@@ +60,5 @@
>  
>  function ContentSecurityPolicy() {
>    CSPdebug("CSP CREATED");
>    this._isInitialized = false;
>    this._reportOnlyMode = false;

Need to remove the _reportOnlyMode flag.

@@ +194,5 @@
>    logViolationDetails:
>    function(aViolationType, aSourceFile, aScriptSample, aLineNum) {
>      // allowsInlineScript and allowsEval both return true when report-only mode
>      // is enabled, resulting in a call to this function. Therefore we need to
>      // check that the policy was in fact violated before logging any violations

After this patch, report-only is no longer a mode, but a separate policy.  We want to log *all* violations regardless of which policy, and that doesn't appear to be happening right now.

Also, logViolationDetails needs to happen for the reportOnly policy *and* the enforced policy.  In the eval-running and inline-script-triggering code, logViolationDetails needs to be called *regardless* of whether allowsInlineScript or allowsEval return false.

This means we need to update callers to always call logViolationDetails:

nsJSProtocolHandler.cpp:206
nsEventListenerManager.cpp:488
nsScriptSecurityManager.cpp:563

Which means we should probably rename it to logAnyViolationDetails().

The general pattern should be:
 PRBool allowsInline;
 rv = csp->GetAllowsInlineScript(&allowsInline);
 csp->LogAnyViolationDetails(...);
 if(!allowsInline) {
   blockStuff();
 }

Sorry I didn't catch this before.  :(

@@ +211,5 @@
>        break;
>      }
>    },
>  
>    set reportOnlyMode(val) {

Need to remove the reportOnlyMode getters/setters and the line in the IDL (which means also bump the GUID in the IDL).  It no longer makes sense to have a "mode" when really it's two policies side-by-side.

@@ +377,5 @@
> +          // This prevents sending cookies with the request,
> +          // in case the policy URI is injected, it can't be
> +          // abused for CSRF.
> +          req.channel.loadFlags |= Ci.nsIChannel.LOAD_ANONYMOUS;
> +

Bug 679772 removed this block, we should leave it out.

@@ +434,3 @@
>          let violatedPolicy = (directive._isImplicit
> +                              ? 'allow' : 'frame-ancestors ')
> +    	    		                      + directive.toString();

nit: indentation... line up last line with open paren line.

@@ +591,1 @@
>                                   aSourceFile, aScriptSample, aLineNum);

nit: line up hanging arguments.

::: content/base/test/test_CSP.html
@@ -52,2 @@
>    observe: function(subject, topic, data) {
> -    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

\o/ yay for not needing enablePrivilege!

Comment 26

[Ian Melven :imelven]

Attachment: latest patch

Updated with Sid's latest feedback and a few tweaks. Still needs a try server run before asking for review.

Comment 27

[Ian Melven :imelven]

Attachment: latest patch

oops, missed one feedback item, this patch drops the anonymous load block as Sid pointed out previously in addition to the prev. patch.

Comment 29

[Ian Melven :imelven]

Comment on attachment 559355 [details] [diff] [review]
latest patch

removing feedback? 

i'm not really happy with this patch and maybe want to refactor it, will discuss with sid later.

Comment 30

[Ian Melven :imelven]

Sid and I discussed this today, the plan currently is to refactor the patch to avoid as many XPCOM calls as possible - tentatively we will change the CSP checks to return an enum (defined in the IDL) that will indicate block, warn, or allow for checking inline script and evals - we can then report violations as needed.

Comment 31

[Sid Stamm [:geekboy or :sstamm]]

imelven - any update here?

Comment 32

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #31)
> imelven - any update here?

No update - i unassigned myself, i will be busy with sandboxing for the foreseeable future...

Comment 33

[Sid Stamm [:geekboy or :sstamm]]

Instead we should probably support n policies (instead of intersecting them), and each policy should be report-only or not.