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

Summarize API behavior and threats relative to CSP #16

Merged
merged 5 commits into from
Nov 22, 2018
Merged

Conversation

titzer
Copy link
Contributor

@titzer titzer commented Sep 13, 2018

Forked and rebased, integrating comments from #10 .

@titzer
Copy link
Contributor Author

titzer commented Sep 24, 2018

@jfbastien PTAL

@jfbastien
Copy link
Member

@saambarati and @kmiller68 will review.

@titzer
Copy link
Contributor Author

titzer commented Oct 2, 2018

@saambarati and @kmiller68 friendly ping. I've integrated comments from the previous PR, and if there aren't any further issues, I'll merge this one.

Protecting the JavaScript-supplied imports to a WebAssembly module is orthogonal
to CSP directives for WebAssembly.
Thus instantiating `WebAssembly.Module` objects need not be subject to CSP
restrictions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. First you say it needs to be subject to restrictions and then not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see that my terminology might introduce confusion. In the first paragraph it is "construction" and in the second it is "instantiation". These are indeed different operations, and the latter is safe while the former is not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I totally buy this. I think you also need to handle post messaging here? If the browser only checks CSP when compiling then what stops someone from post messaging a module from an iframe with less restrictive CSP?

Based on the threat model CSP is trying to protect against, I think it's necessary to either check CSP at compilation/post message time or at instantiation time. I would argue that it's probably prudent to do both though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's a good idea. As I argued in various issues (and as has been the model to date, see the object-capability note in the HTML Standard) we should let objects carry their own authority.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TPAC agenda doc is here but not filled out yet. I've been organizing the topics here. Our meeting days are Thursday and Friday, the 25th and 26th.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I'm personally most flexible Thursday afternoon, so that'd be ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, CSP is on the agenda for the TPAC now. I'll make a presentation over CSP and we can have a discussion there.

In the meantime, is there a version of the text in this PR that we can land to make the proposal a little more unified? Happy to make changes to this text here to make it landable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To be clear, I agree with the PR as-is, I think @kmiller68 does not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think the discussion about postMessage() belongs on #9, so I'd like to make this PR landable.

@titzer
Copy link
Contributor Author

titzer commented Oct 29, 2018

@kmiller68 PTAL in light of recent in-person CG discussion.

@kmiller68
Copy link

Yeah, I'll try to talk to the appropriate peoples when I get to the office today.

@kmiller68
Copy link

CC, @dbates-wk.

I think the case people on WebKit are worried about is that enough frameworks might use Wasm as an escape hatch for eval that it should still be checked on instantiate. For example, example.com could include a framework that has some internal debugging functionality where it adds a post message handler that accepts any wasm code and passes it to instantiate. Additionally, if the framework provides the user agent and document.cookie as imports then example.com is still vulnerable if they haven't fully vetted their use of this framework.

I believe a similar thing was done JQuery (could have been a different framework) where it would prepend some information to document.location then eval whatever it loaded as a debugging convenience. As long as a framework used wasm for a similar purpose instead of eval you'd be in an equally bad spot.

Apologies, I remember @annevk gave me an example of something like the example above that can still be done even with CSP. Alas, I think I forgot the example, although, I recall it had something to do with postMessaging a blob as a URL. Does that example do something morally equivalent to the one above? If so, then I think we would probably withdraw our concerns.

@annevk
Copy link
Member

annevk commented Nov 1, 2018

That sounds like a different case (and my (unrelated, as far as I can tell) example might not have worked necessarily due to special handling of local schemes in CSP, though I haven't tried to verify). Per https://github.com/WebAssembly/content-security-policy/blob/master/proposals/CSP.md#proposed-homogenization-of-existing-behavior WebAssembly.instantiate(BufferSource, ...) is guarded by unsafe-wasm-eval, so I think that's already good as proposed?

@kmiller68
Copy link

kmiller68 commented Nov 2, 2018

@annevk Are you talking about my example or yours? My example would be under the WebAssembly.instantiate(WebAssembly.Module, ...) case, which is always considered safe right now. The framework would be receiving a postMessage'd module object from potentially any iframe and blindly accepting it. It's also especially tricky because the WebAssembly.instantiate method is overloaded so the framework author may have only intended to receive a BufferSource but failed to weed out WebAssembly.Module.

@annevk
Copy link
Member

annevk commented Nov 5, 2018

@kmiller68 it seems to me that BufferSource would always be strictly less safe, so I'm not sure about your final remark there. The reason we decided that the other scenario didn't matter is because Module objects are same-site restricted anyway due to agent clusters and same-site documents can already have synchronous access to each others' globals in a bunch of cases (in particular due to document.domain).

@titzer
Copy link
Contributor Author

titzer commented Nov 12, 2018

I agree with @annevk. Wasm modules cannot be postMessage()'d to another origin; the only way to get a module from another origin is through a foreign fetch which already has CSP applied to it.

@titzer
Copy link
Contributor Author

titzer commented Nov 12, 2018

Also, I'd like to emphasize that there is nothing unsafe about instantiate() of a module if one already has a reference to it[1], as per discussion at TPAC.

[1] The API has overloads of instantiate() that deal with BufferSource and Response, but these can just be considered a compile() followed by an immediate instantiate(); CSP is really only applicable to the compile() step, which is the step by which new executable code is created.

@saambarati
Copy link

Why can’t modules be postMessaged to a different origin? Is this specified behavior? If so, why do we have such a restriction?

@binji
Copy link
Member

binji commented Nov 20, 2018

@saambarati yes, it's specified in the wasm web spec that a module can only be postMessaged to another agent in the same agent cluster. How this maps to the HTML spec is defined here. This matches the way sharing works with SharedArrayBuffer.

@titzer
Copy link
Contributor Author

titzer commented Nov 22, 2018

Ok, I feel like this PR is getting a little long in the tooth, so to unblock I'd like to land it as is and then we can iterate if more information emerges.

@titzer titzer merged commit 57b7b52 into master Nov 22, 2018
@binji binji deleted the threat-summary branch November 23, 2018 04:59
@binji binji mentioned this pull request Jun 30, 2020
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