Bug 173892 – WebAssembly: disable some APIs under CSP

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

[JF Bastien]

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).

[JF Bastien]

<rdar://problem/32914613>

[JF Bastien]

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

[JF Bastien]

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]

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]

Created attachment 314071 [details]
patch

This should be ready to go!

[Build Bot]

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 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]

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 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]

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 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]

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 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]

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]

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]

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]

Created attachment 314118 [details]
patch

Fix path of tests in iOS simulator test expectations.

[Daniel Bates]

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 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
> +<!-- FIXME: add WebAssembly.compile and WebAssembly.instantiate -->

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]

> > LayoutTests/http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html:25
> > +<!-- FIXME: add WebAssembly.compile and WebAssembly.instantiate -->
> 
> 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]

Created attachment 314144 [details]
patch

Address comments.

[Build Bot]

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 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 on attachment 314144 [details]
patch

Clearing flags on attachment: 314144

Committed r218951: <http://trac.webkit.org/changeset/218951>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[JF Bastien]

(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 :)