Skip to content
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

Minimal specification of 'wasm-unsafe-eval' source directive #293

Merged
merged 29 commits into from
Oct 18, 2021

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Feb 16, 2018

This adds the 'wasm-eval' source as an allowed option for the script-source directive.

This is a minimal specification that allows WebAssembly with CSP without requiring 'unsafe-eval' (although 'unsafe-eval' effectively impllies 'wasm-eval' to maintain current behavior).

Implementations are required to gate WebAssembly.instantiate, and may choose to gate new WebAssembly.Module and WebAssembly.compile. This is done to support the existing behavior among browsers.

Note that this does not address WebAssembly.compileStreaming and WebAssembly.instantiateStreaming. The plan is to spec the behavior around these APIs at a later time.


Preview | Diff

@annevk
Copy link
Member

annevk commented Feb 17, 2018

may choose to gate

Why may?

The plan is to spec the behavior around these APIs at a later time.

What's the migration story for sites already using wasm-eval?

@annevk
Copy link
Member

annevk commented Feb 17, 2018

Also, why no unsafe prefix?

@jfbastien
Copy link

A few thoughts:

  • "MAY" seems wrong.
  • I'm not sure we need to care about what browsers currently do. We want to all do the same thing, we just need to agree on what that thing is.
  • Streaming doesn't require CSP, correct? The last paragraph of your PR confuses me. I don't think we want to do anything with streaming+CSP that Web platform streaming doesn't already support.
  • Can you document why we want wasm-eval or however we name it? You're spec'ing a thing, but it's unclear that we want this. I think a case can be made, but you have to break down why it makes sense, and why unsafe-eval should disallow WebAssembly.

@annevk
Copy link
Member

annevk commented Feb 17, 2018

If it didn't disallow WebAssembly, WebAssembly would be introducing a hole in an otherwise "secure" system, no?

@mikesamuel
Copy link
Contributor

@annevk Fyi, WebAssembly/content-security-policy#1 discusses ways to enable WebAssembly without introducing CSP bypasses.

@annevk
Copy link
Member

annevk commented Feb 19, 2018

@mikesamuel I had seen that, but I figured that any insecure outcome would not be accepted anyway. Basically, anything that involves networking or executing code needs to be guarded by CSP, which I think aligns with your and @eholk's understanding.

@mikesamuel
Copy link
Contributor

@annevk Great. Yeah, I think eholk and I are on the same page.

@eholk
Copy link
Contributor Author

eholk commented Feb 20, 2018

@jfbastien -

For your first two bullets, my reason for that was that so far WebKit and Chromium have made different calls on whether WebAssembly.compile should be considered dangerous. You pointed out that WebKit delays actually generating code until instantiation time, so compile is basically a no-op and therefore safe. V8 generates code when WebAssembly.compile is called, so developers may want to actually restrict that step as well. The MAY was to accommodate both choices.

That said, I agree that having the same behavior is better.

I probably didn't state my last paragraph well. The plan is that when using instantiateStreaming/compileStreaming, then this should be allowed when it's given a Response object that came from a URL that's allowed by the script-src directive according to all the normal CSP rules. My point was just that this PR is not addressing that behavior at all.

As far as why we want 'wasm-eval', one use case would be to enable client-side generation of a Wasm module that could then be instantiated and run. Developers may want to allow this without buying into all of unsafe-eval.

@eholk
Copy link
Contributor Author

eholk commented Feb 20, 2018

@annevk -

What's the migration story for sites already using wasm-eval?

I see wasm-eval for WebAssembly.instantiate as orthogonal to how CSP works for WebAssembly.instantiateStreaming. Basically, if developers want to use the non-streaming API, they'll have to add wasm-eval. If they're using the streaming API, then they can use the rest of the CSP source list directives. They don't really need to migrate because they serve different use cases.

@annevk
Copy link
Member

annevk commented Feb 20, 2018

My point was just that this PR is not addressing that behavior at all.

Will there be a separate PR for that? Since it doesn't go through Fetch it's not actually guarded by CSP automatically and you can create synthetic Response objects as well thereby bypassing all of CSP easily at which point it's effectively eval.

@eholk
Copy link
Contributor Author

eholk commented Feb 20, 2018

Will there be a separate PR for that? Since it doesn't go through Fetch it's not actually guarded by CSP automatically and you can create synthetic Response objects as well thereby bypassing all of CSP easily at which point it's effectively eval.

I thought part of the point of the Fetch API and Response objects is that they're meant to provide an unforgeable way to ensure these bytes actually came from a given URL. Is there a way to do this?

