Bug 173892

Summary:

WebAssembly: disable some APIs under CSP

Product:

WebKit

Reporter:

JF Bastien <jfbastien>

Component:

JavaScriptCore

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, commit-queue, dbates, esprehn+autocc, fpizlo, jfbastien, kangil.han, keith_miller, mark.lam, mkwst, msaboff, rniwa, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=111869
https://bugs.webkit.org/show_bug.cgi?id=173105

Bug Depends on:

Bug Blocks:

173977

Attachments:

Description	Flags
patch	jfbastien: review-, jfbastien: commit-queue-
patch	jfbastien: review-, jfbastien: commit-queue-
patch	buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews112 for mac-elcapitan	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
Archive of layout-test-results from ews102 for mac-elcapitan	none
patch	none
patch	dbates: review+, dbates: commit-queue-
patch	none

JF Bastien

Reported 2017-06-27 15:16:42 PDT

We should disable parts of WebAssembly under Content Security Policy as discussed here: https://github.com/WebAssembly/design/issues/1092 Exactly what should be disabled isn't super clear, so we may as well be conservative and disable many things if developers already opted into CSP. It's easy to loosen what we disable later. I plan on disabling: - WebAssembly.Instance - WebAssembly.Memory - WebAssembly.Table And leaving: - WebAssembly on the global object - WebAssembly.Module - WebAssembly.CompileError - WebAssembly.LinkError That way it won't be possible to call WebAssembly-compiled code, or create memories (which use fancy 4GiB allocations sometimes). Table isn't really useful on its own, and eventually we may make them shareable so without more details it seems benign to disable them (and useless if we don't).

Attachments
patch (27.00 KB, patch) 2017-06-28 12:57 PDT, JF Bastien	jfbastien: review- jfbastien: commit-queue-	Details Formatted Diff Diff
patch (29.66 KB, patch) 2017-06-28 14:44 PDT, JF Bastien	jfbastien: review- jfbastien: commit-queue-	Details Formatted Diff Diff
patch (45.38 KB, patch) 2017-06-28 16:58 PDT, JF Bastien	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.16 MB, application/zip) 2017-06-28 18:25 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.85 MB, application/zip) 2017-06-28 18:40 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (20.91 MB, application/zip) 2017-06-28 18:51 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews102 for mac-elcapitan (1007.49 KB, application/zip) 2017-06-28 19:05 PDT, Build Bot	no flags	Details
patch (45.68 KB, patch) 2017-06-28 22:36 PDT, JF Bastien	no flags	Details Formatted Diff Diff
patch (45.62 KB, patch) 2017-06-28 22:56 PDT, JF Bastien	dbates: review+ dbates: commit-queue-	Details Formatted Diff Diff
patch (46.73 KB, patch) 2017-06-29 11:10 PDT, JF Bastien	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

JF Bastien

Comment 1 2017-06-27 15:17:41 PDT

<rdar://problem/32914613>

JF Bastien

Comment 2 2017-06-27 15:18:59 PDT

*** Bug 173688 has been marked as a duplicate of this bug. ***

JF Bastien

Comment 3 2017-06-28 12:57:43 PDT

Created attachment 314053 [details] patch Here's the current patch. The test produces no output though, and I'm not sure why! Some of these should fail...

JF Bastien

Comment 4 2017-06-28 14:44:45 PDT

Created attachment 314058 [details] patch Saam helped me find my obvious mistake. I'd dropped the part that actually enables... Trying it out, and I'll add other tests. I also need to fix the async tests.

JF Bastien

Comment 5 2017-06-28 16:58:41 PDT

Created attachment 314071 [details] patch This should be ready to go!

Build Bot

Comment 6 2017-06-28 17:00:34 PDT

Attachment 314071 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.h:44: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 7 2017-06-28 18:25:30 PDT

Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4016792 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html

Build Bot

Comment 8 2017-06-28 18:25:34 PDT

Created attachment 314080 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 9 2017-06-28 18:40:04 PDT

Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4016777 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html

Build Bot

Comment 10 2017-06-28 18:40:06 PDT

Created attachment 314082 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 11 2017-06-28 18:51:17 PDT

Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4016809 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-iframe.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-script.html

Build Bot

Comment 12 2017-06-28 18:51:19 PDT

Created attachment 314086 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Build Bot

Comment 13 2017-06-28 19:05:09 PDT

Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4016950 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html

Build Bot

Comment 14 2017-06-28 19:05:10 PDT

Created attachment 314088 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6

