Closed Bug 836922 Opened 11 years ago Closed 11 years ago

CSP : support multiple policies

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
relnote-firefox --- 26+

People

(Reporter: imelven, Assigned: geekboy)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 6 obsolete files)

See https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#enforcing-multiple-policies.

CSP allows multiple policies being specified for a resource, including via the Content-Security-Policy header, the Content-Security-Policy-Report-Only header and (proposed) a meta tag.

Our CSP implementation should support multiple policies, including the case of both an enforced and Report-Only policy, per the spec.
Priority: -- → P2
Depends on: CSP
Isn't this a requirement for CSP 1.0 conformance?!

http://www.w3.org/TR/CSP/#policy-delivery  (or https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-1.0-specification.html#policy-delivery when that gets replaced with the 1.1 version)

Section 3.1 says a site MAY send more than one CSP and MAY send more than one CSP-report-only header, and "the user agent MUST enforce each of the policies contained in each such header field" (or "monitor" rather than "enforce" in the report-only case).
Blocks: csp-w3c-1.0, CSP
No longer depends on: CSP
We currently support multiple policies, but poorly (we merge them into one).  This bug is about separating them, removing the intersection logic, and enforcing multiple policies in parallel.  It will also make bug 552523 trivial.
Assignee: nobody → sstamm
Attached patch multipolicy (obsolete) — Splinter Review
First whack -- this retains the semantics of the refinePolicy() era, but supports multiple separate policies inside a single nsIContentSecurityPolicy.

Still todo:
* write tests for having multiple policies with different report requirements
* enable CSP report-only and regular mode at the same time
* enable X- and non-prefixed policies at the same time (maybe?)
* refactor InitCSP() code in nsDocument.cpp to be far more readable.
Comment on attachment 784696 [details] [diff] [review]
multipolicy

Garrett -- if you get time, take a look and let me know what you think about the idl changes.  I'm not sure how I feel about them, but couldn't think of a different way to do this.

Low priority, feel free to clear the flag if you don't have time (this patch is 92kb).
Attachment #784696 - Flags: feedback?(grobinson)
Attached patch multipolicy (obsolete) — Splinter Review
This patch cleans up some of the code and:

> * write tests for having multiple policies with different 
> report requirements
Shortly, I'll post another patch with tests for multiple policies.

> * enable CSP report-only and regular mode at the same time
done.

> * enable X- and non-prefixed policies at the same time (maybe?)
done.  Supports app policies and header policies in tandem too.

> * refactor InitCSP() code in nsDocument.cpp to be far more readable.
done.  I was able to pull a lot of weird logic out and make it simpler.


Quick overview of what this does:
* Fixes bug 552523
* Moves the _reportOnlyFlag into each CSPRep (instead of in the impl of nsIContentSecurityPolicy)
* Removes refinePolicy().  Adds appendPolicy() and removePolicy()
* Updated getters for inline/eval to return an array of booleans to indicate which policies want reports to be generated (for inline blocking where the reports are sent by the caller).  Also updated reporting infrastructure to include a policy index/id.

