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

Limiting to http/https is limiting #178

Closed
martinthomson opened this issue Sep 22, 2020 · 5 comments · Fixed by #244
Closed

Limiting to http/https is limiting #178

martinthomson opened this issue Sep 22, 2020 · 5 comments · Fixed by #244
Assignees

Comments

@martinthomson
Copy link
Member

martinthomson commented Sep 22, 2020

#173 was a bit of a learning experience. The fix for that in #174 limits URLs to http or https. That was the most conservative reaction to that issue.

This means that a number of URI schemes with no known exposure to this vulnerability can no longer be shared. From standard protocols mailto, sip[s], and tel are all examples of schemes that do not invoke actions and should be safe to share. (Why you might use those rather than share a vCard resource is not relevant; these seem valid to me.) Then there are the quasi-standard things like: ipfs, magnet. And a whole bunch of proprietary schemes: acrobat, zoommtg, steam, microsoft-edge (ok, maybe we don't want that...), and so forth.

Now that the dust has settled, it might be worth examining principles a little closer to determine whether a looser set of constraints can be made to work. @dveditz suggested that we block URLs if we might not permit both navigation or redirection. I think that is a reasonable starting point here. We don't allow navigation or redirects to file:// and so sharing that seems to be primarily a means of circumventing that policy.

We can't expect that share targets will respect the same policies that a browser does in case it follows an HTTP URL that redirects to file:///, but we are explicitly accepting that risk already by allowing use of HTTP schemes.

@marcoscaceres
Copy link
Member

The above sounds good. I'd like to get #177 (canShare() method) landed first and get all UAs passing the associated tests. Then we should be in a good position to reconsider what we did in #173.

@tomayac
Copy link
Contributor

tomayac commented Sep 22, 2020

Maybe we need to mint a new absolute and relative URL string that excludes file:, but includes everything else what's currently listed for the allowed value of url.

@martinthomson
Copy link
Member Author

Excluding file: is not sufficient. The patch I'm in the process of landing (@marcoscaceres, details on request) blocks blob, javascript, chrome, about, wss, and data. These are things that Firefox knows about and would not permit a top-level navigation to, as I originally stated. If we find more of these and teach Firefox not to navigate to them, those would be blocked by virtue of that.

Working out what the right primitive is here isn't cheap. This is something that likely touches the HTML or infra specs if we do this right. But the advantage is that we are resting this on principles and a shared understanding. The ideal state is that this doesn't rely on just one implementation finding these problems, but everyone can benefit. Including apps that fetch resources and share them, because responsibility for this problem is a shared burden.

Enough pontification though. Marcos has the right approach.

@marcoscaceres marcoscaceres added the security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. label Aug 17, 2021
@marcoscaceres
Copy link
Member

Emailed public-webappsec for further input on this.

@marcoscaceres
Copy link
Member

marcoscaceres commented Aug 24, 2021

So, from the emails to WebAppSec, I think what we could do is:

  • Disallow sharing to "local scheme" ("about", "blob", or "data"), file, and "javascript", and any other scheme the UA doesn't want to share (e.g., internal "moz-icon:" or whatever).
  • Allow sharing HTTP(S) scheme and, optionally, any of the safe-listed schemes.

That should give us broad coverage, while allowing the UA to retain control over what's actually shared, while excluding the "bad ones".

@marcoscaceres marcoscaceres removed the security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. label Nov 8, 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 a pull request may close this issue.

3 participants