946512 - Implement Content Security Policy for Make Valet

Status: RESOLVED FIXED

(Webmaker Graveyard :: Make Valet, defect)

Description

[Jon Buckley [:jbuck]]

Time to add some CSP to the make-valet.

Comment 1

[Jon Buckley [:jbuck]]

Attachment: https://github.com/mozilla/make-valet/pull/37

:gvn, :k88hudson - Please review this patch throughly! This is the first substantial CSP patch, so I want to find as many dragons as possible now. :gvn, feel free to fail this patch on performance. We can always split this up into smaller bugs to minimize the performance impact.

:mgoodwin - In this patch, I'm using "style-src 'unsafe-inline';". What are the security implications of CSS injection? I can refactor that inline style block to be a separate file, but it does mean extra http overhead, especially for such a small file.

Comment 2

[Gavin Suntop [:gvn]]

Comment on attachment 8342732 [details] [review]
https://github.com/mozilla/make-valet/pull/37

I have a few questions on Github. Specifically I'm concerned about media/img/object policy. Aren't there going to be videos coming from YouTube and imgs coming from random domains?

Comment 3

[Jon Buckley [:jbuck]]

Comment on attachment 8342732 [details] [review]
https://github.com/mozilla/make-valet/pull/37

I didn't change any code, just made a note about img-src * in the PR. It shouldn't be necessary for this patch

Comment 4

[Gavin Suntop [:gvn]]

Comment on attachment 8342732 [details] [review]
https://github.com/mozilla/make-valet/pull/37

R+, but if possible lets run it in report only mode for a bit.

Comment 5

[inactivate account for kate]

Comment on attachment 8342732 [details] [review]
https://github.com/mozilla/make-valet/pull/37

This works fine for me, but those error messages are pretty ugly.

Since a -lot- of people look at the make details pages, personally, I'd rather avoid the reporting stuff until we set up a reporting url... Seems like the regular CSP header would actually be OK here since there all the content we are showing (i.e. the embed shell) is text-based or static (no user-generated assets).

If we don't really care about error messages, this is a plus.

Comment 7

[Jon Buckley [:jbuck]]

:gvn - while we land the csp report, how about landing this patch? We can add the report-uri directive when the server is up and running.

Comment 8

[Jon Buckley [:jbuck]]

Edit - to be clear, landing it in enforcement mode

Comment 9

[Gavin Suntop [:gvn]]

Yeah, I think this should be fine. I'm getting close to having the CSP logger written...

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/make-valet

https://github.com/mozilla/make-valet/commit/3afa949101e8ca737ed9cafdd30ec52a32d52de2
Fix bug 946512 - Implement Content Security Policy

Comment 11

[[github robot]]

Commit pushed to master at https://github.com/mozilla/make-valet

https://github.com/mozilla/make-valet/commit/a383215f897812568357c77dfa389b83b00243e4
Bug 946512 - Fix img-src http://www.google-analytics.com CSP violation

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/make-valet

https://github.com/mozilla/make-valet/commit/26e03a90ac9688e2bfaffdadee320ed8eb3da8c6
Fix bug 946512 - Fix img-src https://ssl.google-analytics.com CSP violation

Comment 13

[Mark Goodwin [:mgoodwin]]

(In reply to Jon Buckley [:jbuck] from comment #1)
> :mgoodwin - In this patch, I'm using "style-src 'unsafe-inline';". What are
> the security implications of CSS injection? I can refactor that inline style
> block to be a separate file, but it does mean extra http overhead,
> especially for such a small file.

Short answer, unless you're also allowing inline script, they're minimal on modern browsers.

Longer answer, take a look at the browser security handbook content on this topic: http://code.google.com/p/browsersec/wiki/Part1#Cascading_stylesheets - note that some of the issues are specific to older browsers, and some of the issues (as I mentioned above) are mitigated by CSP (unless you allow inline scripts)

Comment 14

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8342732 [details] [review]
https://github.com/mozilla/make-valet/pull/37

I'm OK with this if you're happy with the risks as per Comment 13

Comment 15

[Jon Buckley [:jbuck]]

We're definitely not allowing inline script, and we're only supporting IE9+, so I think we're good to go here. Thanks Mark!