From 37e552e95d67c8cf0273d0bb00eccd6ba0424f64 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 15 Jun 2016 10:48:36 -0700 Subject: [PATCH 1/5] Sign binary data not hash on eth_sign --- app/scripts/lib/idStore.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/lib/idStore.js b/app/scripts/lib/idStore.js index e9b9e0e06..e77ba87f8 100644 --- a/app/scripts/lib/idStore.js +++ b/app/scripts/lib/idStore.js @@ -492,8 +492,8 @@ function IdManagement(opts) { // sign message var privKeyHex = this.exportPrivateKey(address) var privKey = ethUtil.toBuffer(privKeyHex) - var msgHash = ethUtil.sha3(message) - var msgSig = ethUtil.ecsign(msgHash, privKey) + var msgBuffer = new Buffer(message.replace('0x',''), 'hex') + var msgSig = ethUtil.ecsign(msgBuffer, privKey) var rawMsgSig = ethUtil.bufferToHex(concatSig(msgSig.v, msgSig.r, msgSig.s)) return rawMsgSig } From ae3993b6d752207115a4767247caddbb42c197e3 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 15 Jun 2016 14:58:06 -0700 Subject: [PATCH 2/5] Factor idManagement into its own module --- app/scripts/lib/id-management.js | 79 ++++++++++++++++++++++++++++++++ app/scripts/lib/idStore.js | 77 +------------------------------ 2 files changed, 80 insertions(+), 76 deletions(-) create mode 100644 app/scripts/lib/id-management.js diff --git a/app/scripts/lib/id-management.js b/app/scripts/lib/id-management.js new file mode 100644 index 000000000..0edd2e336 --- /dev/null +++ b/app/scripts/lib/id-management.js @@ -0,0 +1,79 @@ +const ethUtil = require('ethereumjs-util') + +module.exports = IdManagement + +function IdManagement(opts) { + if (!opts) opts = {} + + this.keyStore = opts.keyStore + this.derivedKey = opts.derivedKey + this.hdPathString = "m/44'/60'/0'/0" + + this.getAddresses = function(){ + return keyStore.getAddresses(this.hdPathString).map(function(address){ return '0x'+address }) + } + + this.signTx = function(txParams){ + // normalize values + txParams.to = ethUtil.addHexPrefix(txParams.to) + txParams.from = ethUtil.addHexPrefix(txParams.from) + txParams.value = ethUtil.addHexPrefix(txParams.value) + txParams.data = ethUtil.addHexPrefix(txParams.data) + txParams.gasLimit = ethUtil.addHexPrefix(txParams.gasLimit || txParams.gas) + txParams.nonce = ethUtil.addHexPrefix(txParams.nonce) + var tx = new Transaction(txParams) + + // sign tx + var privKeyHex = this.exportPrivateKey(txParams.from) + var privKey = ethUtil.toBuffer(privKeyHex) + tx.sign(privKey) + + // Add the tx hash to the persisted meta-tx object + var txHash = ethUtil.bufferToHex(tx.hash()) + var metaTx = configManager.getTx(txParams.metamaskId) + metaTx.hash = txHash + configManager.updateTx(metaTx) + + // return raw serialized tx + var rawTx = ethUtil.bufferToHex(tx.serialize()) + return rawTx + } + + this.signMsg = function(address, message){ + // sign message + var privKeyHex = this.exportPrivateKey(address) + var privKey = ethUtil.toBuffer(privKeyHex) + var msgHash = ethUtil.sha3(message) + var msgBuffer = new Buffer(message.replace('0x',''), 'hex') + var msgSig = ethUtil.ecsign(msgBuffer, privKey) + var rawMsgSig = ethUtil.bufferToHex(concatSig(msgSig.v, msgSig.r, msgSig.s)) + return rawMsgSig + } + + this.getSeed = function(){ + return this.keyStore.getSeed(this.derivedKey) + } + + this.exportPrivateKey = function(address) { + var privKeyHex = ethUtil.addHexPrefix(this.keyStore.exportPrivateKey(address, this.derivedKey, this.hdPathString)) + return privKeyHex + } +} + +function pad_with_zeroes(number, length){ + var my_string = '' + number; + while (my_string.length < length) { + my_string = '0' + my_string; + } + return my_string; +} + +function concatSig(v, r, s) { + r = pad_with_zeroes(ethUtil.fromSigned(r), 64) + s = pad_with_zeroes(ethUtil.fromSigned(s), 64) + v = ethUtil.bufferToInt(v) + r = ethUtil.toUnsigned(r).toString('hex') + s = ethUtil.toUnsigned(s).toString('hex') + v = ethUtil.stripHexPrefix(ethUtil.intToHex(v)) + return ethUtil.addHexPrefix(r.concat(s, v).toString("hex")) +} diff --git a/app/scripts/lib/idStore.js b/app/scripts/lib/idStore.js index e77ba87f8..68a38bc0b 100644 --- a/app/scripts/lib/idStore.js +++ b/app/scripts/lib/idStore.js @@ -11,6 +11,7 @@ const autoFaucet = require('./auto-faucet') const configManager = require('./config-manager-singleton') const messageManager = require('./message-manager') const DEFAULT_RPC = 'https://testrpc.metamask.io/' +const IdManagement = require('./id-management') module.exports = IdentityStore @@ -451,82 +452,6 @@ IdentityStore.prototype._autoFaucet = function() { autoFaucet(addresses[0]) } -function IdManagement(opts) { - if (!opts) opts = {} - - this.keyStore = opts.keyStore - this.derivedKey = opts.derivedKey - this.hdPathString = "m/44'/60'/0'/0" - - this.getAddresses = function(){ - return keyStore.getAddresses(this.hdPathString).map(function(address){ return '0x'+address }) - } - - this.signTx = function(txParams){ - // normalize values - txParams.to = ethUtil.addHexPrefix(txParams.to) - txParams.from = ethUtil.addHexPrefix(txParams.from) - txParams.value = ethUtil.addHexPrefix(txParams.value) - txParams.data = ethUtil.addHexPrefix(txParams.data) - txParams.gasLimit = ethUtil.addHexPrefix(txParams.gasLimit || txParams.gas) - txParams.nonce = ethUtil.addHexPrefix(txParams.nonce) - var tx = new Transaction(txParams) - - // sign tx - var privKeyHex = this.exportPrivateKey(txParams.from) - var privKey = ethUtil.toBuffer(privKeyHex) - tx.sign(privKey) - - // Add the tx hash to the persisted meta-tx object - var txHash = ethUtil.bufferToHex(tx.hash()) - var metaTx = configManager.getTx(txParams.metamaskId) - metaTx.hash = txHash - configManager.updateTx(metaTx) - - // return raw serialized tx - var rawTx = ethUtil.bufferToHex(tx.serialize()) - return rawTx - } - - this.signMsg = function(address, message){ - // sign message - var privKeyHex = this.exportPrivateKey(address) - var privKey = ethUtil.toBuffer(privKeyHex) - var msgBuffer = new Buffer(message.replace('0x',''), 'hex') - var msgSig = ethUtil.ecsign(msgBuffer, privKey) - var rawMsgSig = ethUtil.bufferToHex(concatSig(msgSig.v, msgSig.r, msgSig.s)) - return rawMsgSig - } - - this.getSeed = function(){ - return this.keyStore.getSeed(this.derivedKey) - } - - this.exportPrivateKey = function(address) { - var privKeyHex = ethUtil.addHexPrefix(this.keyStore.exportPrivateKey(address, this.derivedKey, this.hdPathString)) - return privKeyHex - } -} - - // util function noop(){} - -function pad_with_zeroes(number, length){ - var my_string = '' + number; - while (my_string.length < length) { - my_string = '0' + my_string; - } - return my_string; -} - -function concatSig(v, r, s) { - r = pad_with_zeroes(ethUtil.fromSigned(r), 64) - s = pad_with_zeroes(ethUtil.fromSigned(s), 64) - v = ethUtil.bufferToInt(v) - r = ethUtil.toUnsigned(r).toString('hex') - s = ethUtil.toUnsigned(s).toString('hex') - v = ethUtil.stripHexPrefix(ethUtil.intToHex(v)) - return ethUtil.addHexPrefix(r.concat(s, v).toString("hex")) -} From 6b0a99a09ac9efce4cf54d84e9362ac3b6b35a3d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 15 Jun 2016 14:58:17 -0700 Subject: [PATCH 3/5] Began adding signMsg unit test --- test/unit/id-management-test.js | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 test/unit/id-management-test.js diff --git a/test/unit/id-management-test.js b/test/unit/id-management-test.js new file mode 100644 index 000000000..ca1f0ffaa --- /dev/null +++ b/test/unit/id-management-test.js @@ -0,0 +1,31 @@ +var assert = require('assert') +var IdManagement = require('../../app/scripts/lib/id-management') +var sinon = require('sinon') + +describe('IdManagement', function() { + + beforeEach(function() { + // sinon allows stubbing methods that are easily verified + this.sinon = sinon.sandbox.create() + window.localStorage = {} // Hacking localStorage support into JSDom + }) + + afterEach(function() { + // sinon requires cleanup otherwise it will overwrite context + this.sinon.restore() + }) + + describe('#signMsg', function () { + const address = '0x926cD0393816429a580037475ec23eD65fDC893B' + const message = '0x0987654321abcdef' + const privateKey = '0xd291f7aa01b94941b446f260bca42c0752762571428ad4ed6239613c66365cf4' + const expectedResult = 'foo' + + const idManagement = new IdManagement() + const exportKeyStub = sinon.stub(idManagement, 'exportPrivateKey', () => privateKey) + + const result = idManagement.signMsg(address, message) + console.log(result) + assert.equal(result, expectedResult) + }) +}) From 408addb1b2a795515149dbde8131fa40cba721f4 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 16 Jun 2016 11:46:35 -0700 Subject: [PATCH 4/5] Fixed signing of hashes Signing now always takes a 64 digit hex string, and returns a message signature which appropriately pads r, s, and v with zeroes. Need to verify with Denis that this is the behavior he requires. --- app/scripts/lib/id-management.js | 23 ++++++++++------------- test/unit/id-management-test.js | 10 ++++++---- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/app/scripts/lib/id-management.js b/app/scripts/lib/id-management.js index 0edd2e336..30a3141f1 100644 --- a/app/scripts/lib/id-management.js +++ b/app/scripts/lib/id-management.js @@ -39,16 +39,14 @@ function IdManagement(opts) { return rawTx } - this.signMsg = function(address, message){ + this.signMsg = function (address, message) { // sign message - var privKeyHex = this.exportPrivateKey(address) - var privKey = ethUtil.toBuffer(privKeyHex) - var msgHash = ethUtil.sha3(message) - var msgBuffer = new Buffer(message.replace('0x',''), 'hex') - var msgSig = ethUtil.ecsign(msgBuffer, privKey) - var rawMsgSig = ethUtil.bufferToHex(concatSig(msgSig.v, msgSig.r, msgSig.s)) - return rawMsgSig - } + var privKeyHex = this.exportPrivateKey(address); + var privKey = ethUtil.toBuffer(privKeyHex); + var msgSig = ethUtil.ecsign(new Buffer(message.replace('0x',''), 'hex'), privKey); + var rawMsgSig = ethUtil.bufferToHex(concatSig(msgSig.v, msgSig.r, msgSig.s)); + return rawMsgSig; + }; this.getSeed = function(){ return this.keyStore.getSeed(this.derivedKey) @@ -71,9 +69,8 @@ function pad_with_zeroes(number, length){ function concatSig(v, r, s) { r = pad_with_zeroes(ethUtil.fromSigned(r), 64) s = pad_with_zeroes(ethUtil.fromSigned(s), 64) - v = ethUtil.bufferToInt(v) - r = ethUtil.toUnsigned(r).toString('hex') - s = ethUtil.toUnsigned(s).toString('hex') + r = ethUtil.stripHexPrefix(r.toString('hex')) + s = ethUtil.stripHexPrefix(s.toString('hex')) v = ethUtil.stripHexPrefix(ethUtil.intToHex(v)) - return ethUtil.addHexPrefix(r.concat(s, v).toString("hex")) + return ethUtil.addHexPrefix(r.concat(s, v)) } diff --git a/test/unit/id-management-test.js b/test/unit/id-management-test.js index ca1f0ffaa..61cdf4d8b 100644 --- a/test/unit/id-management-test.js +++ b/test/unit/id-management-test.js @@ -17,15 +17,17 @@ describe('IdManagement', function() { describe('#signMsg', function () { const address = '0x926cD0393816429a580037475ec23eD65fDC893B' - const message = '0x0987654321abcdef' + const message = '0x96b8d442f4c09a08d266bf37b18219465cfb341c1b3ab9792a6103a93583fdf7' const privateKey = '0xd291f7aa01b94941b446f260bca42c0752762571428ad4ed6239613c66365cf4' - const expectedResult = 'foo' + const expectedResult = '0x04881196121781472543750166203264808665659193717384627772472141185319786561270240926993050673320157359365329096037150419976876479876332927284781689204045461c' const idManagement = new IdManagement() - const exportKeyStub = sinon.stub(idManagement, 'exportPrivateKey', () => privateKey) + const exportKeyStub = sinon.stub(idManagement, 'exportPrivateKey', (addr) => { + assert.equal(addr, address) + return privateKey + }) const result = idManagement.signMsg(address, message) - console.log(result) assert.equal(result, expectedResult) }) }) From fe83a19c0847b8d4ca051bbd4d37e13d9d7bdd39 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 16 Jun 2016 11:48:02 -0700 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f498f2fa..e4b1d594b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ - Remove nonfunctional QR code button. - Make network loading indicator clickable to select accessible network. - Show more characters of addresses when space permits. -- Fixed bug when signing messages under 64 hex characters long. +- Fixed eth.sign behavior. ## 2.3.1 2016-06-09