-
Notifications
You must be signed in to change notification settings - Fork 73
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
Finalize the integrations that guard eval & Function.constructor #207
Comments
The Blink/V8 implementation already has the behavior above. The ECMAScript bits are progressing through TC39. |
Note: change the spec text once https://github.com/tc39/proposal-dynamic-code-brand-checks stabilizes. It will likely require adding a slot to |
dynamic-code-brand-checks proposal. See w3c#207.
@koto can you link to the issue tracking the addition of the new keyword |
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 we got consensus around tc39/ecma262#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 tc39/ecma262#1498 (comment), do you see a path forward there to get this issue folded into it? |
Based on tc39/ecma262#3222 (comment), 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 tc39/ecma262#3222 was to supersede tc39/ecma262#1498. |
@koto The motivation for that ECMA-262 PR is to allow the |
Thanks for context. Do you know what caused the change during the plenary? What were there concerns to give host the unstringified value? |
Yes. The plenary change was motivated by the desire to expose the string to the host so that CSP's 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. |
Given the Dynamic Code Branch Checks proposal seems to be stuck at stage 1 it would be good to work out the alternatives. |
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? |
Similarly, i.e. every argument to Proposed dynamic-code-brand-checks in ES assemble the function body, stringifying all arguments, and tracking whether all were TrustedScripts (using After assembling, |
Function(TrustedScript, TrustedScript) is confusing, because |
Correct, but it can still execute arbitrary JS, and the type to express that capability is
|
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.
Currently |
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 |
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 |
tc39/proposal-dynamic-code-brand-checks#10 - I've started to clean up the dynamic code brand checks proposal repo. |
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? |
@lukewarlow I think that was always the idea, to give you the compiled code string via |
@caridy So
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. |
I'm a bit confused there's tc39/ecma262#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? |
@caridy fyi I've reopened #221 to discuss the use case for a new CSP keyword that allows trusted eval without using the 'unsafe-eval' keyword. |
Yes, it probably makes sense to close tc39/ecma262#1498 if there's something that supersedes it. |
See #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. |
tc39/ecma262#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. |
Closing this as the changes required are largely finalised. |
Essentially, we'd like:
eval(TrustedScript)
,new Function(TrustedScript)
, andnew Function(TrustedScript, TrustedScript)
to workcreateScript
function (a.k.a. if TT are enforced, to generate violation and not execute code by default)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'
The text was updated successfully, but these errors were encountered: