w3c / trusted-types

A browser API to prevent DOM-Based Cross Site Scripting in modern web applications.
https://w3c.github.io/trusted-types/dist/spec/
Other
606 stars 74 forks source link

Can we drop the default policy value changing from Eval, new Function() (and other usages of the dynamic code brand checks proposal)? #461

Closed lukewarlow closed 5 months ago

lukewarlow commented 8 months ago

Following on from discussions recently with @caridy it's possible we could avoid the default policy fallback for eval (and Function() etc).

This simplifies the TT spec slightly (not much though), it also simplifies implementation, but the key thing is it removes a potentially contentious point from the Ecmascript changes.

This would be a breaking change from Chromiums behaviour but I'm doubtful it has much usage? Especially based on discussions in #458

cc @koto @otherdaniel

lukewarlow commented 8 months ago

The exact specifics of what this could look like are up for discussion, I can see us calling the default policy and just make sure the return value is the same as what went in? I can see us not calling default policy at all and just blocking all raw strings and requiring trusted types.

The main bit that could be good to remove is the abiity to return a different value from the default policy and having that execute.

koto commented 8 months ago

@otherdaniel can we measure when the default policy for TrustedScripts changes the value?

If not used, I think we could write is such that a value that is different from the input (for this particular case only) would be like if false was returned, and in Chrome have a deprecation mechanism for the current behaviour - that doesn't change the function signature and allows most assumed usages to stay as-is.

lukewarlow commented 8 months ago

Yeah so a use counter for when the policy is invoked for eval or function and the return value is a string and that string is different from the string you enter.

mbrodesser-Igalia commented 8 months ago

Following on from discussions recently with @caridy it's possible we could avoid the default policy fallback for eval (and Function() etc).

Is the intention to remove calling the default policy for eval and some other injection sinks, but not all other injection sinks? From a web-dev's perspective that seems inconsistent.

lukewarlow commented 8 months ago

Is the intention to remove calling the default policy for eval and some other injection sinks, but not all other injection sinks? From a web-dev's perspective that seems inconsistent.

Not to remove but to remove its ability to change the value. And yes specifically only for eval etc.

While I agree it would be inconsistent, TT is inconsistent with eval in general already, objects currently aren't evaluated in a meaningful way. So I don't think it's super unreasonable?

mbrodesser-Igalia commented 8 months ago

Is the intention to remove calling the default policy for eval and some other injection sinks, but not all other injection sinks? From a web-dev's perspective that seems inconsistent.

Not to remove but to remove its ability to change the value. And yes specifically only for eval etc.

While I agree it would be inconsistent, TT is inconsistent with eval in general already, objects currently aren't evaluated in a meaningful way. So I don't think it's super unreasonable?

Yes, if there already is unavoidable inconsistency for eval, it seems reasonable.

Btw. it would be clearer if this issue explicitly listed eval's "company".

caridy commented 8 months ago

I'm very supportive of this additional restriction for eval and Function. cc @nicolo-ribaudo

lukewarlow commented 8 months ago

@otherdaniel when you're able to add those use counters could you leave a comment here just to keep the context all in one place.

lukewarlow commented 8 months ago

I think our best course of action is to assume we can make this change, and update the tc39 spec and this one accordingly.

The tc39 change is more likely to get merged without this change based on all discussions so far. Worst case if we need this ability for a compat reason we can always reapproach tc39 for the necessary discussions or just bypass them for the specific change necessary and handle it in TT land.

lukewarlow commented 8 months ago

https://github.com/w3c/trusted-types/pull/465 is the relevant change to this spec and the tc39 proposal change is linked in it (already approved by nico)

lukewarlow commented 8 months ago

@koto @otherdaniel based on the discussions above would you be okay with https://github.com/tc39/proposal-dynamic-code-brand-checks/pull/12 being merged and us progressing the tc39 proposal on the assumption the use counters come back okay?

If down the line the use counters reveal that it would be a compat risk we can revisit what to do (go back to tc39 with more changes, monkeypatch from this spec, etc). But I think it makes sense to try and progress the tc39 proposal sooner rather than later?

koto commented 8 months ago

I think it makes sense.

otherdaniel commented 8 months ago

Yes, looks good!

lukewarlow commented 8 months ago

@otherdaniel were you able to add the use counter for this yet?

lukewarlow commented 5 months ago

This has been done inside of the WebKit implementation and the PRs upstreaming it have this baked in.

If we wanted to revisit this it would require tc39 changes aswell as CSP changes.

lukewarlow commented 5 months ago

Based on discussions at the web engines hackfest, @nicolo-ribaudo it might be useful if you could find the historical push back against being able to change the value. I know this time around there might not have been an explicit discussion about it when presenting but my understanding is that this was based on historical push back when the proposal was previously presented?

If you can't find the historical reasoning no worries it just might be useful to have them collected somewhere.