Peertube: Implement Content Security Policy (CSP)

[Nutomic]

This would improve the security, and prevent XSS attacks. Here's a good overview:

https://www.html5rocks.com/en/tutorials/security/content-security-policy/

Also check out these links:
https://infosec.mozilla.org/guidelines/web_security#content-security-policy
https://observatory.mozilla.org/analyze.html?host=peertube.social

[rigelk]

I already had a try at it some time ago, but it introduced errors at that time so we rolled it back: https://github.com/Chocobozzz/PeerTube/commit/4bdd9473fdecfa7e309e3c59b05b29d0a20ac397 :sweat:

[rigelk]

I tried https://addons.mozilla.org/en-US/firefox/addon/laboratory-by-mozilla/ and it is really interesting to see how the CSP evolves as you record a session on a PeerTube instance. Kudos to that neat extension.

Unsurprisingly, script-src and style-src are bound to use 'self' 'unsafe-inline' which is far from ideal, but which we can't do without.

What is interesting is the connect-src. Since PeerTube's client loads the video from the origin instance, the CSP should hold all intances's URLs the instance knows about (even generating it per watch page sounds overly complex). That's unrealistic of course, so for now we should use 'connect-src': *.

What do you think about the following?

contentSecurityPolicy: {
  directives: {
    defaultSrc: ['*'],
    mediaSrc: ["'self'"],
    fontSrc: ["'self' data:"],
    imgSrc: ["'self' data:"],
    scriptSrc: ["'self' 'unsafe-inline'"],
    styleSrc: ["'self' 'unsafe-inline'"]
  }
}

[Nutomic]

To be honest I have no idea how CSP works, other than the article I linked above. But it's probably better to have a basic policy than having nothing. And you should also be able to limit child-src and some others.

[rigelk]

// Security middleware
app.use(helmet({
  frameguard: {
    action: 'deny' // we only allow it for /videos/embed, see server/controllers/client.ts
  },
  hsts: false,
  contentSecurityPolicy: {
    directives: {
      defaultSrc: ['*'], // by default, not specifying default-src = '*'
      mediaSrc: ["'self'"],
      fontSrc: ["'self' data:"],
      imgSrc: ["'self' data:"],
      scriptSrc: ["'self' 'unsafe-inline'"],
      styleSrc: ["'self' 'unsafe-inline'"],
      objectSrc: ["'none'"],
      pluginTypes: ["'none'"],
      manifestSrc: ["'self'"],
      frameSrc: ["'none'"], // instead of deprecated child-src
      workerSrc: ["'self'"], // instead of deprecated child-src
      upgradeInsecureRequests: true
    },
    browserSniff: false // assumes a modern browser, but allows CDN in front
  },
  referrerPolicy: {
    policy: 'strict-origin-when-cross-origin'
  }
}))

That should be a correct configuration of the CSP (and bonus referrerPolicy to test too) - but would need to be tested on a live instance first.

[Nutomic]

How about merging this for the next release with Content-Security-Policy-Report-Only, and logging the violations somewhere?

[rigelk]

I don't think testing through releases is a good process… and we've frozen merges (except bugfixes) until the next release anyway.

[Nutomic]

I could test it on my instance, but I'm not sure how to compile Peertube for development (without breaking anything). And it would still need some way to log the violations on the server.

[rigelk]

Please don't test it on your instance, that's too hazardous, especially with an instance that big.

I'll check how to create a reporting endpoint.

[Nutomic]

It should be fine to test with Content-Security-Policy-Report-Only, as that only reports violations, but doesn't enforce the policy (it's in the first link if you haven't read it).

[rigelk]

You could use https://gist.github.com/rigelk/54b798bf8098fde431330b3868db133f with https://github.com/mozilla/csp-logger in standalone mode. Since we're only modifying ./server.ts, rebuilding the server can be done with npm run build:server.

[ghost]

@rigelk you don't need a reporting endpoint.
A browser respecting CSP will log the error in the developer console.

using chrome/chromium, you also will get nice error messages.
The messages in firefox aren't that helpful.

[Nutomic]

@rigelk is that code ignored in debug mode? I'm trying to extract the plain header and set it via Nginx on the server, because that's easier (and imo less risky) than recompiling.

@BO41 that means users would actually have to report the errors.

[rigelk]

@Nutomic by debug mode, your mean developing it locally on your computer with npm run dev? The code is not ignored in that case, it is just only applied to the API endpoint: localhost:9000. (it's because we serve the client through angular directly, since it's able to do hot reload and more)

[Nutomic]

@rigelk ah okay, I was only checking localhost:3000. I have it configured now, and it already reported an error:

[2018-10-07 22:15:27.309] [WARN] csp - violation of [script-src] from [default-src *; media-src 'self'; font-src 'self' data:; img-src 'self' data:; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; object-src 'none'; plugin-types 'none'; manifest-src 'self'; frame-src 'none'; worker-src 'self'; upgrade-insecure-requests; report-uri /csp-reporting;]
  on https://peertube.social/video-channels/1a799cd3-7a4a-4b63-83e3-aad1029daa34/videos by eval line:1
  agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
  referrer: https://peertube.social/videos/watch/b757a40a-d1b3-4d8a-b6c8-abd03a3a60f2

[rigelk]

Ah right, Angular uses eval, so script-src must be set to 'unsafe-eval'. It kind of defeats the purposes of CSPs for scripts, but that's the best we can do for now.

[Nutomic]

Okay that seems to fix it, no more errors so far after clicking around a bit. I hope there's some way to get rid of the eval usage. Here's a relevant issue, apparently the eval is solved already in Angular:

https://github.com/angular/angular/issues/19142
https://github.com/angular/angular-cli/issues/3430
https://github.com/angular/angular-cli/issues/6872

[rigelk]

As said in the last issue you linked, using AOT and removing reflect-metadata allows to get rid of unsafe-eval. Maybe we could remove it in production builds, like suggested in https://github.com/angular/angular-cli/issues/6325 ? ping @Chocobozzz

[rigelk]

Meanwhile, I have tested live the CSP configuration I previously suggested and completed it to achieve the following:

  contentSecurityPolicy: {
    directives: {
      defaultSrc: ["'none'"], // by default, not specifying default-src = '*'
      connectSrc: ['*'],
      mediaSrc: ["'self'"],
      fontSrc: ["'self' data:"],
      imgSrc: ["'self' data:"],
      scriptSrc: ["'self' 'unsafe-inline' 'unsafe-eval'"],
      styleSrc: ["'self' 'unsafe-inline'"],
      objectSrc: ["'none'"],
      formAction: ["'self'"],
      frameAncestors: ["'*'"],
      baseUri: ["'none'"],
      pluginTypes: ["'none'"],
      manifestSrc: ["'self'"],
      frameSrc: ["'self'"], // instead of deprecated child-src / self because of test-embed
      workerSrc: ["'self'"], // instead of deprecated child-src
      upgradeInsecureRequests: true
    },
    browserSniff: false // assumes a modern browser, but allows CDN in front
  },

Hopefully we're close to a good first implementation :)

[Nutomic]

@rigelk Can you paste the raw header please?

[rigelk]

@Nutomic Content-Security-Policy: default-src 'none'; connect-src *; media-src 'self'; font-src 'self' data:; img-src 'self' data:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; object-src 'none'; form-action 'self'; frame-ancestors '*'; base-uri 'none'; plugin-types 'none'; manifest-src 'self'; frame-src 'self'; worker-src 'self'; upgrade-insecure-requests

[Nutomic]

• should be frame-ancestors * instead of frame-ancestors '*' (without quotes)
• should be base-uri 'self' instead of base-uri 'none'
• Chromium shows the error Invalid plugin type in 'plugin-types' Content Security Policy directive: ''none''. Did you mean to set the object-src directive to 'none'? Apparently this is because object-src should only be set if some plugins are allowed, according to this thread.

Edit: I guess we also need to set media-src *:

[2018-10-11 21:59:07.631] [WARN] csp - violation of [media-src] from [default-src 'none'; connect-src *; media-src 'self'; font-src 'self' data:; img-src 'self' data:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; object-src 'none'; form-action 'self'; frame-ancestors *; base-uri 'self'; ; manifest-src 'self'; frame-src 'self'; worker-src 'self'; upgrade-insecure-requests; report-uri /csp-reporting;]
  on https://peertube.social/videos/watch/7ae45cdb-a2fa-4544-8dfc-77a0b299b58b by blob
  agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.102 Safari/537.36 Vivaldi/2.0.1309.37

Edit2: Weird, it's still reporting the same error with media-src *, but apparently only in Apple browsers:

[2018-10-11 22:33:57.447] [WARN] csp - violation of [media-src] from [default-src 'none'; connect-src *; media-src *; font-src 'self' data:; img-src 'self' data:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; object-src 'none'; form-action 'self'; frame-ancestors *; base-uri 'self'; ; manifest-src 'self'; frame-src 'self'; worker-src 'self'; upgrade-insecure-requests; report-uri /csp-reporting;]
  on https://peertube.social/videos/watch/7ae45cdb-a2fa-4544-8dfc-77a0b299b58b by blob
  agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.102 Safari/537.36 Vivaldi/2.0.1309.37

And another violation for embeds:

[2018-10-11 22:27:53.615] [WARN] csp - violation of [img-src] from [default-src 'none'; connect-src *; media-src *; font-src 'self' data:; img-src 'self' data:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; object-src 'none'; form-action 'self'; frame-ancestors *; base-uri 'self'; ; manifest-src 'self'; frame-src 'self'; worker-src 'self'; upgrade-insecure-requests; report-uri /csp-reporting;]
  on https://peertube.social/videos/embed/ab8cc45f-8fcf-414e-9492-e5ebfd8e592d by android-webview-video-poster line:1
  agent: Mozilla/5.0 (Linux; Android 8.1.0; Nokia 7 plus Build/OPR1.170623.026; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/69.0.3497.100 Mobile Safari/537.36

[rigelk]

Great! Thanks for the feedback @Nutomic - the plugin-src was confusing as it doesn't yield any error in Chromium :/

Let me try to improve that tomorrow.

[rigelk]

With the fixes suggested by @Nutomic, I realised we're using frame-ancestors: frame-ancestors: * and it overrides X-Frame-Options: DENY on browsers that understand this CSP 2 rule.

Right now we're using X-Frame-Options: DENY on all routes except the embed route. We should replicate the behaviour with the CSP.

EDIT: it's a bit complicated, so I suggest we make a first version without frame-ancestors.

[rigelk]

Finally I managed to replicate it :slightly_smiling_face:

[rigelk]

I also managed to remove unsafe-inline as I said in https://github.com/Chocobozzz/PeerTube/issues/1190#issuecomment-427687718