Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: Use different switch/flag than CSP #1

Closed
mozfreddyb opened this issue Sep 18, 2017 · 22 comments
Closed

Suggestion: Use different switch/flag than CSP #1

mozfreddyb opened this issue Sep 18, 2017 · 22 comments
Labels

Comments

@mozfreddyb
Copy link
Collaborator

I know this will cause a bikeshed, but I think the cmplexity in CSP is best avoided for a thing that in itself causes quite dramatic changes in how the DOM works.

@mikewest
Copy link
Member

I know that @slekies agrees.

I think I'd be a little sad if this was a standalone header. We have too many of those already. I could imagine it tying into feature policy, though it's not really the same model. @clelland/@raymeskhoury might have opinions there, as this is somewhat similar to the document.write restrictions that we (have already? want to?) added there.

I'm not sure it's a great fit, given the way FP wants to cascade out of the specific resource, but, whatever. :)

@mikewest
Copy link
Member

mikewest commented Sep 18, 2017

(That said, this would be a policy governing the security of a page's content. So, you know. :) )

@koto
Copy link
Member

koto commented Sep 18, 2017 via email

@mikewest
Copy link
Member

@koto: +1. Obviously, the way we frame the opt-out is not central to the project. It's the way I've spelled it in the prototype, and I think the framing makes sense. But we have tons of time to argue about it. Nothing is set in stone, and this is not the most important problem on our plate. :)

@mozfreddyb
Copy link
Collaborator Author

I've thought about this for some more and if it's only me speaking up loud enough, you can close this issue.

If people care about DOM XSS mitigations, they should probably also think about mitigating other cases of XSS. So, CSP seems like a good fit.

@mozfreddyb
Copy link
Collaborator Author

So to expand, there are XSS problems that come from JavaScript, but are certainly not a good fit for this API. For example location.href = 'javascript:alert(1)', so it'd be bad to include this here, especially when it's already included in CSP.

@koto
Copy link
Member

koto commented Sep 20, 2017

The example you've given is in scope for this API. location.href is definitely a sink we'd want to require trusted type on. Essentially, all string-accepting DOM XSS sinks would be in scope, though we might back away from sinks noone practically uses with user data.

Similarly, some sinks we couldn't protect in a polyfill (like location.href), but that's a different limitation.

@mikewest
Copy link
Member

I'll leave this open, because I know @koto and @slekies would be happier if it wasn't mentioned in the same breath as CSP. We can argue about that when we have the fundamentals in place. :)

@mozfreddyb: location.href is called out explicitly in the explainer. See the "URL contexts" section of https://github.com/mikewest/trusted-types/. :)

@mozfreddyb
Copy link
Collaborator Author

Well, there's obvious overlap with CSP that needs a discussion, it seems :) It'd be odd for something to disabled twice when you set a reasonable CSP that includes this new DOM API, no?

@koto
Copy link
Member

koto commented Sep 20, 2017

Yes, there is an overlap, I see your point. But, in principle, trusted types enforcement should work even in presence of unsafe-inline, as they aim to achieve different things. Types enable the developers to spell out their interactions with DOM such that accidental DOM XSS does not happen, CSP tries to mitigate JavaScript injection flaws after they already happened.

There's a clear advantage to stuffing their enforcement into CSP (we don't create yet another security header, there's tooling around CSP etc.), but one downside is that we're tied to a mechanism with a clumsy backwards-compatibility story, complex semantics, and a huge misconception of what it provides.

Still - we'll have plenty of time to bikeshed this after the implementation.

@mikewest
Copy link
Member

@koto: I have opinions about your assertions! But, you know, later. :)

@koto
Copy link
Member

koto commented Jan 17, 2019

To document what's currently happening wrt headers:

The spec specifies a Trusted-Types header with slightly different semantics. The current implementation, both in Chrome and polyfill use a CSP header (this was simply faster to implement).

Some authors actually prefer to keep it in CSP for now (mostly because of existing violation report servers that accept a certain syntax; turns out even in Google not everything supports report-to :/ ).

@mikesamuel
Copy link
Collaborator

@koto, I'm happy to adjust the spec to match reality if that's where you want this to go.

@koto
Copy link
Member

koto commented Apr 5, 2019

Before deciding on the final header, we have to solve #66 and the versioning - cc @otherdaniel (we should file a bug to discuss a versioning scheme).

@briansmith
Copy link

I think it would be very confusing to put this in Content-Security-Policy instead of its own header field. Right now everybody reading this is familiar with CSP and so it doesn't seem like a big deal to us to conflate the two things. But a future developer might not learn CSP before they learn Trusted-Types. In fact, if this approach is successful, then in the more distant future they might not even bother learning CSP at all. Then they shouldn't have to learn anything about the CSP syntax.

Further, Trusted-Types is simply not CSP; the two things are completely independent and complementary, AFAICT. A separate header field makes this more clear. In particular, when learning Trusted-Types the first question I had is "how does this affect the rest of the CSP policy's enforcement?" AFAICT, it doesn't affect the rest of the CSP policy at all; however, try discovering this by reading the CSP and Trusted-Types specs.

Finally, see https://www.mnot.net/blog/2018/11/27/header_compression. We shouldn't have multi-party header fields in general.

(Note that the current Trusted-Types syntax also has different semantics than CSP: Trusted-Types: a, b means "a UNION b" if I read the draft correctly, but Content-Security-Policy: a, b means "a INTERSECTION b". I think that using the JSON header format for Trusted-Types may be a mistake because any header injection would allow for the expansion of the Trusted-Types policy, instead of just a restriction as in CSP. Thus, I don't think the spec'd form of the header is the right design either, other than being a separate header field.)