I think a follow-up to this work would be to remove the intersection logic (yuck) since nothing outside of CSP uses that logic and refinePolicy is toast.
Attachment #784696 - Attachment is obsolete: true
Attachment #784696 - Flags: feedback?(grobinson)
Attachment #785199 - Flags: feedback?(grobinson)
Attached patch tests for multiple policies (obsolete) — Splinter Review
Tests (ruthlessly adapted from Ian's tests in bug 552523 -- thanks Ian!) to measure enforcement and report-generation for two policies in parallel: one report-only and one enforced policy.
Attachment #785201 - Flags: review?(grobinson)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> Created attachment 785201 [details] [diff] [review]
> tests for multiple policies

Oh, those edits to CSPUtils.jsm?  Ignore them.  I forgot to qref to remove those edits.
Well, this deletes the intersection logic.  Did I miss anything, Garrett?
Attachment #785212 - Flags: review?(grobinson)
Attachment #785199 - Flags: feedback?(grobinson) → review?(jst)
Comment on attachment 785199 [details] [diff] [review]
multipolicy

Garrett: can you review the csp bits of this patch?  I suckered jst into the rest of it.
Attachment #785199 - Flags: review?(grobinson)
Try run was clean, but mac builds barfed due to an unused member var.  Fixed with one line tweak to remove the declaration of mPolicyIndex, but not gonna upload new versions of the patch for this (will update after reviews).

Offending line:
https://hg.mozilla.org/try/rev/f9d002c127cf#l14.12

Pushed back to try again on mac only:
https://tbpl.mozilla.org/?tree=Try&rev=d2fcd8144d28
Comment on attachment 785199 [details] [diff] [review]
multipolicy

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

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +79,5 @@
>     *     Whether or not the effects of the inline script should be allowed
>     *     (block the compilation if false).
>     */
> +  boolean getAllowsInlineScript(out unsigned long aLength,
> +                                [array,size_is(aLength)] out boolean shouldReportViolations);

Dumb xpcom question: does nsIArray not work here? It would let you eliminate the aLength param here and below.
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #13)
> Dumb xpcom question: does nsIArray not work here? It would let you eliminate
> the aLength param here and below.

We could, but the nsIArray would require an nsISupports subclass for the elements and radically increase the number of XPCOM calls during iteration through this set of booleans.  :(  Maybe jst knows if this is a better idea...
Comment on attachment 785201 [details] [diff] [review]
tests for multiple policies

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

Overall, these tests look good and have good coverage. One question: should the violation reporting go in these tests, or should it be with the rest of the violation reporting in http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_cspreports.js ?

The two .sjs files are completely identical except for their state key. An alternative would be to include a variable indicating ro or not in the query string of each policy's report-uri, and then branch on that in a single .sjs file. It is good to avoid copy-pasting large chunks of code, although I understand if you'd rather leave it as it is now.

::: content/base/src/CSPUtils.jsm
@@ +103,2 @@
>  
> +  dump(aMsg + "\r\n");

Revert this (leftovers from debugging?)

::: content/base/test/file_bug836922_npolicies.html^headers^
@@ +1,2 @@
> +x-content-security-policy: allow 'self'; img-src 'none'; report-uri http://mochi.test:8888/tests/content/base/test/file_bug836922_npolicies_violation.sjs
> +x-content-security-policy-report-only: allow *; img-src 'self'; script-src 'none'; report-uri http://mochi.test:8888/tests/content/base/test/file_bug836922_npolicies_ro_violation.sjs

Should we be testing the 1.0 headers additionally (or instead)?

::: content/base/test/file_bug836922_npolicies_ro_violation.sjs
@@ +4,5 @@
> +const BinaryInputStream = CC("@mozilla.org/binaryinputstream;1",
> +                              "nsIBinaryInputStream",
> +                              "setInputStream");
> +
> +const STATE = "bug836922_ro_violations";

STATE_KEY might be a better name for this variable (it took me a minute to figure out what voodoo was going on here).

@@ +14,5 @@
> +    var [name, value] = val.split('=');
> +    query[name] = unescape(value);
> +  });
> +
> +

Nit: unnecessary newline.

@@ +47,5 @@
> +    // store the violation in the persistent state
> +    var s = getState(STATE);
> +    if (!s) s = "{}";
> +    s = JSON.parse(s);
> +    if (!s) s = {};

You could simplify these 4 lines to:
  var s = JSON.parse(getState(STATE) || "{}");
getState returns "" for all keys by default, so the short circuit logic handles the first two lines.

@@ +50,5 @@
> +    s = JSON.parse(s);
> +    if (!s) s = {};
> +
> +    if (!s[testid]) s[testid] = 0;
> +    s[testid]++;

You could simplify these two lines to
  s[testid] ? s[testid]++ : s[testid] = 1;

::: content/base/test/test_bug836922_npolicies.html
@@ +17,5 @@
> +var path = "/tests/content/base/test/";
> +
> +// These are test results: verified indicates whether or not the test has run.
> +// true/false is the pass/fail result.
> +window.loads = {

It's not necessary to attach this object to window, but it doesn't hurt either.

@@ +59,5 @@
> +                        "URI.asciiSpec");
> +      if (!testpat.test(asciiSpec)) return;
> +      var testid = testpat.exec(asciiSpec)[1];
> +
> +      //violation reports don't come through here, but the requested resources do

Nits: space after comment marker and capitalization. Please fix all occurences in this file.

@@ +72,5 @@
> +      window.testResult(testid, 'allowed', asciiSpec + " allowed by csp");
> +    }
> +
> +    if(topic === "csp-on-violate-policy") {
> +      //these were NOT NECESSARILY blocked, but rather a policy was violated...

Clarify "if the violated policy was report-only, the resource will still be loaded" or similar. Helps people who don't have CSP internalized read these tests :)

@@ +88,5 @@
> +      // record the ones that were supposed to be blocked, but don't use this
> +      // as an indicator for tests that are not blocked but do generate reports.
> +      // We skip recording the result if the load is expected since a
> +      // report-only policy will generate a request *and* a violation note.
> +      if (!window.loads[testid].expected) {

What happens if a test that is not supposed to be blocked is blocked? Will the whole test end up timing out?

@@ +248,5 @@
> +  {'set':[["security.csp.speccompliant", true]]},
> +   function() {
> +    // save this for last so that our listeners are registered.
> +    // ... this loads the testbed of good and bad requests.
> +    document.getElementById('cspframe').src = 'http://mochi.test:8888' + path + 'file_bug836922_npolicies.html';

Is there a particular reason why we need the mochi.test server path?
  document.getElementById('cspframe').src = "file_bug836922_npolicies.html"
should work just as well, as it is in the same directory as this Mochitest.
Attachment #785201 - Flags: review?(grobinson) → review+
Comment on attachment 785201 [details] [diff] [review]
tests for multiple policies

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

Thanks Garrett!  Looking forward to your feedback on the other patches here.

::: content/base/src/CSPUtils.jsm
@@ +103,2 @@
>  
> +  dump(aMsg + "\r\n");

Yeah, see comment 8.

::: content/base/test/file_bug836922_npolicies_ro_violation.sjs
@@ +4,5 @@
> +const BinaryInputStream = CC("@mozilla.org/binaryinputstream;1",
> +                              "nsIBinaryInputStream",
> +                              "setInputStream");
> +
> +const STATE = "bug836922_ro_violations";

Maybe it makes sense to also add a comment here that says: "since many httpd mochitest handlers use a common state store (getState/setState), this is the key for this test's request-persistent storage"
Blocks: 801783
Attached patch 2 - tests for multiple policies (obsolete) — Splinter Review
Thanks for the review, Garrett!  I updated the patch accordingly.

(In reply to Garrett Robinson [:grobinson] from comment #15)
> @@ +88,5 @@
> > +      // record the ones that were supposed to be blocked, but don't use this
> > +      // as an indicator for tests that are not blocked but do generate reports.
> > +      // We skip recording the result if the load is expected since a
> > +      // report-only policy will generate a request *and* a violation note.
> > +      if (!window.loads[testid].expected) {
> 
> What happens if a test that is not supposed to be blocked is blocked? Will
> the whole test end up timing out?

Yes.  Since reporting is async, there's no guarantee of "when", so we have to wait until the test times out.  But I think that's okay -- we want the tests to break horribly if too much gets blocked.  We don't have a notification topic for "item was blocked", and I don't think we want to add one at this point.

> > +    document.getElementById('cspframe').src = 'http://mochi.test:8888' + path + 'file_bug836922_npolicies.html';
> 
> Is there a particular reason why we need the mochi.test server path?
>   document.getElementById('cspframe').src = "file_bug836922_npolicies.html"
> should work just as well, as it is in the same directory as this Mochitest.

I think the idea was to explicitly define "self" to be mochi.test:8888 to not mess up the tests.  Mochitests can be run under multiple hostnames.  I've changed it to be relative URLs since mochit.test:8888 is standard, but this might be ever so slightly more fragile.

Carrying over r=grobinson on this refreshed tests patch.
Attachment #785201 - Attachment is obsolete: true
Attachment #796153 - Flags: review+
Attachment #796153 - Attachment description: tests for multiple policies → 2 - tests for multiple policies
Attachment #785212 - Attachment description: remove intersectsWith → 3 - remove intersectsWith
Attached patch 1- multipolicy (obsolete) — Splinter Review
Was some wicked bitrot due to changes to nsCrypto.cpp and a few other files.  Updated patch to deal with it.

jst, grobinson: any idea when I can expect some feedback on this?
Attachment #785199 - Attachment is obsolete: true
Attachment #785199 - Flags: review?(jst)
Attachment #785199 - Flags: review?(grobinson)
Attachment #796160 - Flags: review?(jst)
Attachment #796160 - Flags: review?(grobinson)
Hey Sid, I'm generally pleased with this change. The one thing that is unfortunate is the pattern of passing back an array of booleans and needing to log violation details selectively based on whether the bit is set in that array etc. The other approaches I can see there are not necessarily better, the first one being to pass in a callback object that can do the violation logging, this callback object could have enough context to do the logging once called, and this could be done w/o too too much overhead. The second options would be to pass in enough context info to the various getAllows* methods, but that requires constructing the error messages etc ahead of time even in the cases (i.e. the common case) where it won't be needed, thus this is more expensive than I'd like.

I'd say the first one of those options as a reasonable one, but it does mean writing a bunch more classes, all of which likely need to be cycle collected etc. So more gunk to maintain etc. The latter options is probably too expensive for our liking here. And leaving things as written is certainly an option too, and very possibly the best one here.

Thoughts?
Yeah, I don't like the boolean arrays either; makes call sites ugly. To clarify, here are the options on the table:
1. Callback objects
2. Pre-generate context (pass it in)
3. Boolean return value arrays

I'd like to rule out #2 since it is problematic (and was our original design, but was a perf hit).

Another option:
4. Two-step.  Go back to the original single-boolean outparam for "need to send reports" and create another function "sendReports()" that takes the context and sends reports for the relevant policies.  This causes extra xpcom overhead for violations (maybe it's fine), eliminates an array outparam, and keeps the context generation conditional on "needing" reports.

IMO, I'd like to keep the boolean arrays around.  They're only necessary for inline script, inline style and eval so there are limited call sites.  Maybe we could abstract out the looping logic into a macro or library function to make it cleaner/prettier.

My second choice would be the callback option.  That seems like extra boilerplate, but perhaps easier to read than the boolean array outparams.  Which do you prefer?
Flags: needinfo?(jst)
I'm having a hard time seeing any downsides to your suggested approach number 4 here, other than a few extra XPCOM method calls, but I don't worry about that at all in failure cases. I know taking that approach means extra work here at this point, but it'd also remove the increased brain print of this code, i.e. not having to deal with XPCOM and boolean arrays and making sure the memory allocated for them gets freed etc. From what I can tell not a single call site does anything policy specific inside the loop over the policies, so one could even theorize that building the violation context once and passing it on to something that will report said violations for multiple policies will even be less overhead than the array approach.

Let me know what you think, but that's where I'm leaning. If there's benefit in landing this with the array approach first and changing that approach in a followup bug then I could get on board with that, but it would mean more API churn here, which may or may not matter (do extensions ever touch these APIs?).
Flags: needinfo?(jst)
Comment on attachment 796160 [details] [diff] [review]
1- multipolicy

Clearing review flags until I can upload a new patch with the option 4 api changes (two-step reporting).
Attachment #796160 - Flags: review?(jst)
Attachment #796160 - Flags: review?(grobinson)
Attached patch 1 - multipolicy (obsolete) — Splinter Review
Okay, this patch changes the api so logViolationDetails does all the looping (and also re-checks each policy for violations).  The upshot is that the callers to allowsEval and friends will not have to deal with boolean arrays.  This is option 4 from comment 20.

jst: this is what we discussed.  I'd appreciate any feedback and also any review on things not in CSP files.
Attachment #796160 - Attachment is obsolete: true
Attachment #800241 - Flags: review?(jst)
Attachment #800241 - Flags: review?(grobinson)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #17)
> > > +    document.getElementById('cspframe').src = 'http://mochi.test:8888' + path + 'file_bug836922_npolicies.html';
> > 
> > Is there a particular reason why we need the mochi.test server path?
> >   document.getElementById('cspframe').src = "file_bug836922_npolicies.html"
> > should work just as well, as it is in the same directoraty as this Mochitest.
> 
> I think the idea was to explicitly define "self" to be mochi.test:8888 to
> not mess up the tests.  Mochitests can be run under multiple hostnames. 
> I've changed it to be relative URLs since mochit.test:8888 is standard, but
> this might be ever so slightly more fragile.
> 

That is reasonable. I agree that we could go either way, and that the relative URLs might be a tiny bit more fragile. For future reference (since this always confuses me), the Mochitest proxy behavior is documented here: https://developer.mozilla.org/en/docs/Mochitest#How_do_I_test_issues_which_only_show_up_when_tests_are_run_across_domains.3F.
Comment on attachment 800241 [details] [diff] [review]
1 - multipolicy

This looks great! r=jst, with a couple of style nits that stood out while reading through this.

- In nsScriptSecurityManager::ContentSecurity...:
         csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_EVAL,
-                                 fileName,
-                                 scriptSample,
-                                 lineNum);
+                                fileName,
+                                scriptSample,
+                                lineNum);

Looks like the indentation got off by one here?

- In CSPService::ShouldLoad():

     if (node) {
         principal = node->NodePrincipal();
         principal->GetCsp(getter_AddRefs(csp));
 
         if (csp) {
 #ifdef PR_LOGGING
-            nsAutoString policy;
-            csp->GetPolicy(policy);
-            PR_LOG(gCspPRLog, PR_LOG_DEBUG,
-                    ("Document has CSP: %s",
-                     NS_ConvertUTF16toUTF8(policy).get()));
+          {
+            int numPolicies = 0;
+            nsresult rv = csp->GetPolicyCount(&numPolicies);
+            if (NS_SUCCEEDED(rv)) {
+              for (int i=0; i<numPolicies; i++) {
+                nsAutoString policy;
...

Inconsistent 2 vs 4 space indentation here (in surrounding code, not your additions). Leave it, or fix the whole file? Your call.

- In nsScriptLoader::ProcessScriptElement():
       csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT,
-                               NS_ConvertUTF8toUTF16(asciiSpec),
-                               scriptText,
-                               aElement->GetScriptLineNumber());
+                              NS_ConvertUTF8toUTF16(asciiSpec),
+                              scriptText,
+                              aElement->GetScriptLineNumber());

Indentation off by one.

- In nsEventListenerManager::SetEventHandler():

         csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT,
-                                 NS_ConvertUTF8toUTF16(asciiSpec),
-                                 scriptSample,
-                                 0);
+                                  NS_ConvertUTF8toUTF16(asciiSpec),
+                                  scriptSample,
+                                  0);

Indetation off by one.

Same deal in nsJSThunk::EvaluateScript() and nsCrypto::GenerateCRMFRequest().
Attachment #800241 - Flags: review?(jst) → review+
Thanks, jst.  Waiting on grobinson before making the whitespace changes.

Also, noticed that the tests fail on b2g:
33388 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug836922_npolicies.html
uncaught exception - NS_ERROR_NOT_IMPLEMENTED: 
Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) 
[nsIObserverService.addObserver] at chrome://specialpowers/content/specialpowersAPI.js:925

So I'll have to adjust the test to work with b2g or disable it until we fix the observer service.
Status: NEW → ASSIGNED
Comment on attachment 800241 [details] [diff] [review]
1 - multipolicy

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

Other than echoing jst's style notes, this looks great. I really like how this simplifies a lot of logic and think this will help us avoid little bugs like those we've been seeing a lot of recently in the future.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +9,5 @@
>  interface nsIDocShell;
>  
>  /**
>   * nsIContentSecurityPolicy
> + * Describes an XPCOM component used to model an enforce CSPs.  Instances of

Nit: "an" "and"
Attachment #800241 - Flags: review?(grobinson) → review+
Comment on attachment 785212 [details] [diff] [review]
3 - remove intersectsWith

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

Full of win.
Attachment #785212 - Flags: review?(grobinson) → review+
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #26)
> Comment on attachment 800241 [details] [diff] [review]

You shoulda punched me for adding churn to files by renaming variables!  A bunch of those screwed up whitespace edits were in files not otherwise edited except for changing a variable name.  I will reverse those edits to make this cause less churn before I land this stuff.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #26)
> Comment on attachment 800241 [details] [diff] [review]
...
> Inconsistent 2 vs 4 space indentation here (in surrounding code, not your
> additions). Leave it, or fix the whole file? Your call.

I'll fix it.  I'll make it all 2 spaces to make the modeline happy.
QA Contact: mwobensmith
Attached patch 1 - multipolicySplinter Review
Updated multipolicy patch with review comments addressed.  Carrying over r=jst,grobinson
Attachment #800241 - Attachment is obsolete: true
Attachment #802677 - Flags: review+
moved tests into content/base/test/csp.  Carrying over r=grobinson.
Attachment #796153 - Attachment is obsolete: true
Attachment #802680 - Flags: review+
Android builds failed on try, but everything else is good:
https://tbpl.mozilla.org/?tree=Try&rev=b208431fe7fb

Not sure why, didn't fail last time I pushed this to try.  Gonna try Android again before pushing to inbound.
https://tbpl.mozilla.org/?tree=Try&rev=753f0144a6b2
Looks like the bustage was due to the mozilla-central revision I had checked out:
https://tbpl.mozilla.org/?rev=740094c07328
Tests accidentally ran on b2g, caused bustage.  Disabled on b2g with trivial follow-up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0af9e0a324
Depends on: 916446
Depends on: 916881
Depends on: 924708
Depends on: 925186
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: