-
Notifications
You must be signed in to change notification settings - Fork 694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSP interaction with WebAssembly JS APIs #1092
Comments
/cc @mikewest We used unsafe-eval since the team didn't want to block on introducing a wasm specific CSP directive. |
For reference: if unsafe-eval is set, can can script-created HTMLScriptElements be added and, if so, can they have a data or blob URLs? |
/cc @mikewest to double-check my spec-reading
Sort of. They can be added, but if the element does not have a src="" attribute (i.e. is an inline script), it will not evaluate.
Script elements with src="" attributes are not governed by unsafe-eval. They are governed by a separate directive, script-src, which has to separately opt in to data: and blob:. |
Forgive the naive questions:
|
A policy whose
Right. If the policy enables |
Ah, thanks for the clarification. So |
I'd best let others (in this case @jeisinger I think?) answer this, as I'm a little fuzzy on the details. I mostly just wanted to open this thread to highlight and track the non-interoperability here. |
unsafe-eval also blocks setTimeout with a string as handler. |
I still don't understand what is being proposed. I'm worried that developers use a pattern such as this to detect if wasm is available:
IIUC that'll be true, but will fail under CSP. |
I think we should keep WebAssembly.instantiate but disallow WebAssembly.compile. If we don't do this, we won't be future proofed against instantiating a thing that was compiled using compileStreaming. This will allow instantiation of Modules that are postMessaged from a worker if that worker was allowed to use WebAssembly.compile |
I haven't seen a concrete proposal yet, so I went ahead and implemented something for the medium-term. I went conservative because it's easy to loosen later, and I don't see this as being a particular thing that people mix and expect to work anyways so the less tricky the better. At a high level here's what I propose (and have implemented): Disables:
Leaves:
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). I haven't done anything with I haven't added any new directives. It's still In all cases I throw something like:
Note that I'll throw CSP errors before other types of WebAssembly errors. I haven't written tests for exception-order, but we should specify it. Concretely, here's what a test looks like: <script>
const empty = Uint8Array.of(0x0, 0x61, 0x73, 0x6d, 0x1, 0x00, 0x00, 0x00);
</script>
</head>
<body>
<!-- The WebAssembly global object and some of its members aren't blocked. -->
<script>if (typeof WebAssembly !== "object") throw new Error(`Expected WebAssembly object to be accessible under CSP`)</script>
<script>new WebAssembly.CompileError(`This is OK`)</script>
<script>new WebAssembly.LinkError(`This is OK`)</script>
<script>new WebAssembly.Module(empty)</script>
<script>if (!WebAssembly.validate(empty)) throw new Error(`Expected validation to succeed`)</script>
<!-- The following APIs aren't accessible under CSP. -->
<script>new WebAssembly.Memory({ initial: 1 })</script>
<script>new WebAssembly.Memory({ initial: 1, maximum: 1 })</script>
<script>new WebAssembly.Memory({ initial: 1, maximum: 1, shared: true })</script>
<script>new WebAssembly.Table({ element: "anyfunc", initial: 1 })</script>
<script>new WebAssembly.Table({ element: "anyfunc", initial: 1, maximum: 1 })</script>
<script>new WebAssembly.Instance(new WebAssembly.Module(empty))</script> Thoughts? If not I'll send a PR with this. |
https://bugs.webkit.org/show_bug.cgi?id=173892 <rdar://problem/32914613> Reviewed by Daniel Bates. Source/JavaScriptCore: We should disable parts of WebAssembly under Content Security Policy as discussed here: WebAssembly/design#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. This patch disables: - WebAssembly.Instance - WebAssembly.instantiate - WebAssembly.Memory - WebAssembly.Table And leaves: - WebAssembly on the global object - WebAssembly.Module - WebAssembly.compile - WebAssembly.CompileError - WebAssembly.LinkError Nothing because currently unimplmented: - WebAssembly.compileStreaming - WebAssembly.instantiateStreaming 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). I haven't done anything with postMessage, so you can still postMessage a WebAssembly.Module cross-CSP, but you can't instantiate it so it's useless. Because of this I elected to leave WebAssembly.Module and friends available. I haven't added any new directives. It's still unsafe-eval. We can add something else later, but it seems odd to add a WebAssembly as a new capability and tell developers "you should have been using this directive which we just implemented if you wanted to disable WebAssembly which didn't exist when you adopted CSP". So IMO we should keep unsafe-eval as it currently is, add WebAssembly to what it disables, and later consider having two new directives which do each individually or something. In all cases I throw an EvalError *before* other WebAssembly errors would be produced. Note that, as for eval, reporting doesn't work and is tracked by https://webkit.org/b/111869 * runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::JSGlobalObject): * runtime/JSGlobalObject.h: (JSC::JSGlobalObject::webAssemblyEnabled): (JSC::JSGlobalObject::webAssemblyDisabledErrorMessage): (JSC::JSGlobalObject::setWebAssemblyEnabled): * wasm/js/JSWebAssemblyInstance.cpp: (JSC::JSWebAssemblyInstance::create): * wasm/js/JSWebAssemblyMemory.cpp: (JSC::JSWebAssemblyMemory::create): * wasm/js/JSWebAssemblyMemory.h: * wasm/js/JSWebAssemblyTable.cpp: (JSC::JSWebAssemblyTable::create): * wasm/js/WebAssemblyMemoryConstructor.cpp: (JSC::constructJSWebAssemblyMemory): Source/WebCore: This does the basic separation of eval-blocked and WebAssembly-blocked, but currently only blocks neither or both. I think we'll eventually consider allowing one to be blocked but not the other, so this separation makes sense and means that when we want to do the change it'll be tiny. At a minimum we want a different error message, which this patch provides (a lot of the code ties blocking to the error message). 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 * bindings/js/ScriptController.cpp: (WebCore::ScriptController::enableWebAssembly): (WebCore::ScriptController::disableWebAssembly): * bindings/js/ScriptController.h: * bindings/js/WorkerScriptController.cpp: (WebCore::WorkerScriptController::disableWebAssembly): * bindings/js/WorkerScriptController.h: * dom/Document.cpp: (WebCore::Document::disableWebAssembly): * dom/Document.h: * dom/ScriptExecutionContext.h: * page/csp/ContentSecurityPolicy.cpp: (WebCore::ContentSecurityPolicy::didCreateWindowProxy): (WebCore::ContentSecurityPolicy::applyPolicyToScriptExecutionContext): * page/csp/ContentSecurityPolicy.h: * page/csp/ContentSecurityPolicyDirectiveList.cpp: (WebCore::ContentSecurityPolicyDirectiveList::create): * page/csp/ContentSecurityPolicyDirectiveList.h: (WebCore::ContentSecurityPolicyDirectiveList::webAssemblyDisabledErrorMessage): (WebCore::ContentSecurityPolicyDirectiveList::setWebAssemblyDisabledErrorMessage): * workers/WorkerGlobalScope.cpp: (WebCore::WorkerGlobalScope::disableWebAssembly): * workers/WorkerGlobalScope.h: LayoutTests: These tests are basically the same as eval-blocked, but with WebAssembly APIs instead of eval. Disable all of them on iOS simulator which doesn't support WebAssembly (whereas iOS does). * http/tests/security/contentSecurityPolicy/WebAssembly-allowed-expected.txt: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-allowed.html: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-expected.txt: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-iframe-expected.txt: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-iframe.html: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-script-expected.txt: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-script.html: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe-expected.txt: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html: Added. * http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html: Added. * http/tests/security/contentSecurityPolicy/resources/WebAssembly-blocked-in-external-script.js: Added. * platform/ios-simulator/TestExpectations: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218951 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Assuming that the point of
If unsafe-eval was a blanket "no dynamic code" directive, perhaps, but because it's already targeted to JS-isms for loading strings of code and because the risk profile of mixing user content into strings is quite different than for binary, I don't think it's odd to have a separate directive. Since |
I'm happy to implement something else if you write a proposal and we agree on it.
I don't either. I simply think existing directives should also disable WebAssembly somehow. |
It's definitely best that we align CSP policies for WASM across browsers and develop some shared tests to enforce some standardization. The right place for these tests is probably in the webplatform tests (https://github.com/w3c/web-platform-tests) Looks like there is already a content-security-policy subdirectory. @mikewest, does that seem like a good long-term home for WebAssembly-specific policies? |
I generally agree with @lukewagner that a separate directive that basically disables "from-raw-bytes" module instantiation makes sense. That I think accomplishes the integrity aspect of CSP, i.e. to protect against corruption of WASM modules by any malicious user code. Not sure that disabling |
I'm looking forward to a concrete proposal. |
I think that if in the CSP header you have something like script-src 'sha256-076c8.....71db5ad5a2743545f' the browser should not disable compilation and instantiation of the byte array whose sha256 is the one indicated. In this way Web browsers supporting WebAssembly MVP functionality would work correctly with CSP. |
@alpertron Are you suggesting that the WebAssembly engine should hash all WASM module compilations can compare them against hashes put into script tags (at least if CSP is enabled)? At first glance that sounds interesting, but there are a couple of drawbacks:
|
Hashing only has to be done if there is an explicit hash in the CSP header or meta tag, as it is done actually on Javascript. Maybe a benchmark is needed in order to establish whether the hash adds significantly more time to start the execution of the WebAssembly module. |
According to https://www.cryptopp.com/benchmarks.html the library Crypto++ can hash sha256 at 233 MB/sec and sha512 at 343 MB/sec in a modern processor, so I believe that hashing is not a bottleneck. Most time is lost downloading the resource from the Web server. |
I think you might be referring to sub-resource integrity which would also be valuable for WASM to integrate with: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity |
Sub-resource integrity is intended for using resources in other servers to check that they were not changed. In this thread what I am talking about is similar as using Javascript in the HTML page where a hacker can start running code using XSS attacks. So the code needs to be hashed so no random code can be executed. |
Link to MDN Web docs about script-src used on CSP header: |
This was previously discussed in #205, mostly about CSP as applied to JS module integration, where it will fall out automatically, and in #972, which touches on the more immediately pertinent issue of the impact of CSP on the WebAssembly.instantiate/compile APIs.
It seems like currently this is not specified, but Chrome has implemented a restriction that prevents WebAssembly.* APIs from being used if unsafe-eval is disallowed via CSP. The group should work toward interoperable semantics for this.
One thing that has evolved since the discussions in #972 is the compileStreaming/instantiateStreaming APIs. At the time of that discussion, the idea was that some hypothetical "from a URL" APIs would not be blocked by CSP, whereas the existing from-byte-array APIs would be. We ended up adding from-Response APIs, not from-URL APIs, and to me those seem closer to from-byte-array APIs:
So probably any decision made should apply to all current APIs, not just the original byte-array-taking ones.
Approaches for reaching interop here are varied. One path may be for Chrome to continue diverge from the spec and gather data for a while. Alternately, this may be a topic worthy of discussion, either here or at a F2F meeting, to come to a consensus sooner.
Another approach might be waiting until ES module integration is ready, so that CSP-using sites have some way of running wasm code (via
<script type="module" src="code.wasm">
) even with unsafe-eval set, which I believe is not currently possible in Chrome. Maybe non-Chrome vendors would find it more palatable to disable WebAssembly.* APIs if<script src>
tags were able to accomplish the same thing, similar to how unsafe-eval disableseval()
but<script src>
tags still work in that setting.The text was updated successfully, but these errors were encountered: