[SNAPSHOT] Plugin Beta Changes #1

lilyannehall · 2019-10-29T16:54:22Z

This PR is a snapshot of the public PR at MetaMask/metamask-snaps-beta#77 for the purpose of private security review.

Originally from @danfinlay:

For reviewing relative to the LoginPerSite branch.

Please note many of these changes are totally experimental and intended for removal, such as:

all default "ambient" APIs (like Fetch and XMLHttpRequest), and so this diff should be compared with an eye of whether the overall approach of isolating plugins and managing the way they communicate with sites and each other is sound.

Included plugin APIs are also each subject to individual consideration before going to production, and are in early stages of implementation. Currently those are:

The auditAddress API.
The manageAsset API.
The registerRpcMethodHandler API.
Capnode integration via registerApiRequestHandler api.
(still in progress, probably beyond audit scope) Custom account type API.

You can view known production blockers here.

As always, the wiki is a great place to start to understand the feature and its general API.

remove provider approval controller integrate json-rpc-capabilities-middleware create PermissionsController move provider approval functionality to permissions controller add permissions approval ui permission requests still atomic but with multiple permissions permission approval UI added, still needs account permission logic rewrite provider.enable to use permissions controller provider-approval controller workflow runs without prompt after user approves permissions refactor clear approved origins to clear permissions move createStandardProvider to metamask-inpage-provider deprecate synchronous inpage methods remove getSiteMetadata; use site metadata now appended to requests by metamask-inpage-provider eth_requestAccounts -> eth_accounts; install latest json-rpc-capabilities-middleware eth_requestAccounts now handled by the inpage provider

basic wait-on-unlock implementatation in permissions controller

add checkbox selection to permission approval page begin add permissions settings page; edit messages revert isUnlocked behavior for now; it is managed both by the permissions and keyring controllers

ignore circleci e2e and coveralls; lint

show "connected" account in popup

…ailable to plugins

…le to plugins

…d plugin request UI

…_ methods

Plugin system enhancements

…en approved

Persist and restart plugins

…ollers available to plugins

Metamask events

To allow passing the origin to restrictedMethods

type rpcMessageHandler = (origin:string, request:Object) => Promise<any>

Allow plugins to handle RPC requests

…s without metamask_ prefix

Make `metamask_` prefix optional

Adds description for the getPluginState permission

Adds getTxById to restricted methods available to plugins

Add support for recipient address audit plugins

Security advisory: https://www.npmjs.com/advisories/1184 The security advisory was already addressed in commit 8e592df, but something about the lockfile seems to confuse the audit API (or we're running into a bug with resolutions). This resolution is simpler, and accomplishes the same goal of updating the vulnerable package.