@mikesamuel
Copy link
Collaborator

@briansmith Re "the two things are completely independent and complementary, AFAICT", what's your take on #143 ?

@briansmith
Copy link

@briansmith Re "the two things are completely independent and complementary, AFAICT", what's your take on #143 ?

Maybe I'm overlooking something, I don't think it makes sense for CSP to block eval(x) when x is not a string since in that case eval(x) is equivalent to x, regardless of Trusted-Types.

@koto
Copy link
Member

koto commented May 3, 2019

I think it would be very confusing to put this in Content-Security-Policy instead of its own header field. Right now everybody reading this is familiar with CSP and so it doesn't seem like a big deal to us to conflate the two things. But a future developer might not learn CSP before they learn Trusted-Types. In fact, if this approach is successful, then in the more distant future they might not even bother learning CSP at all. Then they shouldn't have to learn anything about the CSP syntax.

Further, Trusted-Types is simply not CSP; the two things are completely independent and complementary, AFAICT. A separate header field makes this more clear.

That is true. They work in different ways. To give some context, a switch to CSP was done for a few very tangible reasons:

  • Report-only mode and Reporting API integration "for free"
  • Integration with CSP report collection servers "for free"
  • Well established parser with syntax that was rich enough

It's not set in stone yet, especially as the syntax might change e.g. due to #66.

In particular, when learning Trusted-Types the first question I had is "how does this affect the rest of the CSP policy's enforcement?" AFAICT, it doesn't affect the rest of the CSP policy at all;

That actually seems to be the behaviour of other CSP directives (apart from default). E.g. font-src doesn't affect connect-src.

(Note that the current Trusted-Types syntax also has different semantics than CSP: Trusted-Types: a, b means "a UNION b" if I read the draft correctly, but Content-Security-Policy: a, b means "a INTERSECTION b".

Yes, in the CSP header variant of TT we're not using the JSON notation (policies are just space-separated).

I think that using the JSON header format for Trusted-Types may be a mistake because any header injection would allow for the expansion of the Trusted-Types policy, instead of just a restriction as in CSP. Thus, I don't think the spec'd form of the header is the right design either, other than being a separate header field.

That part is interesting - in general we have two options if we opt to use a separate header. Reusing JSON/JFV (which seems trending in spec world), or writing our own parser. To avoid complexity, I'd lean towards JFV unless there's already a specced parser that works for our purpose. We usually assume that header injection is game over anyway for client side security controls (e.g. double newline injection reveals cookies, set-cookie injection does session fixation).

@mikewest
Copy link
Member

mikewest commented May 3, 2019

I said a million years ago that "I think I'd be a little sad if this was a standalone header." I take that back, if a separate header is the right approach, go for it. Headers for everyone!

CSP is convenient. It made the implementation simpler, and doesn't (to me) seem any worse than the existing directives outside of CSP's core (e.g. Upgrade-Insecure-Requests). But I can understand the desire to move TT further away from CSP for conceptual reasons.

I don't really have a strong opinion about how we spell the policy delivery mechanism

That part is interesting - in general we have two options if we opt to use a separate header. Reusing JSON/JFV (which seems trending in spec world), or writing our own parser.

If we mint a new header, let's base the syntax on https://tools.ietf.org/html/draft-ietf-httpbis-header-structure rather than inventing something new.

@briansmith
Copy link

We usually assume that header injection is game over anyway for client side security controls (e.g. double newline injection reveals cookies, set-cookie injection does session fixation).

Consider:

Trusted-Types: a

<!DOCTYPE html>
<meta http-equiv="Trusted-Types" content="b">

This is equivalent to:

Trusted-Type: a, b

In CSP, we didn't want <meta http-equiv> to be able to weaken the policy in the HTTP header field, and nor did we want multiple layers of HTTP middleware to weaken each others' policies. For example, if you add middleware that adds CSP nonce support, and then somebody else adds another middleware that adds CSP whitelisting support, you don't want the second change to weaken the first change. This is why CSP only strengthens the policy when it sees ',' and I think the same reasoning applies here and to other security controls.

@koto
Copy link
Member

koto commented Jun 1, 2019

That's fair. In principle, you can only go so far with CSP, and it's still not safe under "HTTP header injection" threat model, as the injection can just disable the subsequent CSP with injected: header value\r\ndisable-following-header- payload, but I see the point with regards to middleware.

We need to store at least 3 different kinds of data - policy names, report URI / group name, and type names (to support #66). However, in Structured Headers there's no data structure that would allow for two levels of data and at the same time would be safe in that model under header concatenation rules. For each of them comma is the outer separator.

With these limitations in place, we either accept the risk and move to a separate headers with Structured Headers syntax, move to a separate header with JSON values and choose a structure that's safe under header concatenation, or stick to CSP with its own parser.

I'd say we stick with CSP and extend the current trusted-types syntax to allow for the extra data (via e.g. keywords, or adding directives). So, for example:

CSP: trusted-types: policyA B andC default 'html' 'script-url' 'script' ; report-to internet-police

@koto
Copy link
Member

koto commented Jul 17, 2019

We finally decided to go with CSP (via #191). There's a couple of reasons for that:

  • reporting is built in (and it's tricky to integrate with Reporting API until that API has its issues reported by other browser vendors resolved)
  • syntax is rich enough
  • existing integrations allow us to facilitate adoption by authors
  • TT do introduce restrictions over content, so it feels like matching the CSP mandate
  • propagation for local scheme rules defined, and it matches what we'd like to achieve

@koto koto closed this as completed Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants