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

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

Open
lukewarlow opened this issue May 28, 2024 · 15 comments
Open

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

lukewarlow opened this issue May 28, 2024 · 15 comments
Labels
from: Google Proposed, edited, or co-edited by Google. from: Igalia Proposed, edited, or co-edited by Igalia. topic: security venue: W3C Web Application Security WG Proposal is being reviewed in the W3C's Web Application Security WG (aka WebAppSec)

Comments

@lukewarlow
Copy link
Member

lukewarlow commented May 28, 2024

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

w3c/webappsec-csp@8444566

Issue Tracker URL

No response

Explainer URL

No response

TAG Design Review URL

No response

Mozilla standards-positions issue URL

mozilla/standards-positions#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
Copy link
Member Author

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

@annevk annevk added topic: security venue: W3C Web Application Security WG Proposal is being reviewed in the W3C's Web Application Security WG (aka WebAppSec) from: Google Proposed, edited, or co-edited by Google. from: Igalia Proposed, edited, or co-edited by Igalia. labels May 30, 2024
@annevk
Copy link
Contributor

annevk commented May 30, 2024

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 w3c/webappsec-csp#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 #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
Copy link

caridy commented Jun 3, 2024

@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
Copy link
Contributor

annevk commented Jun 4, 2024

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
Copy link

caridy commented Jun 5, 2024

@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
Copy link
Contributor

annevk commented Jun 5, 2024

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
Copy link

caridy commented Jun 5, 2024

@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
Copy link
Member Author

But strict-dynamic obsoletes unsafe-eval.

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

@annevk annevk mentioned this issue Jun 17, 2024
@lukewarlow lukewarlow changed the title New trusted-eval keyword for CSP script-src New trusted-types-eval keyword for CSP script-src Jun 18, 2024
@gregwhitworth
Copy link

@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
Copy link
Contributor

annevk commented Jul 5, 2024

The strict-dynamic concern was based on a misunderstanding, but the non-naming (thanks for renaming!) aspects of #355 (comment) (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
Copy link

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
Copy link

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?

@lukewarlow
Copy link
Member Author

This was discussed at TPAC and broad support was shown for the idea, is there anything the WebKit team needs to get a position on this?

@gregwhitworth
Copy link

@annevk friendly ping on this

@annevk
Copy link
Contributor

annevk commented Nov 12, 2024

I recommend we mark this as "position: positive" one week from now. I would like to thank everyone who contributed to this thread!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from: Google Proposed, edited, or co-edited by Google. from: Igalia Proposed, edited, or co-edited by Igalia. topic: security venue: W3C Web Application Security WG Proposal is being reviewed in the W3C's Web Application Security WG (aka WebAppSec)
Projects
None yet
Development

No branches or pull requests

5 participants