As far as having another PR, I'd say yes, if it's needed. It may be that all the desired behaviors fall out of the rest of the Web APIs naturally (I'm not intimately familiar with Fetch and CSP just yet to say for sure).

@annevk
Copy link
Member

annevk commented Feb 20, 2018

@eholk you cannot fake the URL of a Response object, but are you banning Response objects without a URL? And if we ever enable cloning of Response objects you could get one from a context not guarded by CSP, but that's a larger problem with CSP that already exists.

@flagxor
Copy link

flagxor commented Feb 20, 2018

eholk@ I think Response is currently spec'ed in a way that would make it forgeable, but the past assumption has been that we could taint track Response objects to correct this.

Separating out wasm-eval vs how streaming is handled seems to make sense, unless folks want insist, as:

  • There's quite a bit of complexity just in deciding what we disable when Wasm is disallowed in a context. Especially as we have very different behavior in the two browsers that disallow it, and two that allow it currently.
  • The taint tracking required for Response probably needs tweaks to the html spec, so will take more churn.
  • There seems to be lingering issues around what CSP is really trying to accomplish, what's its threat model is, how permeable it is, etc. Reaching clarity around that seems key, as it informs how to incorporate wasm appropriately. (Maybe that's been achieved?)

Other folks here, does mikesamuel's description capture the common consensus on how to think about CSP's threat model?
WebAssembly/content-security-policy#1
I.e. that what unsafe-eval is trying to prevent is a ready made Turing machine sitting about for attackers?

@andypaicu
Copy link
Collaborator

Apologies for the wall of text and the likely typos here and there:

===CSP Thread model===

Regarding the CSP threat model you will probably not get a general consensus. I have only came in contact with it about half a year ago so my opinion might have less weight than some people that have been there on the first draft but this is what I think and I am open to corrections and additions:

CSP was initially designed to give authors control over what will be loaded onto their document. Because the mechanism by which CSP does this is usually request/response blocking (but it's not limited to requests see base-uri for example), it seems to have started to be perceived as a request/response filtering mechanism. This has become so widespread that (due to some other factors as well) we have implemented a prefetch-src (#283) because people were shocked to find that requests can be made that bypass CSP (again this is because the requests don't actually affect the content).

It seems that we are now at a point were people's expectation of CSP is that it's a catch-all mechanism that has as a goal to give you control over every aspect of a document's content AND communication.

===unsafe-eval purpose===

This is even more of a personal opinion then the previous paragraphs but I believe the purpose of unsafe-eval is to allow sites to adopt CSP without having to get rid of eval entirely for the moment. It's supposed to facilitate adoption and help a website adopt CSP while being in the process of eliminating its use of eval. unsafe-eval is supposed to be used only temporarily and in fringe cases when you are in the process of refactoring your site to get rid of eval (whether it is used or not like this I don't know...). unsafe-eval does not prevent anything it does the opposite and in a perfect world it would not exist at all.

===patch feedback===

Regarding this patch specifically:

  • The MAY should definitely be rephrased to clearly explain the situations in which it is allowed to NOT control new WebAssembly.Module() and WebAssembly.compile() while maintaining that the expected behavior should be that it controls these 2 calls as well. I understand that it's a no-op in webkit but I don't believe authors will write pages that they expect to only be ran on webkit browsers so they will include the wasm-eval keyword anyway if they want their code to run. Also I don't know much about WebAssembly but do such calls make any sense without WebAssembly.instantiate() later on in the code? If they don't I don't see any point in carving them out and consistency is key. I would rather we don't make these caveats at all and just have this control all 3 calls and be done with it.
  • I'm could be perhaps convinced otherwise on this one but I think this should have a unsafe- prefix.

You also would need some hook into another spec to actually enforce this. I'm not sure how this would look like for WebAssembly but you can look at the https://w3c.github.io/webappsec-csp/#integrations section for example as how the fetch/html spec calls functions in CSP to determine if an action should be blocked. If WebAssembly already relies on https://tc39.github.io/ecma262/#sec-hostensurecancompilestrings then we already have a hook for unsafe-eval and you just need to update the https://w3c.github.io/webappsec-csp/#can-compile-strings algorithm.

@annevk
Copy link
Member

annevk commented Feb 21, 2018

@flagxor I think we should resolve the Response object question since it's part of the attack surface. The only way you "forge" a Response's URL is with a service worker, but that means the document associated with that service worker was allowed to make a request for that URL by its CSP policy, so that seems fine. Otherwise a Response's URL is always correct. The main problem is if you don't handle Response objects without a URL as those are effectively identical to eval in this case and therefore need to be guarded by it.

@domenic
Copy link
Contributor

domenic commented Feb 21, 2018

FWIW it seems reasonable to me to do this in two steps:

  1. This PR, with "may"s fixed, and with instantiateStreaming/compileStreaming also blocked unless wasm-eval or unsafe-eval is specified.
  2. Work on loosening the restriction for instantiateStreaming/compileStreamining for Responses with the correct URL.

