◀ Merged tasks: T4373.

It may also be worth implementing Strict-Transport-Security, although this one feels flimsier and more likely to bite users to me.

This isn't launch blocking, but is something we should have for the SAAS version.

From PHI399:

Some of the challenges I'm aware of:

• We currently use a <script /> tag a the bottom of the body to start the world, and this won't survive CSP. This tag essentially just contains a call to start(<large JSON configuration blob>) so it should conceptually be easy to move to a data property in the DOM, but the behavior of start(...) was, at least at one point, very "clever" and order-sensitive in an effort to build a working environment as quickly as possible in a cross-browser way. This may mean that swapping this out isn't entirely straightforward. I think we can also nonce/sign these and keep them, but I'd rather get rid of them entirely if that doesn't prove to be Very Hard for some reason.
• We don't have a convenient piece of existing infrastructure to repurpose for Report-Only to generate a log. We can probably just skip this, but may need to come up with something if this proves more complicated than I think.
• There's probably some weird stuff I'm forgetting about, although nothing springs to mind. We explicitly don't support third-party images. We do support embedding YouTube videos, so they may need a frame-src exception if the related option is configured. We may load some resources from 'self' even when an alternate file domain is specified?
• This may be a good opportunity to specify Referrer-Policy too.
• STS was enabled on secure but never actually enabled in Phacility production, but should probably be turned on when the rest of this rolls out.

We use inline style="..." attributes and can't realistically get rid of these in the short term. Although some of these are just laziness in obscure interfaces, quite a few are legitimate (for example, inline background-image for profile images, and inline computed width or position values for elements which can be moved or resized in Javascript). We could move more rendering to Javascript, but Phabricator "mostly sort of" works with Javascript disabled and I generally think this is desirable, so I think this would be a tradeoff, not a strict improvement.

The attacks I'm aware of with CSS (with some exceptions) are generally about deceiving the user (restyle elements on the page to look like something else and try to convince the user to click a dangerous button) and I'm hard pressed to come up with any really plausible vector here, I'm not terribly concerned about this. I believe we never give users anything which resembles direct access to a style attribute. The other CSP directives should (presumably?) make CSS attacks which rely on exfiltrating data by loading background images more difficult.

(Offhand, it's actually not immediately clear to me whether unsafe-eval applies to style attributes or not. Presumably, it should. This is easy to test.)

We also use an inline <style /> tag to implement the "Monspaced Font" user preference by defining a .PhabricatorMonospaced CSS rule. This is significantly more dangerous than style attributes and also not trivial to get rid of. We have historically been mindful about the risk here (e.g., see D7274). I can't immediately come up with an easy, clean way to get rid of this. For now, I'm going to leave it alone.

We use data: URIs to inline small images into CSS. For now, I'm going to allow these unconditionally. It's possible we could be more strict in the future, and allow data: as an img-src only in CSS files.

We use a <script> tag in the header to perform framebusting. I'm inclined to update this to rely more heavily on CSP frame-ancestors instead. We could also leave it inline and expect that browsers which understand CSP (everything but IE) will be protected by frame-ancestors, while browsers that do not understand CSP will be protected by explicit framebusting, but then we'll have log warnings and Report-Only will be useless so I don't think this is a great approach.

(Since we also have X-Frame-Options, maybe we can get rid of the framebusting snippet entirely? This would leave us unprotected from clickjacking attacks on IE10 and older.)

(Offhand, it's actually not immediately clear to me whether unsafe-eval applies to style attributes or not. Presumably, it should. This is easy to test.)

style-src 'unsafe-inline' seems to apply to both <style> and style="...". Since we can't realistically get rid of style="..." any time soon, we need to allow this, so I'm not going to worry too much about the one <style> for monospaced preferences.

Idle thought: can we data: an SVG with onhover behaviors?

Initializing __DEV__ early enough is proving tricky. I'm tentatively moving it to <html data-developer-mode="1">.

Idle thought: can we data: an SVG with onhover behaviors?

Yes, but all of Safari, Firefox and Chrome disable Javascript for SVGs embedded with <img src="..." />, including when src is src="data:...", so I don't think this exposes new attack surface, at least today.

I don't think this exposes new attack surface, at least today.

Actually, it looks like it was trivial to restrict to CSS anyway.

On the Stripe payment processing workflow, we embed a piece of Javascript directly from Stripe.

On the Recaptcha flow, we embed a piece of Javascript directly from Google.

These need to be present in the CSP.

Actually, it looks like it was trivial to restrict to CSS anyway.

It looks like the CSP for the main request, not for the CSS file itself, is what matters, so we can't set img-src to one thing on the page and to a more permissive value, including data: URIs, in CSS only. So we need to allow data: URIs everywhere because we use them in CSS (and there's a significant performance benefit to doing so).

When we use a Quicksand transition from page A (which does not have Google or Recaptcha stuff on it) to page B (which does), the CSP from Page A will currently still be in control and prevent Page B from working.

We either need to CSP the trusted sources on all pages (yuck) or decline to perform Quicksand navigations to Stripe/Recaptcha pages (probably better).

Other stuff to test:

• Phurl short domains.
• Phame external domains.

These should probably both be OK by default.

On the Stripe payment processing workflow, we embed a piece of Javascript directly from Stripe.
On the Recaptcha flow, we embed a piece of Javascript directly from Google.

These both pull in hundreds of other javascript files and iframes, as is the way of Javascript development, but I think I eventually whitelisted everything.

When we use a Quicksand transition...

I just blacklisted /auth/ and /phortune/ from Quicksand for now since this is no great loss.

We could use nonce-... but the nonce would have to be the same across the duration of a Quicksand session. This is better than nothing, but not as good as true per-request nonces, and any nonce is less good than no nonce.

We could proxy the JS from Google and Stripe, but that will only work for the initial request, and both workflows create additional requests, frames, etc.

We could proxy the JS from Google and Stripe, evaluate it, replace all the references to Google/Stripe with references to proxies, etc. This only requires us to, e.g., write a JS execution engine in PHP. It's possible that str_replace('google.com', 'local.com/proxy/') is a good enough evaluation engine for this problem, but I'm not enamored with this approach.

I think just blacklisting these URIs, or doing something a little more sophisticated (letting the Quicksand response say "even though this request wasn't blacklisted by URI, while preparing a response it became clear that Quicksand must disengage; shut down Quicksand and repeat the request as a normal navigation event") is the right way forward in the long term.