Merge pull request #712 from maraoz/txp/protocol/security2

Improve tx proposal protocol security

js/models/core/PublicKeyRing.js

@@ -3,6 +3,7 @@
 
 
 var imports     = require('soop').imports();
+var preconditions = require('preconditions').instance();
 var bitcore     = require('bitcore');
 var HK          = bitcore.HierarchicalKey;
 var PrivateKey  = require('./PrivateKey');
@@ -62,6 +63,7 @@ PublicKeyRing.prototype.toObj = function() {
 };
 
 PublicKeyRing.prototype.getCopayerId = function(i) {
+  preconditions.checkArgument(typeof i !== 'undefined');
   return this.copayerIds[i];
 };
 

js/models/core/TxProposals.js

@@ -51,9 +51,9 @@ TxProposal.getSentTs = function() {
   return this.sentTs;
 };
 
-TxProposal.prototype.merge = function(other) {
+TxProposal.prototype.merge = function(other, author) {
   var ret = {};
-  ret.events = this.mergeMetadata(other);
+  ret.events = this.mergeMetadata(other, author);
   ret.hasChanged = this.mergeBuilder(other);
   return ret;
 };
@@ -69,7 +69,7 @@ TxProposal.prototype.mergeBuilder = function(other) {
   return after !== before;
 };
 
-TxProposal.prototype.mergeMetadata = function(v1) {
+TxProposal.prototype.mergeMetadata = function(v1, author) {
   var events = [];
   var v0 = this;
 
@@ -77,6 +77,7 @@ TxProposal.prototype.mergeMetadata = function(v1) {
 
   Object.keys(v1.seenBy).forEach(function(k) {
     if (!v0.seenBy[k]) {
+      if (k != author) throw new Error('Non authoritative seenBy change by '+author);
       v0.seenBy[k] = v1.seenBy[k];
       events.push({
         type: 'seen',
@@ -88,6 +89,7 @@ TxProposal.prototype.mergeMetadata = function(v1) {
 
   Object.keys(v1.signedBy).forEach(function(k) {
     if (!v0.signedBy[k]) {
+      if (k != author) throw new Error('Non authoritative signedBy change by '+author);
       v0.signedBy[k] = v1.signedBy[k];
       events.push({
         type: 'signed',
@@ -99,6 +101,7 @@ TxProposal.prototype.mergeMetadata = function(v1) {
 
   Object.keys(v1.rejectedBy).forEach(function(k) {
     if (!v0.rejectedBy[k]) {
+      if (k != author) throw new Error('Non authoritative rejectedBy change by '+author);
       v0.rejectedBy[k] = v1.rejectedBy[k];
       events.push({
         type: 'rejected',
@@ -168,7 +171,7 @@ TxProposals.prototype.toObj = function(onlyThisNtxid) {
   };
 };
 
-TxProposals.prototype.merge = function(inTxp) {
+TxProposals.prototype.merge = function(inTxp, author) {
   var myTxps = this.txps;
 
   var ntxid = inTxp.getID();
@@ -179,7 +182,7 @@ TxProposals.prototype.merge = function(inTxp) {
   if (myTxps[ntxid]) {
     var v0 = myTxps[ntxid];
     var v1 = inTxp;
-    ret = v0.merge(v1);
+    ret = v0.merge(v1, author);
   } else {
     this.txps[ntxid] = inTxp;
     ret.hasChanged = true;
@@ -192,7 +195,13 @@ TxProposals.prototype.merge = function(inTxp) {
   return ret;
 };
 
+var preconditions = require('preconditions').instance();
 TxProposals.prototype.add = function(data) {
+  preconditions.checkArgument(data.inputChainPaths);
+  preconditions.checkArgument(data.signedBy);
+  preconditions.checkArgument(data.creator);
+  preconditions.checkArgument(data.createdTs);
+  preconditions.checkArgument(data.builder);
   var txp = new TxProposal(data);
   var ntxid = txp.getID();
   this.txps[ntxid] = txp;

js/models/core/Wallet.js

@@ -118,13 +118,18 @@ Wallet.prototype._handlePublicKeyRing = function(senderId, data, isInbound) {
 
 
 Wallet.prototype._handleTxProposal = function(senderId, data) {
+  preconditions.checkArgument(senderId);
   this.log('RECV TXPROPOSAL:', data);
 
   var inTxp = TxProposals.TxProposal.fromObj(data.txProposal);
-
+  var mergeInfo = this.txProposals.merge(inTxp, senderId);
-  var mergeInfo = this.txProposals.merge(inTxp);
   var added = this.addSeenToTxProposals();
 
+  if (added) {
+    this.log('### BROADCASTING txProposals with my seenBy updated.');
+    this.sendTxProposal(inTxp.getID());
+  }
+
   this.emit('txProposalsUpdated');
   this.store();
 
@@ -499,6 +504,7 @@ Wallet.prototype.reject = function(ntxid) {
 
 
 Wallet.prototype.sign = function(ntxid, cb) {
+  preconditions.checkState(typeof this.getMyCopayerId() !== 'undefined');
   var self = this;
   setTimeout(function() {
     var myId = self.getMyCopayerId();
@@ -711,7 +717,6 @@ Wallet.prototype.createTxSync = function(toAddress, amountSatStr, comment, utxos
     }]);
 
   var selectedUtxos = b.getSelectedUnspent();
-
   var inputChainPaths = selectedUtxos.map(function(utxo) {
     return pkr.pathForAddress(utxo.address);
   });

test/test.TxProposals.js

@@ -149,16 +149,23 @@ describe('TxProposals model', function() {
         address: toAddress,
         amountSat: amountSat
       }]);
+    var selectedUtxos = b.getSelectedUnspent();
+    var inputChainPaths = selectedUtxos.map(function(utxo) {
+      return pkr.pathForAddress(utxo.address);
+    });
 
     var signRet;
     if (priv) {
-      var pkeys = priv.getAll(pkr.indexes.getReceiveIndex(), pkr.indexes.getChangeIndex());
+      var pkeys = priv.getForPaths(inputChainPaths);
       b.sign(pkeys);
     }
     var me = {};
-    if (priv) me[priv.id] = Date.now();
+    if (priv) me[priv.getId()] = Date.now();
 
     return {
+      inputChainPaths: inputChainPaths,
+      creator: priv.getId(),
+      createdTs: new Date(),
       signedBy: priv && b.signaturesAdded ? me : {},
       seenBy: priv ? me : {},
       builder: b,
@@ -216,10 +223,11 @@ describe('TxProposals model', function() {
     tx.isComplete().should.equal(false);
     tx.countInputMissingSignatures(0).should.equal(2);
 
-    (w.txps[ntxid].signedBy[priv.id] - ts > 0).should.equal(true);
+    var x = priv.getId();
+    (w.txps[ntxid].signedBy[priv.getId()] - ts > 0).should.equal(true);
     (w.txps[ntxid].seenBy[priv.id] - ts > 0).should.equal(true);
 
-    var info = w.merge(w.txps[ntxid]);
+    var info = w.merge(w.txps[ntxid], pkr.getCopayerId(0));
     info.events.length.should.equal(0);
 
     Object.keys(w.txps).length.should.equal(1);
@@ -293,9 +301,10 @@ describe('TxProposals model', function() {
     (w2.txps[ntxid].signedBy[priv.id] - ts > 0).should.equal(true);
     (w2.txps[ntxid].seenBy[priv.id] - ts > 0).should.equal(true);
 
-    var info = w.merge(w2.txps[ntxid]);
+    var info = w.merge(w2.txps[ntxid], pkr.getCopayerId(0));
-    info.events.length.should.equal(1);
+    info.events.length.should.equal(2);
-    info.events[0].type.should.equal('signed');
+    info.events[0].type.should.equal('seen');
+    info.events[1].type.should.equal('signed');
 
     Object.keys(w.txps).length.should.equal(1);
 
@@ -401,9 +410,10 @@ describe('TxProposals model', function() {
     (w2.txps[ntxid].signedBy[priv.id] - ts > 0).should.equal(true);
     (w2.txps[ntxid].seenBy[priv.id] - ts > 0).should.equal(true);
 
-    var info = w.merge(w2.txps[ntxid]);
+    var info = w.merge(w2.txps[ntxid], pkr.getCopayerId(0));
-    info.events.length.should.equal(1);
+    info.events.length.should.equal(2);
-    info.events[0].type.should.equal('signed');
+    info.events[0].type.should.equal('seen');
+    info.events[1].type.should.equal('signed');
 
     tx = w.txps[ntxid].builder.build();
     tx.isComplete().should.equal(false);
@@ -431,8 +441,7 @@ describe('TxProposals model', function() {
     (w3.txps[ntxid].signedBy[priv2.id] - ts > 0).should.equal(true);
     (w3.txps[ntxid].seenBy[priv2.id] - ts > 0).should.equal(true);
 
-    var info = w.merge(w3.txps[ntxid]);
+    var info = w.merge(w3.txps[ntxid], pkr.getCopayerId(1));
-    info.events.length.should.equal(0);
 
     Object.keys(w.txps).length.should.equal(1);
 
@@ -522,8 +531,7 @@ describe('TxProposals model', function() {
     (w3.txps[ntxid].signedBy[priv3.id] - ts > 0).should.equal(true);
     (w3.txps[ntxid].seenBy[priv3.id] - ts > 0).should.equal(true);
 
-    var info = w.merge(w2.txps[ntxid]);
+    var info = w.merge(w2.txps[ntxid], pkr.getCopayerId(1));
-    info.events.length.should.equal(0);
 
     Object.keys(w.txps).length.should.equal(1);
     var tx = w.txps[ntxid].builder.build();
@@ -535,8 +543,7 @@ describe('TxProposals model', function() {
     (w.txps[ntxid].signedBy[priv2.id] - ts > 0).should.equal(true);
 
 
-    var info = w.merge(w3.txps[ntxid]);
+    var info = w.merge(w3.txps[ntxid], pkr.getCopayerId(2));
-    info.events.length.should.equal(0);
 
     var tx = w.txps[ntxid].builder.build();
     tx.isComplete().should.equal(true);
@@ -601,7 +608,7 @@ describe('TxProposals model', function() {
     should.exist(w2.txps[ntxid].builder);
     should.exist(w2.txps[ntxid].builder.valueInSat);
 
-    w2.merge(w.txps[ntxid]);
+    w2.merge(w.txps[ntxid], pkr.getCopayerId(0));
     Object.keys(w2.txps).length.should.equal(1);
   });
 

test/test.Wallet.js

@@ -67,13 +67,13 @@ describe('Wallet model', function() {
     c.network = new Network(config.network);
     c.blockchain = new Blockchain(config.blockchain);
 
-    c.addressBook =  { 
+    c.addressBook = {
-      '2NFR2kzH9NUdp8vsXTB4wWQtTtzhpKxsyoJ' : {
+      '2NFR2kzH9NUdp8vsXTB4wWQtTtzhpKxsyoJ': {
         label: 'John',
         copayerId: '026a55261b7c898fff760ebe14fd22a71892295f3b49e0ca66727bc0a0d7f94d03',
         createdTs: 1403102115,
-      }, 
+      },
-      '2MtP8WyiwG7ZdVWM96CVsk2M1N8zyfiVQsY' : {
+      '2MtP8WyiwG7ZdVWM96CVsk2M1N8zyfiVQsY': {
         label: 'Jennifer',
         copayerId: '032991f836543a492bd6d0bb112552bfc7c5f3b7d5388fcbcbf2fbb893b44770d7',
         createdTs: 1403103115,
@@ -312,7 +312,7 @@ describe('Wallet model', function() {
     setTimeout(function() {
       sinon.assert.callCount(spy, callCount);
       done();
-    }, w.reconnectDelay*callCount*(callCount+1)/2);
+    }, w.reconnectDelay * callCount * (callCount + 1) / 2);
   });
 
   it('handle network indexes correctly', function() {
@@ -641,7 +641,7 @@ describe('Wallet model', function() {
     var ADDRESSES_RECEIVE = w.deriveAddresses(0, 20, false);
     w.blockchain.checkActivity = function(addresses, cb) {
       var activity = new Array(addresses.length);
-      for(var i=0; i<addresses.length; i++) {
+      for (var i = 0; i < addresses.length; i++) {
         var a1 = ADDRESSES_CHANGE.indexOf(addresses[i]);
         var a2 = ADDRESSES_RECEIVE.indexOf(addresses[i]);
         activity[i] = f(Math.max(a1, a2));
@@ -652,8 +652,10 @@ describe('Wallet model', function() {
 
   it('#indexDiscovery should work without found activities', function(done) {
     var w = createW2();
-    mockFakeActivity(w, function(index) { return false });
+    mockFakeActivity(w, function(index) {
-    w.indexDiscovery(0, false, 5, function(e, lastActive){
+      return false
+    });
+    w.indexDiscovery(0, false, 5, function(e, lastActive) {
       lastActive.should.equal(-1);
       done();
     });
@@ -661,8 +663,10 @@ describe('Wallet model', function() {
 
   it('#indexDiscovery should continue scanning', function(done) {
     var w = createW2();
-    mockFakeActivity(w, function(index) { return index <= 7 });
+    mockFakeActivity(w, function(index) {
-    w.indexDiscovery(0, false, 5, function(e, lastActive){
+      return index <= 7
+    });
+    w.indexDiscovery(0, false, 5, function(e, lastActive) {
       lastActive.should.equal(7);
       done();
     });
@@ -670,8 +674,10 @@ describe('Wallet model', function() {
 
   it('#indexDiscovery should not found beyond the scannWindow', function(done) {
     var w = createW2();
-    mockFakeActivity(w, function(index) { return index <= 10 || index == 17 });
+    mockFakeActivity(w, function(index) {
-    w.indexDiscovery(0, false, 5, function(e, lastActive){
+      return index <= 10 || index == 17
+    });
+    w.indexDiscovery(0, false, 5, function(e, lastActive) {
       lastActive.should.equal(10);
       done();
     });
@@ -679,8 +685,10 @@ describe('Wallet model', function() {
 
   it('#indexDiscovery should look for activity along the scannWindow', function(done) {
     var w = createW2();
-    mockFakeActivity(w, function(index) { return index <= 14  && index % 2 == 0 });
+    mockFakeActivity(w, function(index) {
-    w.indexDiscovery(0, false, 5, function(e, lastActive){
+      return index <= 14 && index % 2 == 0
+    });
+    w.indexDiscovery(0, false, 5, function(e, lastActive) {
       lastActive.should.equal(14);
       done();
     });
@@ -688,7 +696,9 @@ describe('Wallet model', function() {
 
   it('#updateIndexes should update correctly', function(done) {
     var w = createW2();
-    mockFakeActivity(w, function(index) { return index <= 14  && index % 2 == 0 });
+    mockFakeActivity(w, function(index) {
+      return index <= 14 && index % 2 == 0
+    });
     w.updateIndexes(function(err) {
       w.publicKeyRing.indexes.receiveIndex.should.equal(15);
       w.publicKeyRing.indexes.changeIndex.should.equal(15);
@@ -698,7 +708,9 @@ describe('Wallet model', function() {
 
   it('#updateIndexes should store and emit event', function(done) {
     var w = createW2();
-    mockFakeActivity(w, function(index) { return index <= 14  && index % 2 == 0 });
+    mockFakeActivity(w, function(index) {
+      return index <= 14 && index % 2 == 0
+    });
     var spyStore = sinon.spy(w, 'store');
     var spyEmit = sinon.spy(w, 'emit');
     w.updateIndexes(function(err) {
@@ -720,16 +732,13 @@ describe('Wallet model', function() {
     done();
   });
 
-  var contacts = [ 
+  var contacts = [{
-    {
+    label: 'Charles',
-      label: 'Charles',
+    address: '2N8pJWpXCAxmNLHKVEhz3TtTcYCtHd43xWU ',
-      address: '2N8pJWpXCAxmNLHKVEhz3TtTcYCtHd43xWU ',
+  }, {
-    }, 
+    label: 'Linda',
-    {
+    address: '2N4Zq92goYGrf5J4F4SZZq7jnPYbCiyRYT2 ',
-      label: 'Linda',
+  }];
-      address: '2N4Zq92goYGrf5J4F4SZZq7jnPYbCiyRYT2 ',
-    }
-  ];
 
   it('should create new entry for address book', function() {
     var w = createW();
@@ -747,7 +756,7 @@ describe('Wallet model', function() {
     }).should.
     throw();
   });
-  
+
   it('should delete an entry for address book', function() {
     var w = createW();
     contacts.forEach(function(c) {
@@ -763,8 +772,8 @@ describe('Wallet model', function() {
     var w = createW();
     var data = {
       walletId: w.id,
-      addressBook: { 
+      addressBook: {
-        'msj42CCGruhRsFrGATiUuh25dtxYtnpbTx' : {
+        'msj42CCGruhRsFrGATiUuh25dtxYtnpbTx': {
           label: 'Faucet',
           copayerId: '026a55261b7c898fff760ebe14fd22a71892295f3b49e0ca66727bc0a0d7f94d03',
           createdTs: 1403102115,