BTW the spec for causing all the WebAssembly.* methods to throw will need to be in the WebAssembly spec, but it should call out to the CSP spec to determine whether or not to throw. See for example:

You'll use something different than HostEnsureCanCompileStrings/EnsureCSPDoesNotBlockStringCompilation; probably HostEnsureCanCompileWebAssembly(callerRealm, calleeRealm) for now, and maybe add a Response parameter for step 2. But this kind of architecture makes sense to me, instead of trying to put normative requirements on Wasm implementations from a non-wasm spec.

The section you're editing in this PR is meant to just be a statement of facts about normative requirements enforced elsewhere in the platform; by itself it doesn't impose any requirements.

@arturjanc
Copy link

What is the status of this PR? Due to the rising popularity of WebAssembly we're seeing applications which want to use WASM but are otherwise free from eval()-like code; under the status quo such applications would need to add 'unsafe-eval' to their policies and reduce the security of their CSP.

Having something like the proposed 'wasm-eval' would help with this problem so it would be good to get this in before the CSP3 CR. (My only bikesheddy bit of feedback would be to make it consistent with other 'unsafe-' keywords, e.g. call it 'unsafe-wasm')

@annevk
Copy link
Member

annevk commented Oct 10, 2018

See WebAssembly/content-security-policy#15 for your bikeshed, which roughly matches mine.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 12, 2021
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 13, 2021
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 13, 2021
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 13, 2021
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}
pull bot pushed a commit to ZSX-JOJO/chromium that referenced this pull request Oct 14, 2021
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 15, 2021
… wasm-eval, a=testonly

Automatic update from web-platform-tests
[CSP] Added new policy violation source: wasm-eval

This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}

--

wpt-commits: 6ccfe6fafab233ee6063b7bfeabb107ad847a205
wpt-pr: 31041
aosmond pushed a commit to aosmond/gecko that referenced this pull request Oct 16, 2021
… wasm-eval, a=testonly

Automatic update from web-platform-tests
[CSP] Added new policy violation source: wasm-eval

This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}

--

wpt-commits: 6ccfe6fafab233ee6063b7bfeabb107ad847a205
wpt-pr: 31041
@antosart
Copy link
Member

Thanks. This is now fully tested in WPT. I believe it is fine to merge now.

@antosart antosart merged commit 71d8b45 into w3c:main Oct 18, 2021
github-actions bot added a commit that referenced this pull request Oct 18, 2021
SHA: 71d8b45
Reason: push, by @antosart

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@fgmccabe
Copy link
Contributor

fgmccabe commented Oct 18, 2021 via email

@annevk
Copy link
Member

annevk commented Oct 18, 2021

Nice work @fgmccabe!

@boompig
Copy link

boompig commented Nov 16, 2021

Is the current status of this proposal that wasm-unsafe-eval has been accepted as part of the spec for CSP? And right now am I correct in understanding that it's behind a feature flag in Chrome? If that's right, what's the best way to track this proposal and determine when it will hit mainline Chrome?

@fgmccabe
Copy link
Contributor

Yes to the first part of your question.
And, yes too to the second part.
It is currently scheduled to land fully in chrome M97.

@ostap0207
Copy link

And, yes too to the second part.

How to enable it via feature flags? Cannot find it in chrome://flags/ in Chrome 96.

Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}
@fgmccabe
Copy link
Contributor

I am not certain. However, I believe that
"Navigate to about:flags in the URL bar and turn on “Enable experimental web platform features”"
may work.

@fgmccabe fgmccabe deleted the wasm-eval branch November 22, 2021 22:21
@ericlaw1979
Copy link

RE #14: For Chrome/Edge 96, you must use chrome://flags to enable experimental web platform features. For Chrome/Edge 97 (going to stable the first week of January), the feature is enabled by default.

Commit 1562a148... initially landed in 97.0.4675.0

@fosskers
Copy link

For anyone trying this in a Chrome extension with Chrome 97+: you need to set wasm-eval (even though the spec has outlined wasm-unsafe-eval).

@firstdorsal
Copy link

why is script-src: 'self' not sufficient for wasm?

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This extends the suite of policy violation sources to include
a WebAssembly specific source: wasm-eval.

This has also been reflected in the PR
(w3c/webappsec-csp#293 (review))
against the CSP spec.

Added test for proper security violation event of the right form.

Bug: 948834
Change-Id: I0b76fd725136b7ddda92e629f147f5ba77c50ffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197842
Commit-Queue: Francis McCabe <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Antonio Sartori <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Reviewed-by: David Tseng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931206}
NOKEYCHECK=True
GitOrigin-RevId: d476b181f81d3ab5926ae76cd051bdc989a692a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.