trusted-types icon indicating copy to clipboard operation
trusted-types copied to clipboard

Finalize the integrations that guard eval & Function.constructor

Open koto opened this issue 6 years ago • 26 comments
trafficstars

Essentially, we'd like:

  1. eval(TrustedScript), new Function(TrustedScript), and new Function(TrustedScript, TrustedScript) to work
  2. Their string equivalents to go through the default policy createScript function (a.k.a. if TT are enforced, to generate violation and not execute code by default)
  3. The default policy to be able to change the values to be executed.

The language primitives tracked in Dynamic Code Branch Checks TC39 proposal.

There's additional CSP integration required, tracked #143. Since it relaxes the CSP conditions, we might require a new keyword. We propose script-src 'trusted-script'

koto avatar Aug 11 '19 04:08 koto

The Blink/V8 implementation already has the behavior above. The ECMAScript bits are progressing through TC39.

koto avatar Mar 06 '20 11:03 koto

Note: change the spec text once https://github.com/tc39/proposal-dynamic-code-brand-checks stabilizes. It will likely require adding a slot to TrustedScript and adjusting for the new host callout signature - in CSP ( https://w3c.github.io/webappsec-trusted-types/dist/spec/#csp-eval) and HTML.

koto avatar Nov 25 '20 13:11 koto

There's additional CSP integration required, tracked https://github.com/w3c/trusted-types/issues/143. Since it relaxes the CSP conditions, we might require a new keyword. We propose script-src 'trusted-script'

@koto can you link to the issue tracking the addition of the new keyword script-src 'trusted-script'? We will like to see that done sooner rather than later considering the imminent enforcing of the regulations in Europe.

caridy avatar Dec 05 '23 15:12 caridy

The CSP integration would need dynamic-code-brand-checks proposal in ES - that got blocked from advancing to Stage 2 in TC39 (relevant discussion), so it's unlikely this would be specified in CSP soon.

koto avatar Dec 05 '23 16:12 koto

@koto we got consensus around https://github.com/tc39/ecma262/pull/3222 last week, which I think is related. Now the host hook has access to the value. And we are still pending to figure what to do with https://github.com/tc39/ecma262/pull/1498#issuecomment-1831716341, do you see a path forward there to get this issue folded into it?

caridy avatar Dec 05 '23 18:12 caridy

Based on https://github.com/tc39/ecma262/pull/3222#issuecomment-1832845892, the intention changed during the plenary and the host only has access to the stringified value. The meeting notes are not public yet, but I guess the crucial point would be to understand what concerns caused to reintroduce stringifying, because it seemed like the original intention of https://github.com/tc39/ecma262/pull/3222 was to supersede https://github.com/tc39/ecma262/pull/1498.

koto avatar Dec 06 '23 08:12 koto

@koto The motivation for that ECMA-262 PR is to allow the unsafe-hashes CSP to also be used for eval and new Function, other than just for executing code in inline script tags and inline event handlers. It ended up being a change independent from trusted types support.

nicolo-ribaudo avatar Dec 06 '23 19:12 nicolo-ribaudo

Thanks for context. Do you know what caused the change during the plenary? What were there concerns to give host the unstringified value?

koto avatar Dec 07 '23 11:12 koto

Yes. The plenary change was motivated by the desire to expose the string to the host so that CSP's unsafe-hashes policy could be extended to work with eval/new Function. Passing the object as-is doesn't work for this case, because the host would need to stringify the object (which would thus be stringified twice).

My idea is that the hook can still be passed the objects (and it should, for TT, there is no other way), but it can simply receive both the objects and the strings.

nicolo-ribaudo avatar Dec 13 '23 19:12 nicolo-ribaudo

Given the Dynamic Code Branch Checks proposal seems to be stuck at stage 1 it would be good to work out the alternatives.

lukewarlow avatar Jan 16 '24 11:01 lukewarlow

  1. eval(TrustedScript), new Function(TrustedScript), and new Function(TrustedScript, TrustedScript) to work

Is actually meant here:

...
let someTrustedScript = somePolicy.createScript("alert(1)");
eval(someTrustedScript);
new Function(someTrustedScript);

How does a working example for

Function(TrustedScript, TrustedScript)

look like?

mbrodesser-Igalia avatar Jan 22 '24 13:01 mbrodesser-Igalia

Similarly, i.e. every argument to Function constructor is a TrustedScript (created from a policy, or from fromLiteral), or a string.

Proposed dynamic-code-brand-checks in ES assemble the function body, stringifying all arguments, and tracking whether all were TrustedScripts (using assembledFromCodeLike spec variable).

After assembling, assembledFromCodeLike is passed back to the host, which based on CSP settings, rejects the value and prevents execution, or calls a default TT policy, or allows.

koto avatar Jan 22 '24 13:01 koto

Function(TrustedScript, TrustedScript)

is confusing, because Function's first argument is not arbitrary script, but a JS parameter. E.g.: https://jsfiddle.net/7cmefw9q/

mbrodesser-Igalia avatar Jan 22 '24 13:01 mbrodesser-Igalia

Correct, but it can still execute arbitrary JS, and the type to express that capability is TrustedScript.

let p = trustedTypes.createPolicy("p",
{ createScript: (t) => t });

let ts0 = p.createScript("x = alert(2)");
let ts1 = p.createScript("");


console.log(new Function(ts0, ts1)());

