| Summary: | Function constructor needs to follow the spec and validate parameters and body independently | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark S. Miller <erights> | ||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, bfulgham, buildbot, commit-queue, erights, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, product-security, rniwa, saam, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=106160 https://bugs.webkit.org/show_bug.cgi?id=131137 https://github.com/tc39/proposal-realms/issues/54#issuecomment-308017282 |
||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
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. (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; } (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. 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.
Created attachment 313869 [details]
WIP
Created attachment 313870 [details]
WIP
Created attachment 313872 [details]
patch
Comment on attachment 313872 [details]
patch
r=me.
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 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 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
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 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
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 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
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 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
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. Created attachment 313928 [details]
patch for landing
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 Created attachment 313931 [details]
patch for landing
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 Created attachment 313940 [details]
patch for landing
Comment on attachment 313940 [details] patch for landing Clearing flags on attachment: 313940 Committed r218845: <http://trac.webkit.org/changeset/218845> All reviewed patches have been landed. Closing bug. |
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.