Allow CSP to support active and report-only mode simultaneously

RESOLVED DUPLICATE of bug 836922

Status

()

defect
RESOLVED DUPLICATE of bug 836922
10 years ago
7 years ago

People

(Reporter: bsterne, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
mozilla2.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: not-ready)

Attachments

(1 attachment, 9 obsolete attachments)

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.
Posted patch Proposed Patch (obsolete) — Splinter Review
Proposed patch, including mochitests.  I think maybe we should get dveditz to review this too, bsterne.  What do you think?
Attachment #446619 - Flags: review?
Attachment #446619 - Flags: review? → review?(bsterne)
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
Attachment #446619 - Flags: review+
(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.
Status: NEW → ASSIGNED
Posted patch Proposed Patch (obsolete) — Splinter Review
(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.
Attachment #446619 - Attachment is obsolete: true
Attachment #449654 - Flags: review+
Attachment #446619 - Flags: review?(bsterne)
http://hg.mozilla.org/mozilla-central/rev/f0d3b958d38d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Looks like this broke a lot of CSP mochitests; it will be backed out in a few minutes unless a fix appears.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Proposed Patch (obsolete) — Splinter Review
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.
Attachment #449654 - Attachment is obsolete: true
Attachment #457137 - Flags: review?(dveditz)
Target Milestone: mozilla1.9.3 → mozilla2.0
dveditz: any chance you could give this a quick review?
Comment on attachment 457137 [details] [diff] [review]
Proposed Patch

Sorry for the delay, I thought I had done this one
r=dveditz
Attachment #457137 - Flags: review?(dveditz)
Attachment #457137 - Flags: review+
Attachment #457137 - Flags: approval2.0?
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 release-drivers@mozilla.org 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.
Attachment #457137 - Flags: approval2.0? → approval2.0-
(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 on attachment 457137 [details] [diff] [review]
Proposed Patch

Sending mail to release-drivers.
Attachment #457137 - Flags: approval2.0- → approval2.0?
Attachment #457137 - Flags: approval2.0? → approval2.0+
Posted patch proposed patch (obsolete) — Splinter Review
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).
Attachment #457137 - Attachment is obsolete: true
Attachment #493162 - Flags: review?
Attachment #493162 - Flags: approval2.0?
Attachment #493162 - Flags: review? → review?(dveditz)
Posted patch proposed patch (obsolete) — Splinter Review
attaching the *right* version of the patch.
Attachment #493162 - Attachment is obsolete: true
Attachment #493164 - Flags: review?(dveditz)
Attachment #493164 - Flags: approval2.0?
Attachment #493162 - Flags: review?(dveditz)
Attachment #493162 - Flags: approval2.0?
dveditz: any chance for a review?  This has been sitting around for a while.
Posted patch proposed patch (obsolete) — Splinter Review
Updated for bit rot, and made the unit tests slightly more robust.
Attachment #493164 - Attachment is obsolete: true
Attachment #507903 - Flags: review?(dveditz)
Attachment #493164 - Flags: review?(dveditz)
Attachment #493164 - Flags: approval2.0?
Comment on attachment 457137 [details] [diff] [review]
Proposed Patch

(bookkeeping)
Attachment #457137 - Flags: approval2.0+
Whiteboard: not-ready
Assignee: sstamm → 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 ?
You're free to change interfaces. Those comments were made in the context of the pre-Firefox-4 API freeze.
Posted patch sid's existing patch updated (obsolete) — Splinter Review
sid's existing patch updated for current code - passes current csp tests. if this looks ok i will r? to dveditz
Attachment #507903 - Attachment is obsolete: true
Attachment #540929 - Flags: feedback?(sstamm)
Attachment #507903 - Flags: review?(dveditz)
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.
Depends on: 674255
Depends on: 668283
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()
Attachment #540929 - Flags: feedback?(sstamm) → feedback+
No longer depends on: 668283
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.
Attachment #540929 - Attachment is obsolete: true
Attachment #558003 - Flags: review?(dveditz)
Attachment #558003 - Flags: feedback?(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!
Attachment #558003 - Flags: feedback?(sstamm) → feedback-
Posted patch latest patch (obsolete) — Splinter Review
Updated with Sid's latest feedback and a few tweaks. Still needs a try server run before asking for review.
Attachment #558003 - Attachment is obsolete: true
Attachment #559341 - Flags: feedback?(sstamm)
Attachment #558003 - Flags: review?(dveditz)
Posted patch latest patchSplinter Review
oops, missed one feedback item, this patch drops the anonymous load block as Sid pointed out previously in addition to the prev. patch.
Attachment #559341 - Attachment is obsolete: true
Attachment #559355 - Flags: feedback?(sstamm)
Attachment #559341 - Flags: feedback?(sstamm)
Duplicate of this bug: 644960
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.
Attachment #559355 - Flags: feedback?(sstamm)
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.
No longer blocks: 612391
imelven - any update here?
(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...
Assignee: imelven → nobody
Instead we should probably support n policies (instead of intersecting them), and each policy should be report-only or not.
Status: REOPENED → RESOLVED
Closed: 9 years ago7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 836922
You need to log in before you can comment on or make changes to this bug.