Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Do not provide "origin" to plugins when invoking their registered RPC methods without permission #30

Open
wighawag opened this issue Sep 26, 2019 · 12 comments

Comments

@wighawag
Copy link

hey great initiative and great work with the plugin system
That's really cool to have that to experiment new things!

The idea I would like to implement is an encryption/ decryption api

For that I want to get access to the origin of the app requesting encryption/decryption (the app using the plugin)
This is so that encryption is sandboxed to the origin of the app, and not of the plugin (and that is why I don't want to use getAppKey only). This is basically an implementation of what I described here for safe automatic encryption/decryption : https://medium.com/@wighawag/3-proposals-for-making-web3-a-better-experience-974f97765700

For Metamask to preserve privacy though, I should not be allowed to use fetch as this would allow me to track what apps / website the users of the plugin are using.

In other words, access to fetch should be a permission.

Few added thoughts:

Maybe the 'fetch' permission can be granted by default if no other privacy-breaching permission is given.
But I think the simplest is to simply make fetch always an explicit permission

Note that there is an alternative solution to get an encryption key per app's origin without revealing the origin to the plugin:
It would be to have another wallet api : wallet.getAppKeyForCurrentBrowsingContext
But I would argue that is still privacy-breaching as it would allow the plugin to track how many time the user is using that context.

@danfinlay
Copy link
Collaborator

Hey @wighawag, thanks for the feedback, and congrats on being the first external contributor!

Re: Fetch API: Yes, we will absolutely make this and basically all globals into restircted methods, we intend to make the plugin environment as restrictive as possible by default, and only exposed it for now because its API is complicated and we wanted to start the beta quickly.

As for wanting the origin of the requesting site, this should already be possible today, using:

wallet.registerRpcMessageHandler(async (originString, requestObject) => {
   console.log('requesting wallet is ' + originString)
})

Any reason that isn't enough?

@wighawag
Copy link
Author

Oh, yes indeed, sorry I should have read more assiduously. I guess I was not expecting to have access to the origin via this method.
Do you think all use case for registerRpcMessageHandler will need to access the origin? If not it should be a separate permission. But I guess as you mentioned this is a rough API.
I am just sharing my thoughts as they come along :)

Anyway, I hope I ll find some time to experiment with it soon.
Thanks

@danfinlay
Copy link
Collaborator

I don't think the origin detection needs to be a permission, as the requesting origin is only visible when it initiated a request, which is only possible after the domain explicitly requested permission to interact with the plugin, and the user consented to it.

I can leave this issue open for the fetch API issue, but really "remove all global authority" is already an important epic on its own, and I'm happy to just open that as an issue that would be closed before a version of this would ever reach production.

@wighawag
Copy link
Author

wighawag commented Sep 29, 2019

The user is consenting to the use of a plugin code at origin <plugin's origin> for the current application at origin <application's origin>
This to me (and probably many users) does not mean that the user is granting the plugin access to know what application the user is currently using.
You can imagine plugins that do not need to be aware of the application's origin to be useful and as such a plugin should not have access to the origin of the application, unless it is clearly indicated to the user.

@danfinlay
Copy link
Collaborator

Two possible ways to address that concern:

  • We add specificity to the permission request, something like saying "the plugin will be aware when it is interacted with"
  • We allow dapps to call methods anonymously, which will not provide access to a domain string.

That first one feels a little clunky/awkward, would be happy for something a little more elgant.

@wighawag
Copy link
Author

Yes, I agree but we can have both:

The default is anonymous and if the plugin want to know the origin, like the plugin I want to build, it can request origin access

But if fetch is not requested for that particular plugin, a permission request for origin access should not scare the user away.

Indeed, I think while permissions should be as granular as possible, they benefit in being requested in batch so different set of permissions can result in different display, some warning the user more than other.

For example, a combination of fetch and origin access have great privacy risk, while origin alone is pretty harmless

@danfinlay
Copy link
Collaborator

We now have #38 for addressing the "restrict fetch" (and other globals), so I'm going to keep this issue open primarily to discuss when/how to obscure the origin from plugins.

Do you think hiding the origin should be on a per-method basis, or should it be a blanket, like "permission to know the domain of sites and plugins that connect to it"?

@danfinlay danfinlay changed the title add origin permission + make fetch a permission Do not provide "origin" to plugins when invoking their registered RPC methods without permission Oct 1, 2019
@wighawag
Copy link
Author

wighawag commented Oct 2, 2019

I think it should be a blanket one, since having access to the origin by one or several method results in the same potentially privacy risk.

But as I mentioned, a plugin that only access origin and has not fetch access, should not require the same level of user's emotional response if both origin and fetch is given.

I would even go as far as having origin be given by default if there is no way for the plugin to leak it away. But that would depends on what other api the permission system will allow. If there were something else than fetch that could leak info, then origin would need to be given permission

@danfinlay
Copy link
Collaborator

I would even go as far as having origin be given by default if there is no way for the plugin to leak it away.

This sounds like a pretty elegant solution to me, and is very close to how the system operates today:

If we just make network-access permission wordings scary (be they fetch, websockets, or grouped as a single type of permission), we could group together that entire type of risk under a single block of warning.

We could then also naturally provide the ability to specify restrictions on the requestor, per EIP 2255, so a site might say "I only want to access MY_DOMAIN.com".

@wighawag
Copy link
Author

wighawag commented Oct 4, 2019

But one thing I though as part of this issue is that if the only permission requested is fetch and origin (or any other privacy specific access) is never accessed, then the risk of fetch to privacy is non-existant.

That's why I think while grannular permission definition is a must, the actual request of permissions should always be a batch request. So wallet can give proper warning to specific combination.

This is because a combination of permissions can have bigger impact that each of them individually.

@danfinlay
Copy link
Collaborator

Fun find: at 23 minutes into this talk from 2002, Mark Miller suggests the same thing as you (add a special warning if gaining both disk and network access):
https://youtu.be/KoM_aCuFk1w

@wighawag
Copy link
Author

Cool :)
It is crazy how much we need to re-learn/re-discover, no matter how much was already done.
I am really happy to know you are digging all that info :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants