Bug 173303 – Function constructor needs to follow the spec and validate parameters and body independently

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark S. Miller]

In the JavaScript console:

> Function('/*', '*/){')
< function anonymous(/*) {
*/){
}

This violates the spec and enables injection attacks. According to the spec, the code above should be rejected with a syntax error because the last argument does not parse as a valid function body. This can be used, and has been used, by secure frameworks to ensure that untrusted strings parse as valid function bodies. Such secure frameworks can be fooled on JSC because of this bug.

[Radar WebKit Bug Importer]

<rdar://problem/32732526>

[Keith Miller]

Interesting, I wonder what the right solution to this is... Saam, do you have any thoughts?

P.S. I moved this to security because why not.

[Saam Barati]

(In reply to Keith Miller from comment #2)
> Interesting, I wonder what the right solution to this is... Saam, do you
> have any thoughts?
> 
> P.S. I moved this to security because why not.

I'm going to read the spec now, but I don't have any profound ideas here. It seems like this will require us refactoring the parser a bit. The interesting thing is that it seems possible that something can be a valid function body, however, in the context of the parameters, is no longer valid:
function foo(a) { "use strict"; let a; }

[Saam Barati]

(In reply to Saam Barati from comment #3)
> (In reply to Keith Miller from comment #2)
> > Interesting, I wonder what the right solution to this is... Saam, do you
> > have any thoughts?
> > 
> > P.S. I moved this to security because why not.
> 
> I'm going to read the spec now, but I don't have any profound ideas here. It
> seems like this will require us refactoring the parser a bit. The
> interesting thing is that it seems possible that something can be a valid
> function body, however, in the context of the parameters, is no longer valid:
> function foo(a) { "use strict"; let a; }

Actually, I think there is a way to avoid this:
we could use the syntax checker to parse the body without any parameters.

[Mark S. Miller]

I am curious what the spec actually says about your example

new Function('a', '"use strict"; let a;');

I assume it would be an early error since it results in a function that would be an early error if written out. FWIW, JSC, V8, and SpiderMonkey all do reject it with an early error already.

I do not know your parser, but yes, if you can simply ask your parser to do a validating parse with FunctionBody as the start symbol, then that would take care of the issue raised by this bug. Your existing behavior then would still take care of the issue raised in comment #3.

[Saam Barati]

Created attachment 313869 [details]
WIP

[Saam Barati]

Created attachment 313870 [details]
WIP

[Saam Barati]

Created attachment 313872 [details]
patch

[Keith Miller]

Comment on attachment 313872 [details]
patch

r=me.

[Build Bot]

Comment on attachment 313872 [details]
patch

Attachment 313872 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4003110

New failing tests:
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout-no-llint
ChakraCore.yaml/ChakraCore/test/Function/FuncBody.js.default
jsc-layout-tests.yaml/js/script-tests/function-constructor-single-line-comment.js.layout-ftl-eager-no-cjit
apiTests

[Build Bot]

Comment on attachment 313872 [details]
patch

Attachment 313872 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4003189

New failing tests:
fast/events/onload-single-line-comment.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
js/function-constructor-single-line-comment.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
inspector/debugger/search-scripts.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html

[Build Bot]

Created attachment 313878 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313872 [details]
patch

Attachment 313872 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4003187

New failing tests:
fast/events/onload-single-line-comment.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
js/function-constructor-single-line-comment.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html

[Build Bot]

Created attachment 313879 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313872 [details]
patch

Attachment 313872 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4003233

New failing tests:
fast/events/onload-single-line-comment.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
js/function-constructor-single-line-comment.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html

[Build Bot]

Created attachment 313880 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 313872 [details]
patch

Attachment 313872 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4003586

New failing tests:
fast/events/onload-single-line-comment.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
js/function-constructor-single-line-comment.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
inspector/debugger/search-scripts.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html

[Build Bot]

Created attachment 313887 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Saam Barati]

I need to make sure we have a newline before the starting/closing brace of a body. I also need to rebaseline some tests I think. Running tests locally now.

[Saam Barati]

Created attachment 313928 [details]
patch for landing

[Build Bot]

Comment on attachment 313928 [details]
patch for landing

Attachment 313928 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4006973

New failing tests:
ChakraCore.yaml/ChakraCore/test/Function/FuncBody.js.default

[Saam Barati]

Created attachment 313931 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 313931 [details]
patch for landing

Rejecting attachment 313931 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 313931, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4008044

[Saam Barati]

Created attachment 313940 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 313940 [details]
patch for landing

Clearing flags on attachment: 313940

Committed r218845: <http://trac.webkit.org/changeset/218845>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.