WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
173892
WebAssembly: disable some APIs under CSP
https://bugs.webkit.org/show_bug.cgi?id=173892
Summary
WebAssembly: disable some APIs under CSP
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 > +<!-- 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
Comment 20
2017-06-29 11:10:08 PDT
> > 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
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.
Top of Page
Format For Printing
XML
Clone This Bug