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

Provide HostEnsureCanCompileString implementors more context #1498

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikesamuel
Copy link
Member

@mikesamuel mikesamuel commented Apr 3, 2019

Originally floated as a strawman

eval is Evil

The eval function is the most misused feature of JavaScript. Avoid it.

-- Douglas Crockford, "JavaScript: The Good Parts"

eval and its friend new Function are problematic because, too
often, an attacker can turn it against the application.

Most code avoids eval, but JS programs are no longer small, and
self-contained as they were when Crock wrote that.

If one module uses eval, even if it's as simple as
Function('return this')() to get a handle to the
global object then eval has to work.

This prevents the use of security measures like:

which turn off eval globally.

As JavaScript programs get larger, the chance that no part of it needs eval or Function()
to operate gets smaller.


It is difficult in JavaScript for a code reviewer to determine that
code never uses these operators. For example, the below can when x is constructor.

({})[x][x](y)()

// ({})[x]       === Object
// ({})[x][x]    === Function
// ({})[x][x](y)  ~  Function(y)

So code review and developer training are unlikely to prevent abuse of these
operators.


This aims to solve the problem by providing more context to host environments so that they
can make finer-grained trust decisions.

The Trusted Types proposal aims to allow JavaScript development to scale securely.

It makes it easy to separate:

  • the decisions to trust a chunk of code to load.
  • the check that an input to a sensitive operator like eval is trustworthy.

This allows trust decisions tto be made where the maximum context is
available and allows these decisions to be concentrated in small
amounts of thoroughly reviewed code

The checks can also be moved into host code so that they reliably happen before
irrevocable actions like code loading complete.

Specifically, Trusted Types would like to require that the code portion of the
inputs to %Function% and %eval% are TrustedScript.

Fixes #938.

Originally floated as a [strawman][1]

> ### `eval` is Evil
>
> The `eval` function is the most misused feature of JavaScript. Avoid it.
>
> -- <cite>Douglas Crockford, "JavaScript: The Good Parts"</cite>

`eval` and its friend `new Function` are problematic because, too
often, an attacker can turn it against the application.

Most code avoids `eval`, but JS programs are no longer small, and
self-contained as they were when Crock wrote that.

If one module uses `eval`, even if it's as simple as
[`Function('return this')()`][2] to get a handle to the
global object then `eval` has to work.

This prevents the use of security measures like:

*  [Content-Security-Policy][]
*  `node --disallow_code_generation_from_strings`

which turn off `eval` globally.

As JavaScript programs get larger, the chance that no part of it needs `eval` or `Function()`
to operate gets smaller.

----

It is difficult in JavaScript for a code reviewer to determine that
code never uses these operators.  For example, the below can when `x` is constructor.

```js
({})[x][x](y)()

// ({})[x]       === Object
// ({})[x][x]    === Function
// ({})[x][x](y)  ~  Function(y)
```

So code review and developer training are unlikely to prevent abuse of these
operators.

----

This aims to solve the problem by providing more context to host environments so that they
can make finer-grained trust decisions.

The [Trusted Types proposal][3] aims to allow JavaScript development to scale securely.

It makes it easy to separate:

*  the decisions to trust a chunk of code to load.
*  the check that an input to a sensitive operator like `eval` is trustworthy.

This allows trust decisions tto be made where the maximum context is
available and allows these decisions to be concentrated in small
amounts of thoroughly reviewed code

The checks can also be moved into host code so that they reliably happen before
irrevocable actions like code loading complete.

Specifically, Trusted Types would like to require that the code portion of the
inputs to %Function% and %eval% are [*TrustedScript*][4].

[1]: https://github.com/mikesamuel/proposal-hostensurecancompilestrings-passthru/
[2]: https://github.com/zloirock/core-js/blob/2a005abe68520248d4431cab70d86e40b55d6e98/packages/core-js/internals/global.js#L5
[3]: https://wicg.github.io/trusted-types/dist/spec/
[4]: https://wicg.github.io/trusted-types/dist/spec/#typedef-trustedscript
[Content-Security-Policy]: https://csp.withgoogle.com/docs/index.html
@mikesamuel
Copy link
Member Author

I'd like to schedule this for a needs_consensus slot per tc39/proposals#209 (comment)

This should be a semantics free change from the ES side.

Tracking issue filed on the HTML5 side: whatwg/html#4501

1. If _argCount_ = 0, let _bodyText_ be the empty String.
1. Else if _argCount_ = 1, let _bodyText_ be _args_[0].
1. Else _argCount_ &ge; 1,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of splitting the loop out of this if-tree is to preserve the order of side effects due to stringification while allowing the host callout access to bodyText.

The order of events should stay the same:

  1. Host callout
  2. Parameter name arguments stringified in order
  3. Body text stringified

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Apr 3, 2019
@mikesamuel
Copy link
Member Author

How might one specify tests?

AFAIK, there's no way for a test262 implementation to specify that a test runs in the context of a less-than-completely permissive host callout. tc39/test262#2120

Would it be sufficient to include a prose section on what to test like the below?

Run in the context of a HostEnsureCanCompileStrings callout that
does blah blah blah:

// test code here

like https://gist.github.com/mikesamuel/5c71584fc10a603167111e933cdc650d

@mikesamuel
Copy link
Member Author

Fixes #938

@ljharb
Copy link
Member

ljharb commented Apr 3, 2019

It may indeed not be possible to test; that's something i think the test262 maintainers can help confirm.

@littledan
Copy link
Member

littledan commented Apr 7, 2019

