WebKit / standards-positions

WebKit's positions on emerging web specifications
https://webkit.org/standards-positions/
242 stars 19 forks source link

New `trusted-types-eval` keyword for CSP script-src #355

Open lukewarlow opened 4 months ago

lukewarlow commented 4 months ago

WebKittens

@annevk

Title of the spec

Content Security Policy

URL to the spec

https://w3c.github.io/webappsec-csp/

URL to the spec's repository

https://github.com/w3c/webappsec-csp/commit/844456655493197e60e7657cc11f8dfcb505372f

Issue Tracker URL

No response

Explainer URL

No response

TAG Design Review URL

No response

Mozilla standards-positions issue URL

https://github.com/mozilla/standards-positions/issues/1032

WebKit Bugzilla URL

No response

Radar URL

No response

Description

This proposes a new keyword for the CSP script-src directive (preliminarily trusted-eval but can be bikesheded). The main use case for this new keyword is to allow enabling eval only in browsers that support and have Trusted Types enforced. Currently trusted types is used alongside unsafe-eval (when you need eval), which means that in browsers with no trusted types support eval is still allowed (completely unmitigated by the protections TT offer). This new keyword would prevent that situation.

Edit: Based on feedback the name is trusted-types-eval

lukewarlow commented 4 months ago

See #186 for the base position request on Trusted Types as a whole.

annevk commented 4 months ago

I think my main issue with this is that it's only as trusted as the policy, which might well be crap, but that's a general problem with Trusted Types that's no longer fixable. Although given the other directives have the trusted-types-* prefix (based on https://github.com/w3c/webappsec-csp/pull/659) this should probably have that too, to make it clear it's not more special than any of that.

The other discomfort I have is that in https://github.com/WebKit/standards-positions/issues/186 the main motivation for this feature that was brought forward was a way to avoid having to specify unsafe-eval as certain regulation would prohibit that. Which raises the question how hard either of these policies make it to execute script despite the policy being in place. E.g., you have a CSP header in place for regulatory reasons, but you don't actually care much for the potential upsides as you have a deadline to meet and just want to execute some script. I suspect that in both cases you can recreate "unsafe eval", but it's probably a little bit more straightforward with Trusted Types if your default policy blesses any string you pass it.

Modulo a more Trusted Types-aligned name it's probably okay, though I'd be curious to see more analysis of this from other people.

cc @sysrqb @johnwilander

caridy commented 4 months ago

@annevk with respect to the discussion about regulations, I have few comments, but first, few general thoughts on regulations:

  1. By definition, regulators have to evolve their rules as technology evolves, and companies that are regulated are responsible for pushing back on outdated regulations. But this is a two way street.
  2. I haven't seen a regulation that applies only to a particular browser, or a particular version of a browser, they are often vague with respect to the browsers support matrix. This is often in directly-conflict with the way we (standard folks) see the web evolution, we often focus more on backward compatibility and the "do not break the web" mantra.
  3. There is often a fine-line between complying with "the spirit of the regulation" vs "the letter of the regulation". I think we (standard folks) should focus more on the former.

With respect to this particular proposal:

  1. The specific manifestation of the conflict is that browsers without TT are completely open to the attack surface that the regulation is attempting to regulate. The intention of this proposal is not to subvert the regulation, but to comply with the spirit of it. Which is that the app should only work in browsers that can comply with the rules defined to protect eID integrations. You have two ways to achieve that: A) remove unsafe-eval, unsafe-inline and co. entirely (and we know how that goes on complex apps), or B) use controlled evaluations. Unfortunately, option B is not possible today without sniffing the UA when serving the HTML (and we know how that goes as well).
  2. A prior art to consider is strict-dynamic, which does not require unsafe-inline to be present (and if it present, it will be ignored), as a mechanism to prevent arbitrary injections via HTML parsing mechanism while still allowing controlled injection of scripts via javascript.
  3. With respect to your concern about misusing the "default" policy, even if that's the case, fortunately we have layering there, and it is not only that "default" to be enabled (via the CSP header), but you must also have "allow-duplicates" or create a protocol to share the default policy object with arbitrary code that is "allowed" to run in the page. I don't think that's what the regulation is attempting to prevent, IMO they are focused on XSS and any arbitrary injection from malicious actors while the application's code is considered trusted. We cannot see the removal of unsafe-eval in isolation as part of the eID regulation, it is a lot more than that.

