Function constructor subject to injection; can have global side-effects

[dckc]

Build environment: Linux
Target device: xst

Description
The Function constructor doesn't seem to constrain the body arg to a single function literal.

Steps to Reproduce

xst -e "const evil='}), a=1, (function(){'; Function(evil); print(a)"
1

Expected behavior

SyntaxError

for example, in node:

> Function('}), target(), (function(){')
Uncaught SyntaxError: Single function literal required

Other information

Several other injection examples are available in https://github.com/endojs/endo/blob/master/packages/ses/test/test-break-function-eval.js

p.s. IOU a test262 issue or PR, but...

@erights I can't quite tell which part of https://tc39.es/ecma262/#sec-function-constructor says that a SyntaxError is called for.

also: endojs/endo#508

[erights]

The spec got broken since I last looked at this. Whatever this breakage is, it is unintentional, does not have consensus, differs from all the browser-based implementations (I tested this), and would cause massive injection attacks against code already deployed if implemented according to spec. I will file a bug against the spec.

Attn @allenwb @Waldemar @littledan @ljharb @codehag When and how did this get broken?

[ljharb]

Hmm. https://tc39.es/ecma262/#sec-createdynamicfunction does seem to naively concatenate arg names, and the body text, but it calls ParseText in step 20 with a grammar symbol. Parsing this example seems like it should indeed produce a syntax error since it doesn't match the grammar for a single FunctionExpression, GeneratorExpression, or AsyncFunctionExpression, or AsyncGeneratorExpression.

Am I misreading it?

[erights]

tc39/ecma262#2373

[erights]

Further discussion at tc39/ecma262#2373

[erights]

From that further discussion, we found out that the spec has only been broken since tc39/ecma262#2348 which is three days old.

Attn @bakkot

[bakkot]

The spec is not broken. [Edit: or maybe it actually is...] Let's continue discussion there, though.

[dckc]

This looks mostly fixed, but I'm inclined to hold off on closing it until we have mitigated the remaining /* case in endojs/endo#665 .

[dckc]

@phoddie this is done to my satisfaction, so I'm closing it.

Perhaps it should be narrowed to the /* case and re-opened; I leave that up to you.

[erights]

Agreed. The remaining /* case is a bug --- a non-conformance to the spec. But it is no longer an issue of concern to Agoric.