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

Add hooks for CSP #13

Merged
merged 4 commits into from
Apr 16, 2021
Merged

Add hooks for CSP #13

merged 4 commits into from
Apr 16, 2021

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Mar 9, 2018

This PR adds HostEnsureCanCompileWasmBytes and HostEnsureCanCompileWasmResponse as hooks that allow CSP to enforce its policy, similar to HostEnsureCanCompileStrings in the ECMA-262 spec.

Implementations of these abstract operations will be implemented by the CSP spec. See w3c/webappsec-csp#293 for an in-progress PR.

@eholk eholk requested a review from littledan March 9, 2018 00:13
@jfbastien
Copy link
Member

I'd have the WebAssembly spec spell its own name out fully :)

@littledan
Copy link
Contributor

I can picture some different editorial choices here (e.g., using an HTML-style wording-based hook rather than a ES-style abstract operation), but I'm happy with the decisions here personally.

Is there cross-browser consensus on these semantics? In particular, does anyone want to any sort of CSP checks when constructing an instance from an already-compiled module?

Is there a PR I can review for how these hooks are implemented by another specification?

Otherwise LGTM.

@jfbastien
Copy link
Member

Is there cross-browser consensus on these semantics?

No.

@littledan
Copy link
Contributor

OK, I'd suggest waiting to land this patch until we can work out the differences.

@jfbastien
Copy link
Member

I'm OK for this patch moving forward without consensus, since it's an outstanding proposal repository. I think we can iterate here in this overall direction, and reach consensus separately with webappsec people's input.

@annevk
Copy link
Member

annevk commented Mar 12, 2018

Is the disagreement documented somewhere?

@jfbastien
Copy link
Member

@annevk our implementations diverge significantly, and we haven't agreed on where to converge. WebAssembly/design#1092 has some fo the discussion. This proposal tries to get to a point where we agree, but I don't feel like it's at a point where we can evaluate a full picture that we'd agree on.

I'm happy moving forward as Erik proposes here, and later discussing a full proposal and try to agree on it.

@littledan
Copy link
Contributor

Maybe we should have a higher level conversation about what goes in the specification and how we want to go about resolving compatibility issues for WebAssembly between implementations. In my mind, the goal of writing a specification for a feature that already shipped would be making implementations compatible; I'd like to understand others' perspectives. This is coming up in a number of places at once.

@binji
Copy link
Member

binji commented Jun 30, 2020

@littledan I'm looking to try and move this proposal forward. Some progress was made on alignment after this PR, see for example #16. It seems that the main disagreement was whether instantiation needs to be CSP-gated in addition to compilation (or is there more disagreement that I'm missing?)

@littledan
Copy link
Contributor

This PR still looks reasonable to me editorially.

Is there a matching definition of these hooks somewhere (e.g., in the Web API spec + the CSP spec)?

Do we know if WebKit is OK with these semantics, given the explanations made in #16? @saambarati @kmiller68

@kmiller68
Copy link

If we all agree that compilation rather than instantiation is the right place to block WASM that's fine. I'd still prefer to do the check at instantiation in case a module "leaks" across origins but I'm not going to fight it.

To double check, it's semantically unobservable to check at both instantiation and compilation? If that's not the case then I want to say we should require checks in both places. That's probably what JSC is going to want to implement given the spec doesn't check at instantiation only.

@littledan
Copy link
Contributor

Is anyone aware of a downside to checking in both places in the spec? I am not sure it is completely unobservable.

@annevk
Copy link
Member

annevk commented Jul 8, 2020

@kmiller68 you mean same-site-cross-origin or something else? Why is CSP the right place to enforce that?

@binji
Copy link
Member

binji commented Jul 16, 2020

Is anyone aware of a downside to checking in both places in the spec? I am not sure it is completely unobservable.

I'm guessing a CSP check is relatively small compared to the cost of instantiation, so if we need to check at compilation and instantiation, it's probably OK from that perspective.

OTOH, I don't really see how a Wasm module could "leak" into an environment with stricter CSP requirements, but that may just be my ignorance w.r.t. CSP works.

@jfbastien
Copy link
Member

IIRC, we'd discussed whether one should be able to postMessage a Module (or something similar) between parts of a page with different capabilities. CSP was one example, but this was in a wider context of "what if some code is allowed to compile but not instantiate, and another is allowed to instantiate but not compile, should it be possible for one to compiler then had off to the other for instantiation", as well as other combinations of capabilities.

@annevk
Copy link
Member

annevk commented Jul 20, 2020

Since postMessage()'s security-design is by-and-large object-based capabilities and the receiver is in control it's not clear to me how it matters that the sender and receiver might have different policies. And note that CSP isn't the only policy in play here, there's also referrer policy, permissions policy, etc. It's very likely those will differ, also between a document and a worker, which cannot have all of the same policies. (Also, Module is restricted to an agent cluster (at most a site, but if you want to use shared memory an origin) so you're already pretty limited in what you can do.)

@binji
Copy link
Member

binji commented Jul 27, 2020

@kmiller68 does JSC still generate code during instantiation? Is that part of the concern here?

@annevk

Since postMessage()'s security-design is by-and-large object-based capabilities and the receiver is in control it's not clear to me how it matters that the sender and receiver might have different policies.

Do you mean that a CSP policy check would be done when the Module object is received, or am I misunderstanding?

@annevk
Copy link
Member

annevk commented Aug 4, 2020

The opposite, I don't think it makes sense to do policy checks around messaging. I suppose you could limit object creation and that in turn would influence what can be messaged, but you might be able to work around that as well if there are inconsistent policies across documents.

pull bot pushed a commit to Mu-L/chromium that referenced this pull request Oct 20, 2020
Piex is the "Preview Image EXtractor" and is used by the Files app on
ChromeOS for creating thumbnails inside the "image loader" component
extension. It uses the `wasm-eval` CSP directive, which is only
currently enabled for apps and extensions (standardisation being
discussed at github.com/WebAssembly/content-security-policy/pull/13 ).

To make the WASM work, this CL enables `wasm-eval` for
chrome-untrusted://* CSP directives, and adds that to
chrome-untrusted://media-app's CSP.

Two new JS files are added to chrome-untrusted://media-app:
 - piex_module.js is a thin wrapper around the existing piex_loader.js
   to do (effectively) RAW -> JPEG conversions of Blob data.
 - piex_module_loader.js adds logic for runtime-loading of the
   WebAssembly, piex_loader.js, and piex_module.js

An end-to-end test using the handcrafted "raw.orf" file is added to
ensure the WASM loads and successfully extracts preview data, and it
is successfully loaded by the app.

Note this CL adds support, but doesn't change file handlers to direct
RAW files to chrome://media-app, which will be gated behind a flag.

Design Doc: go/bl-raw
Launch Bug: https://crbug.com/1126203

Bug: 1030935, b/154062029, b/167496867
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: I0c1f1da8bc39c8e6223214b49d9ac09dc93bf420
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422100
Reviewed-by: dpapad <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Reviewed-by: Noel Gordon <[email protected]>
Reviewed-by: Patti <[email protected]>
Commit-Queue: Noel Gordon <[email protected]>
Auto-Submit: Trent Apted <[email protected]>
Cr-Commit-Position: refs/heads/master@{#818636}
@fgmccabe
Copy link
Collaborator

Since this PR has been waiting in the wings for so long, but no actual action taken, I propose to merge this PR in the next 72 hours.

@fgmccabe fgmccabe merged commit 7b1980b into WebAssembly:master Apr 16, 2021
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.

7 participants