MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.94k stars 4.88k forks source link

Can Component.utils replace current injection strategy? #813

Closed danfinlay closed 4 years ago

danfinlay commented 7 years ago

Right now Firefox supports an API for an extension to inject an API into web pages (sound familiar?) called Component.utils.exportFunction or its sister Component.utils.cloneInto.

This would presumably allow MetaMask with much safer permissions to guard against the possibility that an attacker gained the ability to publish a malicious version of MetaMask, and wanted to read & write confidential information from ordinary websites.

I've been meaning to open this issue for a while, this can serve as a place to continue discussing this goal.

danfinlay commented 7 years ago

Does anyone know more about how to campaign for wider browser adoption of a standard?

danfinlay commented 7 years ago

From the exportFunction docs, it looks like it really exports just a single function at a time, but we would need to export a whole object. Need to investigate if it's possible to do this with exportFunction, or if maybe we could push to get a similar API added?

danfinlay commented 7 years ago

Looks like Component.utils.cloneInto is actually what we want, since we want to inject a whole API object!

danfinlay commented 7 years ago

Since the injected object is isolated, the APIs injected have to be cloneable, which means creating a lot of new instances of things to inject, apparently this can be tricky, but is part of the security process.

An example of a cloneInto usage is here.

rhashimoto commented 7 years ago

It isn't clear to me how this would accomplish the goal of eliminating the global content script. These APIs appear to be for content scripts to export objects into the injected page (the linked example of cloneInto is from a content script). Can you make such a call without injecting a content script in the first place?

danfinlay commented 7 years ago

A content script is not injected, we currently use a content script to write another script into the DOM which creates a web3 instance that communicates through our background process.

If we used cloneInto to expose web3 to the javascript context, we would not need permission to read & write to the DOM, we would simply inject an object that is available to websites, creating a seemingly very minimal attack surface for our use case.

kumavis commented 7 years ago

@flyswatter I think @rhashimoto's concern is that the presence and permission requirements of the contentscript are a privacy and security concern -- you can still read the DOM from a contentscript.

that said, it looks like you can use cloneInto from the background page https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.cloneInto

danfinlay commented 7 years ago

Ah, thanks for clarifying.

rpl commented 7 years ago

@kumavis @flyswatter in a WebExtension addon cloneInto is something that is only available from content scripts (the above doc page is related to the same feature but in the context of what you can access from the Firefox sources itself, and legacy addon technologies, there is no Components.utils available in WebExtensions pages).

There is also another important reason that makes "injecting an API object in a webpage" something that has to be done from a content script: the background page has no direct access to the DOM of a webpage running in a tab, and when "multiprocess Firefox" is enabled the background page is actually running in a different process from the one where the tab is running on.

As an additional side note, besides injecting a content script using the content_script manifest field, it is possible to inject a content script programmatically from a background page (or a popup page etc.) using browser.tabs.executeScript.

kumavis commented 7 years ago

@rpl So our holy grail is an API that lets an extension add an api object to a windowGlobal that sends messages to the background, without having to give the extension permission to read the DOM. This would allow us to add browser functionality while preserving privacy guarantees to the user.

rpl commented 7 years ago

@kumavis I see your point, and it sounds reasonable to me.

it also looks pretty similar to what "externally_connectable from a webpage" provides on chrome, but limited to the basic messaging APIs (e.g. runtime.sendMessage and runtime.connect), would that be enough for your use cases? are there in your use cases any scenarios for which it wouldn't be enough?

kumavis commented 7 years ago

@rpl AFAIK "externally_connectable from a webpage" requires each domain to be whitelisted. This would be for all pages.

rpl commented 7 years ago

@kumavis @flyswatter I filed on bugzilla Bug 1319168 - Implement externally_connectable from a website to track the "externally_connectable from a webpage" feature (and I also mentioned there the additional "externally_connectable from any webpage" scenario).

danfinlay commented 7 years ago

That's a really fantastic discussion you've started, @rpl, thank you! You seem to have a solid grasp of our needs!

Worth noting:

You've mentioned a concern about whether other scripts/addons will be able to access our injected stream, and while this is a good question for security reasons, it's actually beyond our needs as MetaMask developers.

The API we are injecting exists exactly so that websites will have access to it, and be able to send messages to it, and so it is fundamental to our own architecture that we do not allow this message channel to return private information, or initiate secure operations.

When websites request secure methods via this messaging system, we currently open a new window to get user confirmation, so we actually consider our own authorization layer to be its own security measure.

The nice thing here, is that our security is only responsible for our own secret information. A website can hammer our extension, and we are responsible to keep it secure, but our extension is not able to read/write to the website, which makes installing it much lower general risk for users.

