From 4b4bee77c708b18531179f1574f1058455c13ff8 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Oct 2019 11:33:49 -0700 Subject: [PATCH 1/4] Move signTypedData signing out to keyrings This simplifies the logic of signing and improves security: - Private keys are never moved to the base controller. - Hardware wallets are abstracted in the same way as local keys. This also paves the way for allowing even more modular accounts, provided by plugins: https://github.com/MetaMask/metamask-plugin-beta/pull/63 Fixes #7075. --- app/scripts/metamask-controller.js | 29 ++++------------- package.json | 4 +-- yarn.lock | 51 +++++++++++++++++++----------- 3 files changed, 41 insertions(+), 43 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index b4014272a..16d879f34 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -53,7 +53,6 @@ const seedPhraseVerifier = require('./lib/seed-phrase-verifier') const log = require('loglevel') const TrezorKeyring = require('eth-trezor-keyring') const LedgerBridgeKeyring = require('eth-ledger-bridge-keyring') -const HW_WALLETS_KEYRINGS = [TrezorKeyring.type, LedgerBridgeKeyring.type] const EthQuery = require('eth-query') const ethUtil = require('ethereumjs-util') const sigUtil = require('eth-sig-util') @@ -1168,28 +1167,14 @@ module.exports = class MetamaskController extends EventEmitter { const version = msgParams.version try { const cleanMsgParams = await this.typedMessageManager.approveMessage(msgParams) - const address = sigUtil.normalize(cleanMsgParams.from) - const keyring = await this.keyringController.getKeyringForAccount(address) - let signature - // HW Wallet keyrings don't expose private keys - // so we need to handle it separately - if (!HW_WALLETS_KEYRINGS.includes(keyring.type)) { - const wallet = keyring._getWalletForAccount(address) - const privKey = ethUtil.toBuffer(wallet.getPrivateKey()) - switch (version) { - case 'V1': - signature = sigUtil.signTypedDataLegacy(privKey, { data: cleanMsgParams.data }) - break - case 'V3': - signature = sigUtil.signTypedData(privKey, { data: JSON.parse(cleanMsgParams.data) }) - break - case 'V4': - signature = sigUtil.signTypedData_v4(privKey, { data: JSON.parse(cleanMsgParams.data) }) - break - } - } else { - signature = await keyring.signTypedData(address, cleanMsgParams.data) + + // For some reason every version after V1 has stringified params. + if (version !== 'V1') { + cleanMsgParams.data = JSON.parse(cleanMsgParams.data) } + + const address = sigUtil.normalize(cleanMsgParams.from) + const signature = await this.keyringController.signTypedMessage(cleanMsgParams, { version }) this.typedMessageManager.setMsgStatusSigned(msgId, signature) return this.getState() } catch (error) { diff --git a/package.json b/package.json index 10a858130..2ffbef2a9 100644 --- a/package.json +++ b/package.json @@ -91,7 +91,7 @@ "eth-json-rpc-filters": "^4.1.0", "eth-json-rpc-infura": "^4.0.1", "eth-json-rpc-middleware": "^4.2.0", - "eth-keyring-controller": "^5.1.0", + "eth-keyring-controller": "^5.3.0", "eth-ledger-bridge-keyring": "^0.2.0", "eth-method-registry": "^1.2.0", "eth-phishing-detect": "^1.1.4", @@ -203,7 +203,6 @@ "css-loader": "^2.1.1", "deep-freeze-strict": "^1.1.1", "del": "^3.0.0", - "read-installed": "^4.0.3", "deps-dump": "^1.1.0", "envify": "^4.0.0", "enzyme": "^3.4.4", @@ -266,6 +265,7 @@ "radgrad-jsdoc-template": "^1.1.3", "react-devtools": "^3.6.1", "react-test-renderer": "^15.6.2", + "read-installed": "^4.0.3", "redux-mock-store": "^1.5.3", "redux-test-utils": "^0.2.2", "remote-redux-devtools": "^0.5.16", diff --git a/yarn.lock b/yarn.lock index 3224c34c1..02efa867f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9737,13 +9737,14 @@ eth-hd-keyring@^2.0.0: events "^1.1.1" xtend "^4.0.1" -eth-hd-keyring@^2.1.0: - version "2.1.0" - resolved "https://registry.yarnpkg.com/eth-hd-keyring/-/eth-hd-keyring-2.1.0.tgz#da93224f791fb1d9220598f7c96f04181dff6ace" - integrity sha512-1ZcPmj4h18tmwG7RIturC2lpeimuiAyUGW9Ic5ztCFNeRAcfiQ3ic7sENWhzqbSgth9xkAMIHqM8IY31VJviZg== +eth-hd-keyring@^3.4.0: + version "3.4.0" + resolved "https://registry.yarnpkg.com/eth-hd-keyring/-/eth-hd-keyring-3.4.0.tgz#288e73041f2b3f047b4151fb4b5ab5ad5710b9a6" + integrity sha512-MMKSSwDWuEfItEM/826LHrs2HVjy57qQQfcgSxIYOCJY0vykw++LH8d6QJOBrGFe+xu/gtbHBRMURrFGdqfevw== dependencies: bip39 "^2.2.0" - eth-sig-util "^2.0.1" + eth-sig-util "^2.4.4" + eth-simple-keyring "^3.4.0" ethereumjs-abi "^0.6.5" ethereumjs-util "^5.1.1" ethereumjs-wallet "^0.6.0" @@ -9846,17 +9847,17 @@ eth-keyring-controller@^5.0.1: obs-store "^4.0.3" promise-filter "^1.1.0" -eth-keyring-controller@^5.1.0: - version "5.1.0" - resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.1.0.tgz#1dbe539f6ca483d6f2fa16b33b00d66eeb2de974" - integrity sha512-NCXzYIBHJo5o8XoyPdGRLRLTi2pitJKlZPcDO+Un4wbt3djrvWH8adopLH+kNw9zUfk2C5OQqUQ4/UibSvanxA== +eth-keyring-controller@^5.3.0: + version "5.3.0" + resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.3.0.tgz#8d85a67b894360ab7d601222ca71df8ed5f456c6" + integrity sha512-aoqYjKUbdOSgS1s63IsTp69QAMtLPTCqR4VAbCULps71o3yuaA9JRx2W+GAxqVNBtIdyZllxoOJlvErkO4iWNw== dependencies: bip39 "^2.4.0" bluebird "^3.5.0" browser-passworder "^2.0.3" - eth-hd-keyring "^2.1.0" + eth-hd-keyring "^3.4.0" eth-sig-util "^1.4.0" - eth-simple-keyring "^2.1.0" + eth-simple-keyring "^3.4.0" ethereumjs-util "^5.1.2" loglevel "^1.5.0" obs-store "^4.0.3" @@ -9962,6 +9963,18 @@ eth-sig-util@^2.1.0: tweetnacl "^1.0.0" tweetnacl-util "^0.15.0" +eth-sig-util@^2.4.4, eth-sig-util@^2.5.0: + version "2.5.0" + resolved "https://registry.yarnpkg.com/eth-sig-util/-/eth-sig-util-2.5.0.tgz#1018cf8bef2fe275ecbd526cf3248757b0880053" + integrity sha512-ahApxr+e1cls/GwcFSGsgRLrMqG6D6cBnK9CRHhx97O/s9ow+URIxbPvov8jfE70ZnNBdHMircoSCpi1b4QHjA== + dependencies: + buffer "^5.2.1" + elliptic "^6.4.0" + ethereumjs-abi "0.6.5" + ethereumjs-util "^5.1.1" + tweetnacl "^1.0.0" + tweetnacl-util "^0.15.0" + eth-simple-keyring@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/eth-simple-keyring/-/eth-simple-keyring-2.0.0.tgz#ef1e97c4aebb7229dce9c0ec5cc84efcd3a76395" @@ -9974,12 +9987,12 @@ eth-simple-keyring@^2.0.0: events "^1.1.1" xtend "^4.0.1" -eth-simple-keyring@^2.1.0: - version "2.1.0" - resolved "https://registry.yarnpkg.com/eth-simple-keyring/-/eth-simple-keyring-2.1.0.tgz#bfff8f7c438a9b5164e995db1cccbbdb9e27752f" - integrity sha512-31emUxGHOVhYzlPoEGyfrn1Usi2ZI9qDqMnDHwG0R0HdIuF/I10qlf401MkiD9lcMDuvgJkLpqqAvGxrKrFCwA== +eth-simple-keyring@^3.4.0: + version "3.4.0" + resolved "https://registry.yarnpkg.com/eth-simple-keyring/-/eth-simple-keyring-3.4.0.tgz#01464234b070af42a343a3c451dd58b00ae1a042" + integrity sha512-g/ObWqWaTHikrhhm7fNinpkkpEPqBRz02oBXcH81mc3VFkOLb3pjfvNg1Da6Jh+A4wA0kBE4vkiiG7BUwF1zNg== dependencies: - eth-sig-util "^2.0.1" + eth-sig-util "^2.5.0" ethereumjs-abi "^0.6.5" ethereumjs-util "^5.1.1" ethereumjs-wallet "^0.6.0" @@ -13470,9 +13483,9 @@ https-proxy-agent@2.2.1, https-proxy-agent@^2.1.1: debug "^3.1.0" https-proxy-agent@^2.2.1: - version "2.2.2" - resolved "https://registry.yarnpkg.com/https-proxy-agent/-/https-proxy-agent-2.2.2.tgz#271ea8e90f836ac9f119daccd39c19ff7dfb0793" - integrity sha512-c8Ndjc9Bkpfx/vCJueCPy0jlP4ccCCSNDp8xwCZzPjKJUm+B+u9WX2x98Qx4n1PiMNTWo3D7KK5ifNV/yJyRzg== + version "2.2.3" + resolved "https://registry.yarnpkg.com/https-proxy-agent/-/https-proxy-agent-2.2.3.tgz#fb6cd98ed5b9c35056b5a73cd01a8a721d7193d1" + integrity sha512-Ytgnz23gm2DVftnzqRRz2dOXZbGd2uiajSw/95bPp6v53zPRspQjLm/AfBgqbJ2qfeRXWIOMVLpp86+/5yX39Q== dependencies: agent-base "^4.3.0" debug "^3.1.0" From d26a8e7f82938e59b7d6b1668d3b480246e3da49 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Oct 2019 14:53:25 -0700 Subject: [PATCH 2/4] Allow non strigified typed data params --- app/scripts/metamask-controller.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 16d879f34..e9683580c 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1168,9 +1168,12 @@ module.exports = class MetamaskController extends EventEmitter { try { const cleanMsgParams = await this.typedMessageManager.approveMessage(msgParams) - // For some reason every version after V1 has stringified params. + // For some reason every version after V1 used stringified params. if (version !== 'V1') { - cleanMsgParams.data = JSON.parse(cleanMsgParams.data) + // But we don't have to require that. We can stop suggesting it now: + if (typeof cleanMsgParams.data === 'string') { + cleanMsgParams.data = JSON.parse(cleanMsgParams.data) + } } const address = sigUtil.normalize(cleanMsgParams.from) From 3eee9a2458c71087bd68682239754869f97e785a Mon Sep 17 00:00:00 2001 From: Dan Finlay <542863+danfinlay@users.noreply.github.com> Date: Wed, 23 Oct 2019 08:47:15 +0900 Subject: [PATCH 3/4] Linted --- app/scripts/metamask-controller.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index e9683580c..55750918b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1176,7 +1176,6 @@ module.exports = class MetamaskController extends EventEmitter { } } - const address = sigUtil.normalize(cleanMsgParams.from) const signature = await this.keyringController.signTypedMessage(cleanMsgParams, { version }) this.typedMessageManager.setMsgStatusSigned(msgId, signature) return this.getState() From 25076da9cbab44b71465984fcaec64d3cced83ba Mon Sep 17 00:00:00 2001 From: Dan Finlay <542863+danfinlay@users.noreply.github.com> Date: Wed, 23 Oct 2019 08:59:20 +0900 Subject: [PATCH 4/4] Remove unused dependency --- app/scripts/metamask-controller.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 55750918b..22ae6f0f5 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -55,7 +55,6 @@ const TrezorKeyring = require('eth-trezor-keyring') const LedgerBridgeKeyring = require('eth-ledger-bridge-keyring') const EthQuery = require('eth-query') const ethUtil = require('ethereumjs-util') -const sigUtil = require('eth-sig-util') const contractMap = require('eth-contract-metadata') const { AddressBookController,