I hope that this helps to clarify the relationship between this proposal and the eID regulations. We are looking forward to see @sysrqb and @johnwilander feedback on this issue.

annevk commented 4 months ago

In some discussion today at Web Engines Hackfest strict-dynamic came up as well. I believe strict-dynamic is the recommended way of using CSP these days so given that I wonder why we would add a feature that is redundant with it.

caridy commented 3 months ago

@annevk it is my understanding that they are complementary to each other. strict-dynamic by itself is not enough since it only protects from parsing-related injections. Plus, you're still at risk from Non-Script-Based XSS, DOM Clobbering, etc. TT can complement it by adding additional parsing (sanitizer), plus it can add additional restrictions to control who can evaluate code via script insertion or eval. I can only speak about our codebase at salesforce, which is significant, and all I can say is strict-dynamic is not enough for us at Salesforce, we need/want more fine controls.

annevk commented 3 months ago

I'm not disputing you don't want strict-dynamic and Trusted Types at the same time. But strict-dynamic obsoletes unsafe-eval. Thus I'm disputing the need for a variant of that.

caridy commented 3 months ago

@annevk I might be missing something important here. I don't understand how strict-dynamic obsoletes unsafe-eval. It has no implications on eval() or Function() constructor programatic invocations as far as I can tell, it only cares about parsed evaluations.

In theory, if you have a path forward to only evaluate trusted code (via strict-dynamic), then what's the risk? In practice, that's a lot harder to achieve, because trusted code might need to do funky stuff, specially for large systems with a lot of moving parts and legacy pieces (enterprise software is full of that), and the only way to use that is by eval some string, what would you do in this case?

One of the premises of TT is that it helps to pave the way to a better security posture by slowly closing more and more avenues. At least that's my understanding from the original discussions with Mike Samuels many years ago at the SES Meetings.

lukewarlow commented 3 months ago

But strict-dynamic obsoletes unsafe-eval.

This is a misunderstanding of what strict-dynamic allows. Today CSP requires 'unsafe-eval' for use of eval.

gregwhitworth commented 3 months ago

@annevk circling back on this since @caridy is currently on PTO. Did the discussions with @lukewarlow around the potential misunderstanding assuage your concerns with the need for introduction of a new CSP header for trusted type support?

annevk commented 2 months ago

The strict-dynamic concern was based on a misunderstanding, but the non-naming (thanks for renaming!) aspects of https://github.com/WebKit/standards-positions/issues/355#issuecomment-2139661389 (my initial comment) still stand. It's not a strong concern as there's ways to cheat with the existing policies as well, though perhaps it would take some more effort. Mainly I'm interested in seeing opinions from other security and CSP experts.

lweichselbaum commented 1 month ago

I'm not particularly worried about the 'cheating' aspect, as developers can simply choose not to use this new keyword if that's the concern. Moreover, CSP is generally not well suited to protecting against malicious developers or against attackers that already have the ability to execute arbitrary code via XSS or similar.

It's also worth noting that there are specific scenarios where eval() is unavoidable. In such cases, even a single eval() call can necessitate adding unsafe-eval to your otherwise strict CSP. This is unlike strict-dynamic-based CSP, where there's usually refactoring alternatives to leverage safer APIs to achieve the same functionality.

At Google, we encountered this challenge during large-scale CSP deployments. While we successfully deployed strict-dynamic-based CSP to over 800 web applications, eliminating unsafe-eval proved difficult in many cases. We eventually adopted Trusted Types to block eval or allowed it selectively via Trusted Types in a few security-reviewed contexts.

Overall, I support the new keyword because I believe its benefits outweigh the potential drawbacks, especially considering its opt-in nature.

gregwhitworth commented 3 weeks ago

Thank you for the feedback @lweichselbaum. @annevk are there any other folks you would like to loop in here to weigh in on this issue?