feat(csp): enable report only CSP in prodiction #3179

vladikoff · 2015-10-15T21:23:31Z

vladikoff · 2015-10-15T21:23:59Z

The log for this will give us:

{"op":"server.csp","time":"2015-10-15T21:00:00.000Z","blocked":"https://cdnjs.cloudflare.com"}

thoughts?

vladikoff · 2015-10-19T16:27:10Z

todo: make this work only for 10%

vladikoff · 2015-10-19T16:28:29Z

@jrgm let me know if the log format is okay

vladikoff · 2015-11-04T00:43:09Z

Updated!

shane-tomlinson · 2015-11-05T12:03:13Z

tests/server/routes/post-csp.js

+
+    assert.isFalse(postCsp.process({
+      body: {
+        'csp-report': {}


What happens if we intentionally pass bad data, like:

body: { 'csp-report': 'will this cause anything to go awry?' }

shane-tomlinson · 2015-11-05T12:05:33Z

server/bin/fxa-content-server.js

+  // correct is 'application/csp-report': https://w3c.github.io/webappsec/specs/content-security-policy/
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1192840
+  app.use(bodyParser.json({
+    type: ['json', 'application/csp-report']


If chrome sends application/csp-report, will express automatically interpret that as JSON data? If not, we'll have to manually decode the data before this line

will express automatically interpret that as JSON data?

No,
That's why this line is added, it signals that the data must be interpret as JSON for that content type.

I totally read the code incorrectly, thanks for clarifying.

shane-tomlinson · 2015-11-05T12:10:30Z

server/lib/routes/post-csp.js

+      today.setMinutes(0, 0, 0);
+
+      var entry = {
+        blocked: req.body['csp-report']['blocked-uri'],


Looking at the CSP spec, it may be useful to know the referrer of the offending script. I have a suspicion that addons like LastPass will cause CSP violations if they insert scripts into the DOM automatically, and we'll want to know that.

Which brings up the question would the referrer that causes an error be the addon's script, or would it be FxA?

shane-tomlinson · 2015-11-06T12:04:34Z

server/lib/routes/post-csp.js

+      // TODO: This is a temporary measure
+      // Not sure how many CSP errors we will get, for now we rate limit this.
+      // To avoid overflowing Heka logs rate limit the logging
+      if (isRateLimited(options.rateLimitPercentage)) {


Using isRateLimited seems a little strange and is the reverse of how metrics are handled. Metrics uses isSampledUser and a sampling percentage, whereas here we use a rate limiting percentage. 0% sample rate is no samples are let through, 0% rate limiting means all CSP reports are let through. For our own future sanity, we should use a consistent scheme in both places. I propose changing options.rateLimitPercentage to something like options.cspReportSamplePercentage

shane-tomlinson · 2015-11-06T12:46:55Z

server/lib/routes/post-csp.js

+   * @returns {boolean}
+   */
+  function isRateLimited(rateLimitPercentage) {
+    var RATE_LIMIT = rateLimitPercentage;


RATE_LIMIT isn't really a constant, so no need for the caps.

shane-tomlinson · 2015-11-06T12:51:48Z

server/lib/routes/post-csp.js

+    var min = 0;
+    var max = 100;
+
+    var rand = min + Math.floor(Math.random() * (max - min + 1));


Ahha, I see this code comes from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/random to include both endpoints. Could this be made into its own function (probably preferable) or simplified since both min and max are hard coded:

var rand = Math.floor(Math.random() * (100 + 1));

shane-tomlinson · 2015-11-06T12:54:09Z

server/lib/routes/post-csp.js

+ * Route to report CSP Violations to metrics
+ */
+
+var DEFAULT_RATE_LIMIT = 90;


This constant seems like it belongs in the lib/configuration.js csp section.

This is just a temporary measure, didn't want to add a config value, because we are planning to remove this in 1 train

shane-tomlinson · 2015-11-06T13:18:20Z

A report in Firefox:

content-server PORT 3030-3 fxa-content-server.server.requests.INFO: POST /_/csp-violation 200 0.429
content-server PORT 3030-3 {"blocked":"self","op":"server.csp","time":"2015-11-06T13:00:00.000Z"}

A report in Chrome:

content-server PORT 3030-3 fxa-content-server.server.requests.INFO: POST /_/csp-violation 200 0.898

@vladikoff - when testing in Chrome, the reports are sent, and even with rate limiting code disabled no reports are sent to Heka.

Here is the diff I'm using to test:

diff --git a/server/lib/routes/post-csp.js b/server/lib/routes/post-csp.js
index 680b34a..d794efc 100644
--- a/server/lib/routes/post-csp.js
+++ b/server/lib/routes/post-csp.js
@@ -17,7 +17,7 @@ module.exports = function (options) {
    * @returns {boolean}
    */
   function isRateLimited(rateLimitPercentage) {
-    var RATE_LIMIT = rateLimitPercentage;
+    var RATE_LIMIT = 0;//rateLimitPercentage;
     // rate limit can be 0
     if (typeof rateLimitPercentage === 'undefined') {
       RATE_LIMIT = DEFAULT_RATE_LIMIT;
@@ -38,9 +38,13 @@ module.exports = function (options) {
       // TODO: This is a temporary measure
       // Not sure how many CSP errors we will get, for now we rate limit this.
       // To avoid overflowing Heka logs rate limit the logging
+      /*
       if (isRateLimited(options.rateLimitPercentage)) {
         return false;
       }
+      */
+
+      console.log('req.body', req.body);

       if (! req.body || ! req.body['csp-report'] || ! req.body['csp-report']['blocked-uri']) {
         return false;

I use this command in the developer console to trigger a CSP violation:

var script = document.createElement('script');script.innerHTML = 'alert(1);';document.body.appendChild(script);

With the diff applied (which has logging), the console output is:

{ 'csp-report':
   { 'document-uri': 'http://127.0.0.1:3030/signup',
     referrer: '',
     'violated-directive': 'default-src \'self\'',
     'effective-directive': 'script-src',
     'original-policy': 'connect-src \'self\' http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src \'self\'; img-src \'self\' data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; report-uri /_/csp-violation',
     'blocked-uri': '',
     'status-code': 200 } }

So, for Chrome, the blocked-uri is empty.

Firefox sends:

{ 'csp-report':
   { 'blocked-uri': 'self',
     'document-uri': 'http://127.0.0.1:3030/signup',
     'line-number': 1,
     'original-policy': 'connect-src http://127.0.0.1:3030 http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src http://127.0.0.1:3030; img-src http://127.0.0.1:3030 data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; report-uri http://127.0.0.1:3030/_/csp-violation',
     referrer: '',
     'script-sample': 'alert(1);',
     'source-file': 'http://127.0.0.1:3030/signup',
     'violated-directive': 'default-src http://127.0.0.1:3030' } }

Safari sends a similar report to Chrome:

{ 'csp-report':
   { 'document-uri': 'http://127.0.0.1:3030/signup',
     referrer: '',
     'violated-directive': 'default-src \'self\'',
     'original-policy': 'connect-src \'self\' http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src \'self\'; img-src \'self\' data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; report-uri /_/csp-violation',
     'blocked-uri': '',
     'source-file': '',
     'line-number': 1 } }

With this info, I think we should generate a CSP report even if blocked-uri is empty. We should perhaps report blocked-uri if available and violated-directive always?

Finally, the answer to my question about what happens if an addon causes a CSP violation. One of my addons is causing this report to be generated:

{ 'csp-report':
   { 'blocked-uri': 'self',
     'document-uri': 'http://127.0.0.1:3030/signup',
     'line-number': 1,
     'original-policy': 'connect-src http://127.0.0.1:3030 http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src http://127.0.0.1:3030; img-src http://127.0.0.1:3030 data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; report-uri http://127.0.0.1:3030/_/csp-violation',
     referrer: '',
     'script-sample': '(function () {\n\n    var event_id = docum...',
     'source-file': 'http://127.0.0.1:3030/signup',
     'violated-directive': 'default-src http://127.0.0.1:3030' } }

The referrer is empty.

vladikoff · 2015-11-09T17:21:00Z

from mtg: could 'violated-directive' be useful?

vladikoff · 2015-11-09T17:23:02Z

@jrgm how do we keep consistent with Firefox Hello CSP logging? What is the format?

Fixes #1426

vladikoff · 2015-11-12T21:18:05Z

Updated!

shane-tomlinson · 2015-11-13T10:23:24Z

With no addons, we have problems in Firefox Beta.

content-server PORT 3030-3 {"blocked":"self","op":"server.csp","referrer":"http://127.0.0.1:3030/styles/main.css","time":"2015-11-13T10:00:00.000Z","violated":"default-src http://127.0.0.1:3030"}

shane-tomlinson · 2015-11-13T10:24:21Z

More context - the above problem is on the /settings page and the .svg avatar seems to cause the problem.

shane-tomlinson · 2015-11-13T10:46:25Z

Here is the CSP report filed by Firefox:

 "csp-report": {
    "blocked-uri": "self",
    "document-uri": "http://127.0.0.1:3030/images/default-profile.svg",
    "line-number": 1,
    "original-policy": "connect-src http://127.0.0.1:3030 http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src http://127.0.0.1:3030; img-src http://127.0.0.1:3030 data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; report-uri http://127.0.0.1:3030/_/csp-violation",
    "referrer": "",
    "script-sample": ".st0{fill:#C3CFD8;} .st1{fill-rule:eveno...",
    "source-file": "http://127.0.0.1:3030/images/default-profile.svg",
    "violated-directive": "default-src http://127.0.0.1:3030"
  }

The .svg file contains an inline style, which is verboten by our CSP rules.

<style xmlns="http://www.w3.org/2000/svg">.st0{fill:#C3CFD8;} .st1{fill-rule:evenodd;clip-rule:evenodd;fill:#FFFFFF;}</style>

Here is relevant the portion of the W3C CSP spec: http://www.w3.org/TR/CSP/#directive-style-src

We can:

ditch the .svg for a .png or .jpg
allow unsafe-inline for style-src
edit the .svg to add a valid hash, and add a hash-src directive
edit the .svg to add a valid nonce, and add a nonce-src directive

shane-tomlinson · 2015-11-13T11:11:33Z

V3 of the CSP spec: https://w3c.github.io/webappsec-csp/#directive-style-src

shane-tomlinson · 2015-11-13T11:20:55Z

I was wrong, hash-src and nonce-src are types of source, not the directive themselves. I can avoid the CSP violation in Chrome only with the following lines added to the CSP configuration:

  styleSrc: [
    SELF,
    "'sha256-9n6ek6ecEYlqel7uDyKLy6fdGNo3vw/uScXSq9ooQlk=='"
  ],

shane-tomlinson · 2015-11-13T11:22:48Z

https://bugzilla.mozilla.org/show_bug.cgi?id=958702 - CSP 1.1: (followup) hash-source on multiprocess Gecko

shane-tomlinson · 2015-11-13T11:27:00Z

@vladikoff - while tooling around trying to figure out the CSP problem, I had the following patch:

diff --git a/server/lib/configuration.js b/server/lib/configuration.js
index 4b4a8f6..75ccc31 100644
--- a/server/lib/configuration.js
+++ b/server/lib/configuration.js
@@ -65,6 +65,10 @@ var conf = module.exports = convict({
       default: false,
       doc: 'Only send the "Content-Security-Policy-Report-Only" header'
     },
+    reportSampleRate: {
+      default: 10,
+      doc: 'Sample rate at which CSP violation reports should be logged'
+    },
     reportUri: {
       default: '/_/csp-violation',
       doc: 'Location of "report-uri"'
diff --git a/server/lib/routes.js b/server/lib/routes.js
index e9cfefd..4fc00d2 100644
--- a/server/lib/routes.js
+++ b/server/lib/routes.js
@@ -27,7 +27,10 @@ module.exports = function (config, i18n) {
     require('./routes/get-config')(i18n),
     require('./routes/get-client.json')(i18n),
     require('./routes/post-metrics')(),
-    require('./routes/post-csp')(),
+    require('./routes/post-csp')({
+      path: config.get('csp.reportUri'),
+      reportSampleRate: config.get('csp.reportSampleRate')
+    }),
     require('./routes/get-metrics-errors')(),
     require('./routes/get-openid-login')(config),
     require('./routes/get-openid-authenticate')(config)
diff --git a/server/lib/routes/post-csp.js b/server/lib/routes/post-csp.js
index e49138c..2110ada 100644
--- a/server/lib/routes/post-csp.js
+++ b/server/lib/routes/post-csp.js
@@ -6,38 +6,30 @@
  * Route to report CSP Violations to metrics
  */

-var DEFAULT_SAMPLE_RATE = 10;
-
 module.exports = function (options) {
   options = options || {};

   /**
-   * 'sampleRate' % of messages.
-   * @param sampleRate
+   * 'reportSampleRate' % of messages.
+   * @param reportSampleRate
    * @returns {boolean}
    */
-  function isSampledUser(sampleRate) {
-    var rateLimit = sampleRate;
-    // rate limit can be 0
-    if (typeof sampleRate === 'undefined') {
-      rateLimit = DEFAULT_SAMPLE_RATE;
-    }
-
+  function isSampledUser() {
     // random between 0 and 100, inclusive
     var rand = Math.floor(Math.random() * (100 + 1));
-    return rand <= rateLimit;
+    return rand <= options.reportSampleRate;
   }

   return {
     method: 'post',
-    path: '/_/csp-violation',
+    path: options.path,
     process: function (req, res) {
       res.json({result: 'ok'});

       // TODO: This is a temporary measure
       // Not sure how many CSP errors we will get, for now we rate limit this.
       // To avoid overflowing Heka logs rate limit the logging
-      if (! isSampledUser(options.sampleRate)) {
+      if (! isSampledUser()) {
         return false;
       }

diff --git a/tests/server/routes/post-csp.js b/tests/server/routes/post-csp.js
index c1b57ae..ad9fdab 100644
--- a/tests/server/routes/post-csp.js
+++ b/tests/server/routes/post-csp.js
@@ -30,7 +30,7 @@ define([

   suite['it drops if no csp-report set'] = function () {
     var options = {
-      sampleRate: 100
+      reportSampleRate: 100
     };
     var postCsp = proxyquire(path.join(process.cwd(), 'server', 'lib', 'routes', 'post-csp'), {})(options);
     assert.isFalse(postCsp.process({
@@ -49,7 +49,7 @@ define([

   suite['it allows no messages with 0% sample rate '] = function () {
     var options = {
-      sampleRate: 0
+      reportSampleRate: 0
     };
     var postCsp = proxyquire(path.join(process.cwd(), 'server', 'lib', 'routes', 'post-csp'), {})(options);

@@ -63,7 +63,7 @@ define([

   suite['it allows all messages with 100% sample rate'] = function () {
     var options = {
-      sampleRate: 100
+      reportSampleRate: 100
     };

     var postCsp = proxyquire(path.join(process.cwd(), 'server', 'lib', 'routes', 'post-csp'), {})(options);

The changes:

sampleRate=>reportSampleRate to be more specific about what's being sampled.
Move the 10% number to configuration. We can take it out later if we want.
The route's path was in 2 places, in configuration and hard coded in the route. Rip out the hard coded path and use configuration's so only one source of truth exists.

shane-tomlinson · 2015-11-13T11:49:40Z

More info - A hash-source problem was fixed in Fx 43 - https://bugzilla.mozilla.org/show_bug.cgi?id=1026520

I can avoid the CSP error in Firefox with the following styleSrc directive:

  styleSrc: [
      SELF,
      "'sha256-9n6ek6ecEYlqel7uDyKLy6fdGNo3vw/uScXSq9ooQlk='"
  ]

I had an extra = on the end that Chrome didn't care about.

Oddly, with full CSP enabled and the above styleSrc, Safari refuses to load the svg as a standalone page because of a CSP violation. If I load /settings, the image is shown anyways. Works fine on Mobile safari too.

shane-tomlinson · 2015-11-13T11:52:50Z

With the above styleSrc applied and full CSP enabled, the default avatar shows in Fx 18, Fx 20, no reports are generated. We can move forward!

shane-tomlinson · 2015-11-13T13:19:53Z

Closing, re-opening against origin.

vladikoff added the waffle:progress label Oct 15, 2015

vladikoff force-pushed the vladikoff:i1426 branch from 3cd0c98 to 2e33ab6 Oct 16, 2015

vladikoff self-assigned this Oct 26, 2015

vladikoff force-pushed the vladikoff:i1426 branch 2 times, most recently from 37b83f3 to 647d027 Nov 3, 2015

vladikoff assigned shane-tomlinson and unassigned vladikoff Nov 4, 2015

shane-tomlinson reviewed Nov 5, 2015
View changes

shane-tomlinson reviewed Nov 6, 2015
View changes

server/lib/routes/post-csp.js

* @returns {boolean}

*/

function isRateLimited(rateLimitPercentage) {

var RATE_LIMIT = rateLimitPercentage;

This comment has been minimized.

Sign in to view

shane-tomlinson Nov 6, 2015
Member

RATE_LIMIT isn't really a constant, so no need for the caps.

shane-tomlinson reviewed Nov 6, 2015
View changes

shane-tomlinson assigned vladikoff and unassigned shane-tomlinson Nov 9, 2015

feat(csp): enable report only CSP in prodiction

69ef16e

Fixes #1426

vladikoff force-pushed the vladikoff:i1426 branch from 647d027 to 69ef16e Nov 12, 2015

vladikoff assigned shane-tomlinson and unassigned vladikoff Nov 12, 2015

shane-tomlinson closed this Nov 13, 2015

shane-tomlinson removed the waffle:progress label Nov 13, 2015

shane-tomlinson mentioned this pull request Nov 13, 2015

feat(csp): enable report only CSP in prodiction #3286

Merged

mozilla / fxa-content-server Archived

feat(csp): enable report only CSP in prodiction #3179

feat(csp): enable report only CSP in prodiction #3179

vladikoff commented Oct 15, 2015

vladikoff commented Oct 15, 2015

vladikoff commented Oct 19, 2015

vladikoff commented Oct 19, 2015

vladikoff commented Nov 4, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented Nov 6, 2015

vladikoff commented Nov 9, 2015

vladikoff commented Nov 9, 2015

vladikoff commented Nov 12, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

mozilla / fxa-content-server Archived

feat(csp): enable report only CSP in prodiction #3179

feat(csp): enable report only CSP in prodiction #3179

Conversation

vladikoff commented Oct 15, 2015

vladikoff commented Oct 15, 2015

vladikoff commented Oct 19, 2015

vladikoff commented Oct 19, 2015

vladikoff commented Nov 4, 2015

This comment has been minimized.

shane-tomlinson Nov 5, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 5, 2015 Member

This comment has been minimized.

vladikoff Nov 5, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson Nov 6, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 5, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 5, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 6, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 6, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 6, 2015 Member

This comment has been minimized.

shane-tomlinson Nov 6, 2015 Member

This comment has been minimized.

vladikoff Nov 9, 2015 Author Contributor

shane-tomlinson commented Nov 6, 2015

vladikoff commented Nov 9, 2015

vladikoff commented Nov 9, 2015

vladikoff commented Nov 12, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson commented Nov 13, 2015

shane-tomlinson Nov 5, 2015
Member

shane-tomlinson Nov 5, 2015
Member

vladikoff Nov 5, 2015
Author Contributor

shane-tomlinson Nov 6, 2015
Member

shane-tomlinson Nov 5, 2015
Member

shane-tomlinson Nov 5, 2015
Member

shane-tomlinson Nov 6, 2015
Member

shane-tomlinson Nov 6, 2015
Member

shane-tomlinson Nov 6, 2015
Member

shane-tomlinson Nov 6, 2015
Member

vladikoff Nov 9, 2015
Author Contributor