Worst case scenario for users would be that we fail our basic job of securing our own information, but never that we compromise their whole browser.

rhashimoto commented 7 years ago

@flyswatter Would a wildcard externally_connectable really make a meaningful difference for MetaMask? Its use seems to require "opt-in" by web page Dapps - i.e. it doesn't appear to advance your goal of injecting a web3 global.

The advantage of wildcard externally_connectable versus relaying via an <iframe> is direct communication with the extension, so it is a little simpler to code and should be faster. On the other hand, relaying works now (and whether messaging performance matters to MetaMask seems doubtful). If you're going to pursue an "opt-in" mechanism eventually, doesn't it make sense to do it earlier than later? Plus, if you abstract the mechanism with a script element, you can always change it out without disruption if/when a superior mechanism lands.

I understood and respected your previous decision not to pursue "opt-in" mechanisms but the revised understanding of cloneInto and your promotion of what appears to be a pretty similar "opt-in" mechanism makes me wonder if you would reconsider.

danfinlay commented 7 years ago

@rhashimoto I think you've misunderstood. For the reasons I've stated before, we're not interested in forcing Dapps to perform special procedures to be MetaMask compatible. We are exploring other APIs, but they may require revision to meet our requirements. Our investigation into any given API does not represent our endorsement of all of the implications of adopting that API in its current form.

kumavis commented 7 years ago

@rhashimoto we're experimenting with what we hope one day (perhaps outlandishly) will be a standard browser API. Currently the standard is shared by the Mist browser and MetaMask.

We'd like the early-adopter developers to use it as a standard, as opposed to a library, in order to maintain interoperability with Mist and MetaMask.

rpl commented 7 years ago

That's a really fantastic discussion you've started, @rpl, thank you! You seem to have a solid grasp of our needs!

@flyswatter Thanks for having took a look to the above bugzilla issue, I immediately linked that bugzilla issue in the above comment to be sure that the important pieces of our discussion here has been included there (and the proposal and its reasons are correctly described), so that we can take them into account during its discussion and implementation.

You've mentioned a concern about whether other scripts/addons will be able to access our injected stream, and while this is a good question for security reasons, it's actually beyond our needs as MetaMask developers.

@flyswatter yep, I added the "security concerns" part in a separate comment and in form of questions so that it is more explicit that is to be considered as a starting point for a discussion related to the security-related concerns for the general use case provided (externally_connectable from a specific url pattern).

Part of them are probably not an issue for the proposed additional behavior (externally_connectable from every website), but it still possible that "preventing a different add-on from being to use an API object that was supposed to be provided to the webpage" can be necessary in this scenario (but given that it is just the starting point of a discussion, I don't exclude that, once discussed, we end up to agree on a different approach or we may notice that it doesn't really matter).

danfinlay commented 7 years ago

Ok cool, thanks for verifying we're on the same page.

danfinlay commented 7 years ago

@rpl actually, I'd like to add to your last point.

I think MetaMask actually likely wouldn't mind at all if other extensions could access the API that we inject in the current webpage. It is designed to be passed to an untrusted party.

We might even prefer to be the first thing injected, or for there to be an injecting round for all extensions, so that we could ensure our API was available to other extensions, because injection of Ethereum’s web3 API could allow a variety of other extensions, once installed.

An example of an extension that could be possible by interfacing with MetaMask would be a subscription-based website-visiting tip-system like the Brave browser is implementing, where you could divide a given subscription price between the sites you visit each month. You’d be able to blacklist sites to not tip, but otherwise tip sites you’ve visited according to the visits, all proven with zero-knowledge proofs.

Anyways, that’s one example, but the point is, from our perspective, other extensions are valid consumers of our injected API. As long as our background process is isolated to only communicate with that object, and not any direct connection to our background process we are happily enforcing the permissions we desire.

MicahZoltu commented 7 years ago

Related to this discussion, I created a Chromium issue requesting that externally_connectable be opened up to full wildcard domains: https://bugs.chromium.org/p/chromium/issues/detail?id=679238

At the least, I'm hoping the Chromium maintainers explain their reasoning behind allowing only third level domain wildcards since I am of the belief that this constraint is a massive security hole in Chrome's extensions.

danfinlay commented 6 years ago

I see that the externally_connectable: "*" from websites permission was merged into Firefox on June 20th! Looks like it's time to revisit this!

MicahZoltu commented 4 years ago

Error from Firefox when using Components from a content script:

The Components object is deprecated. It will soon be removed.

danfinlay commented 4 years ago

Well I guess that rules out this issue.