JF Bastien

Comment 15 2017-06-28 22:36:11 PDT

Created attachment 314115 [details] patch Fix failures, add a test I'd missed, and fix style. The iOS simulator failures were due to WebAssembly being disabled on the simulator.

Build Bot

Comment 16 2017-06-28 22:38:20 PDT

Attachment 314115 [details] did not pass style-queue: ERROR: LayoutTests/platform/ios-simulator/TestExpectations:137: Path does not exist. [test/expectations] [5] ERROR: LayoutTests/platform/ios-simulator/TestExpectations:138: Path does not exist. [test/expectations] [5] ERROR: LayoutTests/platform/ios-simulator/TestExpectations:139: Path does not exist. [test/expectations] [5] ERROR: LayoutTests/platform/ios-simulator/TestExpectations:140: Path does not exist. [test/expectations] [5] ERROR: LayoutTests/platform/ios-simulator/TestExpectations:141: Path does not exist. [test/expectations] [5] Total errors found: 5 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 17 2017-06-28 22:56:14 PDT

Created attachment 314118 [details] patch Fix path of tests in iOS simulator test expectations.

Daniel Bates

Comment 18 2017-06-29 10:44:00 PDT

Comment on attachment 314118 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314118&action=review This patch looks good. I had some very minor stylistic nits. Please remove leading whitespace on empty lines. Every added empty line I checked has leading whitespace. > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:103 > + auto* jsmemory = JSWebAssemblyMemory::create(exec, vm, exec->lexicalGlobalObject()->WebAssemblyMemoryStructure(), adoptRef(*memory.leakRef())); jsmemory => jsMemory (since the name contains two words: "js" - shorthand for JavaScript and "memory") > Source/WebCore/ChangeLog:13 > + Tests: http/tests/security/contentSecurityPolicy/WebAssembly-allowed.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-iframe.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-script.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html Nit: The list of tests should come after the description of the fix as seen in the example ChangeLog on <https://webkit.org/contributing-code/#changelog-files>. > Source/WebCore/ChangeLog:16 > + WebAssembly-blocked but currently only blocks neither or both. I Nit: Missing a comma before "but". > Source/WebCore/bindings/js/WorkerScriptController.cpp:198 > + JSLockHolder lock(vm()); Although it is an unwritten convention (we should make it written) we have been using C++11 brace initialization syntax in new code.

Keith Miller

Comment 19 2017-06-29 10:53:29 PDT

Comment on attachment 314118 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314118&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html:25 > + Why can't we do these now? We must have some promise testing api. We should at least file a bug about this. >> Source/WebCore/bindings/js/WorkerScriptController.cpp:198 >> + JSLockHolder lock(vm()); > > Although it is an unwritten convention (we should make it written) we have been using C++11 brace initialization syntax in new code. Really? We definitely don't do that in JSC...

JF Bastien

Comment 20 2017-06-29 11:10:08 PDT

> > LayoutTests/http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html:25 > > + > > Why can't we do these now? We must have some promise testing api. We should > at least file a bug about this. https://bugs.webkit.org/show_bug.cgi?id=173977

JF Bastien

Comment 21 2017-06-29 11:10:32 PDT

Created attachment 314144 [details] patch Address comments.

Build Bot

Comment 22 2017-06-29 11:12:30 PDT

Attachment 314144 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:190: Missing space before { [whitespace/braces] [5] ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:198: Missing space before { [whitespace/braces] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 23 2017-06-29 11:36:51 PDT

Comment on attachment 314144 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314144&action=review > Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:48 > + auto exception = [&] (JSObject* error) { > + throwException(exec, throwScope, error); > + return nullptr; > + }; there is no need for this function.

WebKit Commit Bot

Comment 24 2017-06-29 11:49:23 PDT

Comment on attachment 314144 [details] patch Clearing flags on attachment: 314144 Committed r218951: <http://trac.webkit.org/changeset/218951>

WebKit Commit Bot

Comment 25 2017-06-29 11:49:25 PDT

All reviewed patches have been landed. Closing bug.

JF Bastien

Comment 26 2017-06-29 13:02:05 PDT

(In reply to Saam Barati from comment #23) > Comment on attachment 314144 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314144&action=review > > > Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:48 > > + auto exception = [&] (JSObject* error) { > > + throwException(exec, throwScope, error); > > + return nullptr; > > + }; > > there is no need for this function. I imported it from another file, I like how it makes the code easier to read even if single-use here :)

Note You need to log in before you can comment on or make changes to this bug.