koto avatar Jan 22 '24 13:01 koto

Correct, but it can still execute arbitrary JS,

No, see e.g. https://jsfiddle.net/aLgctrsn/. MDN might be wrong here (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/Function#syntax), but presumably it's not arbitrary JS to be executed here. See the spec at https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-function-p1-p2-pn-body which calls https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-createdynamicfunction. Step 10-d-iii of the latter constructs the parameters as a comma separated string of the arguments.

and the type to express that capability is TrustedScript.

let p = trustedTypes.createPolicy("p",
{ createScript: (t) => t });

let ts0 = p.createScript("x = alert(2)");
let ts1 = p.createScript("");


console.log(new Function(ts0, ts1)());

CurrentlyTrustedScript (or presumably TrustedHTML) needs to be abused for those parameters. Presumably there could be an alias for TrustedScript, e.g. TrustedFunctionArgument relying on new Function() to fail for wrong input.

mbrodesser-Igalia avatar Jan 22 '24 14:01 mbrodesser-Igalia

Fair point, i was using the term arbitrary in a colloquial sense, from a security perspective. Function argument declarations can define and/or call other functions, which is XSS-equivalent in Trusted Types context:

new Function("a = (()=>{let c = 2; alert(c)})()", "return a")()

Technically, this could be represented a different type, but we didn't find a strong reason for it. Similarly TrustedHTML is required for both document.write and innerHTML assignments though different HTML parsers are used in both cases.

koto avatar Jan 22 '24 14:01 koto

It's a detail and one can reconsider it when a solution for the actual issue of this ticket is found. Updating MDN with an appropriate note might suffice.

Anyway wondering how often new Function(...) is used by web-developers.

mbrodesser-Igalia avatar Jan 22 '24 15:01 mbrodesser-Igalia

https://github.com/tc39/proposal-dynamic-code-brand-checks/pull/10 - I've started to clean up the dynamic code brand checks proposal repo.

lukewarlow avatar Feb 20 '24 18:02 lukewarlow

Based on discussions regarding the above linked PR I have what I think is an idea that could work for TT and eval+Function.

It would potentially be a different behaviour than expected for Function. I would need to check what Chrome's existing behaviour for new Function and TT to make sure my idea is web compatible.

I'll finish the PR to the proposal and make a corresponding trusted types one and we can evaluate if we think it's a tenable solution to the issue.

Edit: Chrome's current behaviour means my plan can't be as simple as I'd hoped due to web compat issues.

Tldr was to use the bodyString and parameterStrings that are already passed into the HostEnsureCanCompileStrings, but unfortunately we'll need to pass in the full compiled code string.

I do wonder how useful it actually is to give someone a precompiled string for a function?

image

lukewarlow avatar Feb 20 '24 20:02 lukewarlow

@lukewarlow I think that was always the idea, to give you the compiled code string via HostEnsureCanCompileStrings. cc. @nicolo-ribaudo @ptomato

caridy avatar Feb 21 '24 04:02 caridy

@caridy So HostEnsureCanCompileStrings now gets a list of parameter strings and the body string, I was hoping we could avoid needing to pass through the compiled string as well but it seems we can't because Chromium already ships with the full string passed through.

/\(function anonymous\((.*,+.*)\\n\) {\\n(.*)\\n}\)/ - I'm guessing that consumers do something like this regex to extract the parameter list CSV and the body string from the function to actually process? Or is it more a case of just ensuring the string matches verbatim against an allowlist?

Either way I've got a PR that updates the dynamic code proposal to match the current state of the Ecmascript spec to ensure it's kept up to date. Likewise I've made a PR for this spec to follow the updated version.

I'll read through prior discussion on the proposal as to why it didn't get out of stage 1 originally to try and address any if we can. The observable behaviour change from moving the call has already been merged so that should make this proposal less contentious now.

lukewarlow avatar Feb 21 '24 12:02 lukewarlow

I'm a bit confused there's https://github.com/tc39/ecma262/pull/1498 - which mentions it's for trusted types, but the shape is different from the dynamic brand checks proposal. The dynamic brand checks proposal is the latest version right?

If so does it make sense to close that linked PR as it's both outdated on the ecmascript side, and outdated in terms of what we'd actually be proposing?

lukewarlow avatar Feb 21 '24 12:02 lukewarlow

@koto can you link to the issue tracking the addition of the new keyword script-src 'trusted-script'? We will like to see that done sooner rather than later considering the imminent enforcing of the regulations in Europe.

@caridy fyi I've reopened https://github.com/w3c/trusted-types/issues/221 to discuss the use case for a new CSP keyword that allows trusted eval without using the 'unsafe-eval' keyword.

lukewarlow avatar Feb 21 '24 15:02 lukewarlow

Yes, it probably makes sense to close tc39/ecma262#1498 if there's something that supersedes it.

ptomato avatar Feb 21 '24 15:02 ptomato

See https://github.com/w3c/trusted-types/issues/461 for discussion about removing default policy handling from eval and co. This is a change that should help make the tc39 change less contentious.

lukewarlow avatar Feb 29 '24 12:02 lukewarlow

https://github.com/tc39/ecma262/pull/3294 - I've opened a draft PR with the changes from the dynamic code brand checks proposal. WIll work on relevant tests needed as well.

lukewarlow avatar Mar 07 '24 15:03 lukewarlow