URGENT SECURITY Function constructor spec creates massive code injection vulnerability

[erights]

Somehow the text of the Function constructor specs was accidentally changed in a way that, if implemented, would create massive code injection vulnerabilities for code already deployed.

https://tc39.es/ecma262/#sec-function-p1-p2-pn-body

The old spec, and the behavior of v8 (Brave, Chromium, Chrome, Node), SpiderMonkey (Firefox), and JSC (Safari), (and IIRC Edge, though I cannot currently test that) all agree that the code

new Function(attackerProvidedString);

would validate whether the attackerProvidedString is a syntactically valid FunctionBody (or GeneratorBody, ...) without any possibility that attacker provided code would get run as a consequence. If the string is a valid FunctionBody, it would return a function, and otherwise it would throw a SyntaxError.

The current definition creates a source string by concatenation and then parses only the concatenated string. Thus

new Function('}), a=1, (function(){');

would be concatenated into the source string

function anonymous(
)) {}), a=1, (function(){}

which parses, with the resulting ast meeting all the requirements set out in the rest of that spec. The Function constructor spec would not itself cause the injected code to execute. But other code treats this success as validating that this string is a syntactically valid function body, that may then be safely concatenated in exactly this way without creating an injection attack.

See Moddable-OpenSource/moddable#623

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]

@ljharb I see your point. It is possible that is a sufficient constraint. But it is a syntax constraint imposed after concatenation, and so too delicate and indirect. As shown by the bug we just filed against XS, this subtle constraint is too subtle to reliably lead developers to do the validation of bodyArg that others rely on to prevent injection attacks. So, even if this is not technically wrong, it fails to do the job the written spec must do.

[erights]

It is also subtle enough that I missed it just now, when I was explicitly looking for it, and when I filed these bugs. I did not see it until you pointed it out. Of those not looking for it, how many will miss it?

[erights]

And at the beginning of Moddable-OpenSource/moddable#623 we see that @dckc missed it too.

[ljharb]

https://262.ecma-international.org/6.0/#sec-createdynamicfunction contains basically the same spec text, and this appears to be the first implementation that chose an approach subject to that flaw (there have been multiple new implementations since that time).

I do see that https://262.ecma-international.org/5.1/#sec-15.3.2.1 specifies it a bit more tightly, parsing the argument list and the body separately, but I'm not sure that remains a tenable approach given the 4 kinds of constructible functions we now have.

I'm sure the editors would be open to a PR that clarified the requirement, but I'm not sure how urgent the issue is.

[bakkot]

This was just changed in #2348. I believed at the time, and still believe on review, that the change in that PR is strictly editorial (per @ljharb's comment above) and is a considerable simplification. It wasn't the cause of the XS bug, which considerably predates this change to the specification.

I'm happy to call out this subtlety somehow, perhaps with a NOTE in the vicinity of the relevant step. However, I don't want to go back to the old spec text, which had an unmaintainable list of syntax errors copied from multiple other places in the spec.

[dckc]

a test262 test is more likely to reach the intended audience than spec text, I expect. I had planned to file an issue or PR, but I wasn't sure how to justify it from the text. If it can be justified, please do add a test.

[erights]

I would be happy simply to have this validation made explicit. There should be a separate parse step before concatenation like

ParseText(bodyArg, bodySym)

where bodySym is the start production for the appropriate function body. Given the amazing bizarreness of simply lexing/tokenizing JS accurately, and given that the grammar changes over time, I do not want to rely on the subtle implication that @ljharb points out. I certainly do not want to rely on that implication remaining true over time as the grammar changes without people reexamining this issue.

[erights]

I see at that change that the old text included

Let _goal_ be the grammar symbol |FunctionBody[~Yield, ~Await]|

That, and then doing a validating ParseText using that, must be restored.

[bakkot]

I'm willing to do that, but I'm not convinced that it's better than a simple note or assertion which points out to the reader the importance of validating that the resulting parse node is in fact a single instance of FunctionExpression (and perhaps explaining that this implies that _bodyString_ must be a valid FunctionBody).

[bakkot]

I've opened #2374. @erights does that look good to you?

[erights]

Hi @bakkot yes! Thanks much!

Not that it is any longer relevant, but the reason that validating the post-concatenation node would not have been adequate is that the implication might get broken as the grammar changes, without anyone noticing the implications as the time. In any case, I am overjoyed that I no longer need to worry about that!

[erights]

Once #2374 is merged, as far as I am concerned, this bug can be closed.

[Jack-Works]

Function('//', 'a', 'return a').toString()

"function anonymous(//,a
) {
return a
}"

Function('//', 'a', 'return a')(1)

Uncaught ReferenceError: a is not defined

Is this expected?

[bakkot]

Yes, that's expected and unrelated to this change. It's worked like that forever and changing it would be breaking. (I recall prior discussions about this, but I can't find them offhand.)

[bakkot]

Re: test262, I couldn't find a test for the above case among the primary set, but there's a similar-ish implementation-contributed one from V8.