From de6455151e3861a36134ab7f92180f571d55f7dc Mon Sep 17 00:00:00 2001 From: Frankie Date: Thu, 9 Feb 2017 17:31:44 -0800 Subject: Revert old style message sighing --- app/scripts/keyrings/hd.js | 13 ++++++++++++- app/scripts/keyrings/simple.js | 13 ++++++++++++- test/unit/id-management-test.js | 2 +- test/unit/keyrings/simple-test.js | 2 +- ui/app/components/pending-msg.js | 9 +++++++++ 5 files changed, 35 insertions(+), 4 deletions(-) diff --git a/app/scripts/keyrings/hd.js b/app/scripts/keyrings/hd.js index 2e3b74192..7b10f925a 100644 --- a/app/scripts/keyrings/hd.js +++ b/app/scripts/keyrings/hd.js @@ -74,7 +74,18 @@ class HdKeyring extends EventEmitter { } // For eth_sign, we need to sign transactions: - signMessage (withAccount, msgHex) { + // hd + signMessage (withAccount, data) { + const wallet = this._getWalletForAccount(withAccount) + const message = ethUtil.stripHexPrefix(data) + var privKey = wallet.getPrivateKey() + var msgSig = ethUtil.ecsign(new Buffer(message, 'hex'), privKey) + var rawMsgSig = ethUtil.bufferToHex(sigUtil.concatSig(msgSig.v, msgSig.r, msgSig.s)) + return Promise.resolve(rawMsgSig) + } + + // For eth_sign, we need to sign transactions: + newGethSignMessage (withAccount, msgHex) { const wallet = this._getWalletForAccount(withAccount) const privKey = wallet.getPrivateKey() const msgBuffer = ethUtil.toBuffer(msgHex) diff --git a/app/scripts/keyrings/simple.js b/app/scripts/keyrings/simple.js index fa8e9fd78..b6ffc606e 100644 --- a/app/scripts/keyrings/simple.js +++ b/app/scripts/keyrings/simple.js @@ -58,7 +58,18 @@ class SimpleKeyring extends EventEmitter { } // For eth_sign, we need to sign transactions: - signMessage (withAccount, msgHex) { + signMessage (withAccount, data) { + const wallet = this._getWalletForAccount(withAccount) + const message = ethUtil.stripHexPrefix(data) + var privKey = wallet.getPrivateKey() + var msgSig = ethUtil.ecsign(new Buffer(message, 'hex'), privKey) + var rawMsgSig = ethUtil.bufferToHex(sigUtil.concatSig(msgSig.v, msgSig.r, msgSig.s)) + return Promise.resolve(rawMsgSig) + } + + // For eth_sign, we need to sign transactions: + + newGethSignMessage (withAccount, msgHex) { const wallet = this._getWalletForAccount(withAccount) const privKey = wallet.getPrivateKey() const msgBuffer = ethUtil.toBuffer(msgHex) diff --git a/test/unit/id-management-test.js b/test/unit/id-management-test.js index 25eea8777..cbc6403bc 100644 --- a/test/unit/id-management-test.js +++ b/test/unit/id-management-test.js @@ -16,7 +16,7 @@ describe('IdManagement', function() { }) describe('#signMsg', function () { - it.skip('passes the dennis test', function() { + it('passes the dennis test', function() { const address = '0x9858e7d8b79fc3e6d989636721584498926da38a' const message = '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0' const privateKey = '0x7dd98753d7b4394095de7d176c58128e2ed6ee600abe97c9f6d9fd65015d9b18' diff --git a/test/unit/keyrings/simple-test.js b/test/unit/keyrings/simple-test.js index 5fe29a67d..77eeb834c 100644 --- a/test/unit/keyrings/simple-test.js +++ b/test/unit/keyrings/simple-test.js @@ -55,7 +55,7 @@ describe('simple-keyring', function() { const privateKey = '0x7dd98753d7b4394095de7d176c58128e2ed6ee600abe97c9f6d9fd65015d9b18' const expectedResult = '0x28fcb6768e5110144a55b2e6ce9d1ea5a58103033632d272d2b5cf506906f7941a00b539383fd872109633d8c71c404e13dba87bc84166ee31b0e36061a69e161c' - it.skip('passes the dennis test', function(done) { + it('passes the dennis test', function(done) { keyring.deserialize([ privateKey ]) .then(() => { return keyring.signMessage(address, message) diff --git a/ui/app/components/pending-msg.js b/ui/app/components/pending-msg.js index f4bde91dc..b2cac164a 100644 --- a/ui/app/components/pending-msg.js +++ b/ui/app/components/pending-msg.js @@ -28,6 +28,15 @@ PendingMsg.prototype.render = function () { }, }, 'Sign Message'), + h('.error', { + style: { + margin: '10px', + }, + }, `Signing this message can have + dangerous side effects. Only sign messages from + sites you fully trust with your entire account. + This will be fixed in a future version.`), + // message details h(PendingTxDetails, state), -- cgit From 26cd66cb6264c714ddeb924127921581fa58d558 Mon Sep 17 00:00:00 2001 From: Frankie Date: Thu, 9 Feb 2017 17:34:05 -0800 Subject: Add to CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55c60e185..11588278b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ## 3.2.1 2017-2-8 +- Revert back to old style message signing. - Fixed some build errors that were causing a variety of bugs. ## 3.2.0 2017-2-8 -- cgit From 26ae3d206f71025b403b8124bb6f35b96dc90565 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 9 Feb 2017 18:21:11 -0800 Subject: Version 3.2.2 --- CHANGELOG.md | 4 ++++ app/manifest.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11588278b..2a29e3c25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Current Master +## 3.2.2 2017-2-8 + +- Revert eth.sign behavior to the previous one with a big warning. We will be gradually implementing the new behavior over the coming time. https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign + ## 3.2.1 2017-2-8 - Revert back to old style message signing. diff --git a/app/manifest.json b/app/manifest.json index c5d34e6ea..95475ddf1 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.2.1", + "version": "3.2.2", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", -- cgit From b862d942097b645abe31fa2af87226e544ecd627 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 13 Feb 2017 20:13:11 -0800 Subject: Add failing test for signature recovery --- test/unit/keyrings/simple-test.js | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/unit/keyrings/simple-test.js b/test/unit/keyrings/simple-test.js index 77eeb834c..60b762cb8 100644 --- a/test/unit/keyrings/simple-test.js +++ b/test/unit/keyrings/simple-test.js @@ -1,5 +1,7 @@ const assert = require('assert') const extend = require('xtend') +const Web3 = require('web3') +const web3 = new Web3() const ethUtil = require('ethereumjs-util') const SimpleKeyring = require('../../../app/scripts/keyrings/simple') const TYPE_STR = 'Simple Key Pair' @@ -65,6 +67,37 @@ describe('simple-keyring', function() { done() }) }) + + it('reliably can decode messages it signs', function (done) { + + const message = 'hello there!' + let address, msgHex + + keyring.addAccounts(10) + .then((addresses) => { + address = addresses[0] + msgHex = web3.sha3(message) + return keyring.signMessage(address, msgHex) + }) + .then((signature) => { + var sgn = signature + var r = ethUtil.toBuffer(sgn.slice(0,66)) + var s = ethUtil.toBuffer('0x' + sgn.slice(66,130)) + var v = parseInt(sgn.slice(130,132)) + 27 + var msgBuffer = ethUtil.toBuffer(msgHex) + var msgHash = ethUtil.hashPersonalMessage(msgBuffer) + var pub = ethUtil.ecrecover(msgHash, v, r, s) + var adr = '0x' + ethUtil.pubToAddress(pub).toString('hex') + assert.equal(adr, address) + done() + }) + .catch((reason) => { + console.log('failed for reason:') + console.dir(reason) + assert.equal(true, false, reason) + done() + }) + }) }) describe('#addAccounts', function() { -- cgit From f2486fbdd34246619f62ff67bb3ab6e77d202f26 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 13 Feb 2017 21:25:02 -0800 Subject: got hash test passing --- app/scripts/keyrings/hd.js | 4 +++- app/scripts/keyrings/simple.js | 3 ++- test/unit/keyrings/simple-test.js | 37 +++++++++++++++++++++++-------------- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/app/scripts/keyrings/hd.js b/app/scripts/keyrings/hd.js index 7b10f925a..3a66f7868 100644 --- a/app/scripts/keyrings/hd.js +++ b/app/scripts/keyrings/hd.js @@ -112,9 +112,11 @@ class HdKeyring extends EventEmitter { _getWalletForAccount (account) { + const targetAddress = sigUtil.normalize(account) return this.wallets.find((w) => { const address = w.getAddress().toString('hex') - return ((address === account) || (sigUtil.normalize(address) === account)) + return ((address === targetAddress) || + (sigUtil.normalize(address) === targetAddress)) }) } } diff --git a/app/scripts/keyrings/simple.js b/app/scripts/keyrings/simple.js index b6ffc606e..82881aa2d 100644 --- a/app/scripts/keyrings/simple.js +++ b/app/scripts/keyrings/simple.js @@ -88,7 +88,8 @@ class SimpleKeyring extends EventEmitter { /* PRIVATE METHODS */ _getWalletForAccount (account) { - let wallet = this.wallets.find(w => ethUtil.bufferToHex(w.getAddress()) === account) + const address = sigUtil.normalize(account) + let wallet = this.wallets.find(w => ethUtil.bufferToHex(w.getAddress()) === address) if (!wallet) throw new Error('Simple Keyring - Unable to find matching address.') return wallet } diff --git a/test/unit/keyrings/simple-test.js b/test/unit/keyrings/simple-test.js index 60b762cb8..c09695b4c 100644 --- a/test/unit/keyrings/simple-test.js +++ b/test/unit/keyrings/simple-test.js @@ -71,31 +71,40 @@ describe('simple-keyring', function() { it('reliably can decode messages it signs', function (done) { const message = 'hello there!' - let address, msgHex + let address, msgHashHex - keyring.addAccounts(10) + keyring.deserialize([ privateKey ]) + .then(() => { + return keyring.getAccounts() + }) .then((addresses) => { address = addresses[0] - msgHex = web3.sha3(message) - return keyring.signMessage(address, msgHex) + msgHashHex = web3.sha3(message) + return keyring.signMessage(address, msgHashHex) }) - .then((signature) => { - var sgn = signature + .then((sgn) => { var r = ethUtil.toBuffer(sgn.slice(0,66)) var s = ethUtil.toBuffer('0x' + sgn.slice(66,130)) - var v = parseInt(sgn.slice(130,132)) + 27 - var msgBuffer = ethUtil.toBuffer(msgHex) - var msgHash = ethUtil.hashPersonalMessage(msgBuffer) - var pub = ethUtil.ecrecover(msgHash, v, r, s) + var v = ethUtil.bufferToInt(ethUtil.toBuffer('0x' + sgn.slice(130,132))) + var m = ethUtil.toBuffer(msgHashHex) + console.dir({ r, s, v, m }) + var pub = ethUtil.ecrecover(m, v, r, s) var adr = '0x' + ethUtil.pubToAddress(pub).toString('hex') - assert.equal(adr, address) + + /* + var sgn = ethUtil.stripHexPrefix(signature) + var r = ethUtil.toBuffer(sgn.slice(0,64)) + var s = ethUtil.toBuffer(sgn.slice(64,128)) + var v = parseInt(sgn.slice(128,130)) + 27 + var msgHashBuffer = ethUtil.toBuffer(msgHashHex) + var pub = ethUtil.ecrecover(msgHashBuffer, v, r, s) + var adr = '0x' + ethUtil.pubToAddress(pub).toString('hex') + */ + assert.equal(adr, address, 'recovers address from signature correctly') done() }) .catch((reason) => { - console.log('failed for reason:') console.dir(reason) - assert.equal(true, false, reason) - done() }) }) }) -- cgit From 97b8410b3001a45249244b5a236d31e1975154a2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 13 Feb 2017 21:29:22 -0800 Subject: Verify messages in a loop --- test/unit/keyrings/simple-test.js | 52 +++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/test/unit/keyrings/simple-test.js b/test/unit/keyrings/simple-test.js index c09695b4c..ba7dd448a 100644 --- a/test/unit/keyrings/simple-test.js +++ b/test/unit/keyrings/simple-test.js @@ -71,41 +71,39 @@ describe('simple-keyring', function() { it('reliably can decode messages it signs', function (done) { const message = 'hello there!' - let address, msgHashHex + const msgHashHex = web3.sha3(message) + let address + let addresses = [] keyring.deserialize([ privateKey ]) + .then(() => { + keyring.addAccounts(9) + }) .then(() => { return keyring.getAccounts() }) - .then((addresses) => { - address = addresses[0] - msgHashHex = web3.sha3(message) - return keyring.signMessage(address, msgHashHex) + .then((addrs) => { + addresses = addrs + return Promise.all(addresses.map((address) => { + return keyring.signMessage(address, msgHashHex) + })) }) - .then((sgn) => { - var r = ethUtil.toBuffer(sgn.slice(0,66)) - var s = ethUtil.toBuffer('0x' + sgn.slice(66,130)) - var v = ethUtil.bufferToInt(ethUtil.toBuffer('0x' + sgn.slice(130,132))) - var m = ethUtil.toBuffer(msgHashHex) - console.dir({ r, s, v, m }) - var pub = ethUtil.ecrecover(m, v, r, s) - var adr = '0x' + ethUtil.pubToAddress(pub).toString('hex') - - /* - var sgn = ethUtil.stripHexPrefix(signature) - var r = ethUtil.toBuffer(sgn.slice(0,64)) - var s = ethUtil.toBuffer(sgn.slice(64,128)) - var v = parseInt(sgn.slice(128,130)) + 27 - var msgHashBuffer = ethUtil.toBuffer(msgHashHex) - var pub = ethUtil.ecrecover(msgHashBuffer, v, r, s) - var adr = '0x' + ethUtil.pubToAddress(pub).toString('hex') - */ - assert.equal(adr, address, 'recovers address from signature correctly') + .then((signatures) => { + + signatures.forEach((sgn, index) => { + const address = addresses[index] + + var r = ethUtil.toBuffer(sgn.slice(0,66)) + var s = ethUtil.toBuffer('0x' + sgn.slice(66,130)) + var v = ethUtil.bufferToInt(ethUtil.toBuffer('0x' + sgn.slice(130,132))) + var m = ethUtil.toBuffer(msgHashHex) + var pub = ethUtil.ecrecover(m, v, r, s) + var adr = '0x' + ethUtil.pubToAddress(pub).toString('hex') + + assert.equal(adr, address, 'recovers address from signature correctly') + }) done() }) - .catch((reason) => { - console.dir(reason) - }) }) }) -- cgit From 0b0eec9c52667148fc69e1a3446bcd208dab3858 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 13 Feb 2017 21:30:08 -0800 Subject: Bump changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a29e3c25..00ab548ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +- Improve test coverage of eth.sign behavior, including a code example of verifying a signature. + ## 3.2.2 2017-2-8 - Revert eth.sign behavior to the previous one with a big warning. We will be gradually implementing the new behavior over the coming time. https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign -- cgit