I think you'd need more changes than just this. Step 2 of PerformEval will not evaluate non-String arguments, so it wouldn't really be very useful to apply with a TrustedScript.

Edit: I see, this is covered by #1495
Edit 2: Actually, it doesn't.

@mikesamuel
Copy link
Member Author

@Jamesernator
Copy link

Question. Do we even need to callout to HostEnsureCanCompileStrings for the case of new Function() (with no params)? It doesn't seem to me that new Function() on it's own can be used as a vector for XSS attacks.

@mikesamuel
Copy link
Member Author

@Jamesernator
You're right that (new Function) is not an XSS risk but I suspect I'm missing something more in your question.

IIUC new Function reaches CreateDynamicFunction with zero arguments which reaches 19.2.1.1.1.1 step 13

  1. If argCount = 0, let bodyText be the empty String.

so new Function('') and (new Function) are equivalent and both are already guarded by the host callout.

@Jamesernator
Copy link

It just seems surprising to me that new Function() needs to invoke the machinery for checking whether or not a string is safe given that it wasn't given a string in the first place. (Given that eval(nonString) doesn't bother invoking the HostEnsureCanCompileStrings machinery when it's not given a string).

Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This layering change generally LGTM. I'd like to wait to land it until we have a PR within a host environment that takes advantage of this PR.

1. Let _bodyText_ be _args_[_argCount_ - 1].
1. Perform ? HostEnsureCanCompileStrings(_callerRealm_, _calleeRealm_, _goal_, _bodyText_).
1. Let _P_ be the empty String.
1. If _argCount_ = 1, let _bodyText_ be _args_[0].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the purpose of this line. Wasn't this already handled four lines up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It indeed was.

mikesamuel added a commit to tc39/agendas that referenced this pull request Jun 2, 2019
Instead of splitting discussion of a related topic into 10 min for [an editorial change](tc39/ecma262#1498) and 15 min for what [WICG/trusted-types](https://mikesamuel.github.io/proposal-hostensurecancompilestrings-passthru/) needs, consolidate them into one discussion of the latter.

Apologies if the diff is ugly.  I just removed one row from one table and added its time to a row in another table.
1. If _argCount_ = 0, let _bodyText_ be the empty String.
1. Else if _argCount_ = 1, let _bodyText_ be _args_[0].
1. Else _argCount_ &ge; 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Else _argCount_ &ge; 1,
1. Else,
1. Assert: _argCount_ &ge; 1,

Comment on lines +24706 to 24707
1. If _argCount_ = 1, let _bodyText_ be _args_[0].
1. Else _argCount_ &gt; 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If _argCount_ = 1, let _bodyText_ be _args_[0].
1. Else _argCount_ &gt; 1,
1. If _argCount_ &gt; 1,

@caridy
Copy link
Contributor

caridy commented Nov 2, 2023

We would try to revamp this PR (cc @ptomato)

ptomato pushed a commit to ptomato/ecma262 that referenced this pull request Nov 16, 2023
This is a continuation of Mike Samuel's work in tc39#1498. Due to tc39#2670, this
change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

The original purpose of tc39#1498 was to support Trusted Types, which is still
a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks
@ptomato
Copy link
Contributor

ptomato commented Nov 16, 2023

I've opened a rebased version of this over at #3222.

ptomato pushed a commit to ptomato/ecma262 that referenced this pull request Nov 16, 2023
This is a continuation of Mike Samuel's work in tc39#1498. Due to tc39#2670, this
change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

The original purpose of tc39#1498 was to support Trusted Types, which is still
a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks
ptomato added a commit to ptomato/ecma262 that referenced this pull request Nov 17, 2023
This is a continuation of Mike Samuel's work in tc39#1498. Due to tc39#2670, this
change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

The original purpose of tc39#1498 was to support Trusted Types, which is still
a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks

Co-authored-by: Philip Chimento <pchimento@igalia.com>
ptomato added a commit to ptomato/ecma262 that referenced this pull request Nov 17, 2023
This is a continuation of Mike Samuel's work in tc39#1498. Due to tc39#2670, this
change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

The original purpose of tc39#1498 was to support Trusted Types, which is still
a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks

Co-authored-by: Philip Chimento <pchimento@igalia.com>
ptomato added a commit to ptomato/ecma262 that referenced this pull request Nov 22, 2023
This is a continuation of Mike Samuel's work in tc39#1498. Due to tc39#2670, this
change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

The original purpose of tc39#1498 was to support Trusted Types, which is still
a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks

Co-authored-by: Philip Chimento <pchimento@igalia.com>
ptomato added a commit to ptomato/ecma262 that referenced this pull request Nov 29, 2023
This is a continuation of Mike Samuel's work in tc39#1498. Due to tc39#2670, this
change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

The original purpose of tc39#1498 was to support Trusted Types, which is still
a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks

Co-authored-by: Philip Chimento <pchimento@igalia.com>
@caridy
Copy link
Contributor

caridy commented Nov 29, 2023

@ptomato should we close this, or do you prefer to keep it around for when the time comes to work on trusted types?

@ljharb
Copy link
Member

ljharb commented Nov 29, 2023

If it should be closed, it’d be done on merging of the replacement, I’d expect.

@nicolo-ribaudo
Copy link
Member

Note that the other PR only replaces part of the motivation of this one. If we'll want to ever support trusted types, we'll need to also pass the uncoerced objects to the host (which is what this PR does).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include the string to be compiled in the call to HostEnsureCanCompileStrings
8 participants