Allows plugins to define a very flexible JS-based API like this: ``` wallet.registerApiRequestHandler((fromOrigin) => { return { ping: () => 'pong!', on: this.eventEmitter.on.bind(this.eventEmitter), } }) ``` All methods are converted to async methods per [capnode](https://www.npmjs.com/package/capnode). Sites can then request the flexible API like this: ``` async function requestIndex () { index = await ethereum.requestIndex() pluginApi = await index.getPluginApi(pluginOrigin) // Use the API provided by the plugin here! } ``` Includes a change to the `metamask-inpage-provider` that adds the `requestIndex()` method to it, for requesting a MetaMask capnode API. Adds a `requestPluginApi(pluginId)` method to the MetaMask index for requesting plugin APIs. Has weird issue: - [ ] Logs an inaccurate unauthorized error in dapp console when requesting permissions. Permissions request still succeeds.

Add plugin wallet.registerApiRequestHandler method

lilyannehall

Some thoughts, questions, and things we should should follow up on from my initial pass code review.

lilyannehall · 2019-10-31T17:54:38Z

app/scripts/controllers/assets.js

+
+  validateAsset (fromDomain, opts) {
+    assetRequiredFields.forEach((requiredField) => {
+      if (!(requiredField in opts)) {


This check will return true if the opts[requiredField] property is declared but undefined. Example:

em@xps7390 ~ $ node > const opts = { symbol: undefined, balance: null, identifier: function() {}, decimals: -19, customViewUrl: '' }; undefined > const assetRequiredFields = ['symbol', 'balance', 'identifier', 'decimals', 'customViewUrl'] undefined > assetRequiredFields.forEach((requiredField) => { ... if (!(requiredField in opts)) { ..... throw new Error('missing required field'); ..... } ... }) undefined >

Instead of (!(requiredField in opts)) use (typeof opts[requiredField] === 'undefined'), as well as adding more sophisticated validation for the specific fields.

I wasn't aware of this best practice. Can you tell me why it's better?

@danfinlay in that example i posted up there you can see that using (foo in bar) returns true, even if bar[foo] === undefined, because the the in check only checks that the property exists - not the value. so if you don't check against undefined (or check for general "truthiness" of the value) you can end up with null reference exceptions and similar issues because {foo:undefined} will pass the check for 'foo' in bar but not typeof bar[foo] !== 'undefined' or even just !!bar[foo]. am i explaining that clearly?

Oh right, of course, we want to be validating for value presence. Thanks!

lilyannehall · 2019-10-31T18:15:27Z

app/scripts/controllers/permissions/permissions.js

+          // We swallow this error, we don't want the plugin permissions prompt to block the resolution
+          // Of the main dapp's permissions prompt.
+          console.error(`Error when adding plugin:`, err)


If we swallow the error, does the user get notified that there was an issue?

@danfinlay should the user be notified? could failing silently be bad?

The requesting site would be the thing that doesn't get notified. It could break the site's ability to use the plugin, and will result in errors when trying to interact with the plugin in the future, so this seems like a passable interaction to me, but we could not respond to the dapp until all the plugins are installed. In that case we'd want to add some stronger UI to indicate to the user that installation is happening (which we'll surely add anyways).

lilyannehall · 2019-10-31T18:19:08Z

app/scripts/controllers/permissions/permissions.js

    this.testProfile = {
      name: 'Dan Finlay',
    }


This supposed to be here still?

This very much no longer needs to be here, we'll definitely be removing it too. Sorry this is def. a very early audit, some very drafty proof-of-concept code.

lilyannehall · 2019-10-31T18:25:27Z

app/scripts/controllers/permissions/restrictedMethods.js

+  updatePluginState: 'Store data locally',
+  getPluginState: 'Get data stored locally',


NOTE: I wonder if these can be used to donk up the state machine and cause problems.

Yes, because it's part of the main state. I had forgotten to record this as a prod-blocker on this repo, but had opened an issue upstream. Now open here as I understand it: MetaMask/metamask-snaps-beta#88

lilyannehall · 2019-10-31T18:29:12Z

app/scripts/controllers/permissions/restrictedMethods.js

+  getFilteredTxList: 'Get a list of filtered transactions',
+  getGasPrice: 'Estimates a good gas price at recent prices',
+  getTxById: 'Get full data of a transaction with a given metamask tx id',
+  getState: 'Get a JSON representation of MetaMask data, including sensitive data. This is only for testing purposes and will NOT be included in production.',


I want to follow up on this and make sure that it doesn't get left in or otherwise is accessible.

how does it get removed for production?

it reads like "let's remember to remove this" but maybe @danfinlay knows something we don't?

Yes, this section is basically entirely composed of permissions that we are including for the beta period, but will only include each after concluding it is useful and safe. It will be a manual process of actually grooming for production, and then re-introducing permissions deliberately.

@danfinlay that's going to make it kind of tough for us to evaluate, since we don't really know what will and won't make it to prod. the only one that really sticks out to me as a big red flag is the getState one which is obviously noted to included sensitive data. clearly, you all plan to remove it, but i think for the purposes of our audit, we probably need to operate under the "truth" that it is there now, so i'll open a ticket here.

Yeah, I would say you should just ignore these individual permissions, sorry if I didn't make it clear. Rather than considering each one by one (which we will be doing before we accept any of these into production), I would recommend limiting the scope to a more framework-level consideration:

Does the security model of granting permissions to a plugin make sense?

Are there ways for a plugin to escalate the privileges it's been granted (excluding individual permissions).

If we were to include some permissions in the audit, I would limit the scope to ones we like enough to have documented in our wiki here.

lilyannehall · 2019-10-31T19:03:35Z

app/scripts/controllers/plugins.js

+const isTest = process.env.IN_TEST === 'true' || process.env.METAMASK_ENV === 'test'
+const SES = (
+  isTest
+    ? {
+      makeSESRootRealm: () => {
+        return {
+          evaluate: () => {
+            return () => true
+          },
+        }
+      },
+    }
+    : require('ses')
+)


Is it possible for another script to trick this into not using SES by setting global.process.env.IN_TEST?

This is a good concern, we should ensure we freeze the environment at boot time.

lilyannehall · 2019-10-31T19:05:27Z

app/scripts/controllers/plugins.js

+
+    // TODO:SECURITY disable errorStackMode for production
+    this.rootRealm = SES.makeSESRootRealm({
+      consoleMode: 'allow', errorStackMode: 'allow', mathRandomMode: 'allow',


Let's make sure this doesn't wiggle its way into prod

lilyannehall · 2019-10-31T19:07:47Z

app/scripts/controllers/plugins.js

+    console.log(`Adding ${sourceUrl}`)
+
+    // Deduplicate multiple add requests:
+    if (!(pluginName in this.adding)) {


Avoid using (foo in bar) truth check, since it checks initialization, not value.

lilyannehall · 2019-10-31T19:13:53Z

app/scripts/controllers/plugins.js

+    const possibleApis = {
+      updatePluginState: this.updatePluginState.bind(this, pluginName),
+      getPluginState: this.getPluginState.bind(this, pluginName),
+      onNewTx: () => {},


Is this supposed to be a noop?

This looks like a stub that we considered implementing but haven't yet.

lilyannehall · 2019-10-31T19:26:33Z

app/scripts/metamask-controller.js

+        // TODO: is this safe?
        if (origin === 'MetaMask') {


Follow up on this. What passes origin to this method? Can it be manipulated?

danfinlay

Whoops I added comments under a review. Submitting so those comments go through.

I agree we can probably open a single issue with check marks to reflect the various points to hit.

kumavis and others added 30 commits August 26, 2019 11:14

finish privacy mode removal

b6ac66d

basic wait-on-unlock implementatation in permissions controller

add permissions settings page

5169f9a

add checkbox selection to permission approval page begin add permissions settings page; edit messages revert isUnlocked behavior for now; it is managed both by the permissions and keyring controllers

rebase -> develop; yarn; babel@7

5c370ea

ignore circleci e2e and coveralls; lint

rebase; refactor site metadata; persist perms

15b78d3

show "connected" account in popup

Plugin mvp v3

4d437af

feat: add descriptions for all MetaMask-specific API methods (#7)

f22749a

Ensure none-plugins cannot access metamask controller methods made av…

78fcf38

…ailable to plugins

Make permissions.removePermissionsFor and getApprovedAccounts availab…

94da610

…le to plugins

Prefix plugin restricted methods with metamask_

3dfeac2

Use forceParams caveat for addPlugin and support display of url on ad…

25cdc62

…d plugin request UI

Fix runPlugin call resolution

c127802

Make pluginsController._generateApisToProvide support nested metamask…

dc8a103

…_ methods

Merge pull request #9 from MetaMask/plugin-system-enhancements

53e5412

Plugin system enhancements

Persist plugins to state

bf89800

Only save a new plugin to state after permission to install it has be…

f24e088

…en approved

Use plugin from state, instead of fetching, if it exists in state

a5fc7b5

Run existing plugins on start

bac015e

Merge pull request #11 from MetaMask/persist-and-restart-plugins

f525516

Persist and restart plugins

Make metamask events from blocktracker, transaction and network contr…

556136b

…ollers available to plugins

Update example plugin to reflect latest changes

2383987

Begin adding basic plugin message passing

27f7d34

Merge pull request #12 from MetaMask/metamask-events

06cc268

Metamask events

Merge branch 'plugin-beta' into SimplifyPluginConnections

e76ce12

Bump rpc-cap version

35b0f32

To allow passing the origin to restrictedMethods

Add support for plugins to call wallet.registerRpcMessageHandler()

7defa17

type rpcMessageHandler = (origin:string, request:Object) => Promise<any>

Add plugin wildcard method for rpc message passing.

871f09d

Actually expose method to plugins

ae1e7eb

Merge pull request #13 from MetaMask/SimplifyPluginConnections

4e0355f

Allow plugins to handle RPC requests

Fix typo

b1c1643

danjm and others added 15 commits October 9, 2019 09:21

Allow initialPermissions requests for internal metamask methods/event…

679c925

…s without metamask_ prefix

Merge pull request #56 from MetaMask/initial-permissions-wo-namespace

f6c74bd

Make `metamask_` prefix optional

Merge pull request #57 from MetaMask/getpluginstate-description

d7ab8d8

Adds description for the getPluginState permission

Merge pull request #58 from MetaMask/getTxById

5cae165

Adds getTxById to restricted methods available to plugins

Add support for recipient address audit plugins

3db17de

Merge pull request #59 from MetaMask/recipient-address-audit

8859d80

Add support for recipient address audit plugins

add onUnlock to default api; minor fixes

7763d21

update ses, realms-shim

cd44d71

lint

44baef7

fix security advisory

8e592df

add existing plugins to console (#66)

515165b

add Date to endowments

829b223

Merge pull request #76 from MetaMask/AddCapnode

789e84a

Add plugin wallet.registerApiRequestHandler method

lilyannehall requested review from aleitner and dominictarr October 29, 2019 16:56

lilyannehall assigned lilyannehall, dominictarr and aleitner Oct 29, 2019

lilyannehall removed request for dominictarr and aleitner October 29, 2019 16:56

lilyannehall commented Oct 31, 2019

View reviewed changes

danfinlay suggested changes Nov 5, 2019

View reviewed changes

hkurhan unassigned aleitner Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SNAPSHOT] Plugin Beta Changes #1

[SNAPSHOT] Plugin Beta Changes #1

lilyannehall commented Oct 29, 2019

lilyannehall left a comment

lilyannehall Oct 31, 2019

danfinlay Nov 2, 2019

lilyannehall Nov 4, 2019 •

edited

Loading

danfinlay Nov 5, 2019

lilyannehall Oct 31, 2019

lilyannehall Oct 31, 2019

danfinlay Nov 2, 2019

lilyannehall Oct 31, 2019

lilyannehall Oct 31, 2019

danfinlay Nov 2, 2019

lilyannehall Oct 31, 2019

danfinlay Nov 2, 2019

lilyannehall Oct 31, 2019

dominictarr Oct 31, 2019

lilyannehall Nov 1, 2019

danfinlay Nov 2, 2019

lilyannehall Nov 4, 2019

danfinlay Nov 5, 2019

lilyannehall Oct 31, 2019

danfinlay Nov 2, 2019

lilyannehall Oct 31, 2019

lilyannehall Oct 31, 2019

lilyannehall Oct 31, 2019

danfinlay Nov 2, 2019

lilyannehall Oct 31, 2019

danfinlay left a comment

		updatePluginState: 'Store data locally',
		getPluginState: 'Get data stored locally',

[SNAPSHOT] Plugin Beta Changes #1

Are you sure you want to change the base?

[SNAPSHOT] Plugin Beta Changes #1

Conversation

lilyannehall commented Oct 29, 2019

lilyannehall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyannehall Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danfinlay left a comment

Choose a reason for hiding this comment

lilyannehall Nov 4, 2019 •

edited

Loading