[security] resharing: wait for final acks from the new committee befo…

…re ending (#75)

This is the fix for a vulnerability reported by Omer Shlomovits of KZen Networks (ZenGo).

It adds a final ack round to the re-sharing protocol where the new committee sends acks to members of both the old and new committees before they save any data to disk.

Other Changes:

* readme: mention the UpdateFromBytes bool arg changes, misc edits

* resharing: edit a comment in round 4

* remove the confusing to committee bools

* resharing: remove a redundant line in round 5

README.md

@@ -13,7 +13,7 @@ Permissively MIT Licensed.
 Note! This is a library for developers. You may find a TSS tool that you can use with the Binance Chain CLI [here](https://docs.binance.org/tss.html).
 
 ## Introduction
-This is an implementation of multi-party {t,n}-threshold ECDSA (elliptic curve digital signatures) based on Gennaro and Goldfeder CCS 2018 protocol [\[1\]](#references)
+This is an implementation of multi-party {t,n}-threshold ECDSA (elliptic curve digital signatures) based on Gennaro and Goldfeder CCS 2018 [\[1\]](#references)
 
 This library includes three protocols:
 
@@ -25,7 +25,7 @@ This library includes three protocols:
 
 ## Rationale
 ECDSA is used extensively for crypto-currencies such as Bitcoin, Ethereum (secp256k1 curve), NEO (NIST P-256 curve) and many more. 
-For such currencies this technique may be used to create crypto wallets where multiple participants must collaborate to sign transactions. See [MultiSig Use Cases](https://en.bitcoin.it/wiki/Multisignature#Multisignature_Applications)
+For such currencies this technique may be used to create crypto wallets where multiple parties must collaborate to sign transactions. See [MultiSig Use Cases](https://en.bitcoin.it/wiki/Multisignature#Multisignature_Applications)
 
 One secret share per key/address is stored locally by each participant and these are kept safe by the protocol – they are never revealed to others at any time. Moreover, there is no trusted dealer of the shares.
 
@@ -99,15 +99,17 @@ go func() {
 }()
 ```
 
+⚠️ During re-sharing the key data may be modified during the rounds. Do not ever overwrite any data saved on disk until the final struct has been received through the `end` channel.
+
 ## Messaging
 In these examples the `outCh` will collect outgoing messages from the party and the `endCh` will receive save data or a signature when the protocol is complete.
 
 During the protocol you should provide the party with updates received from other participating parties on the network.
 
-A `Party` has two thread-safe methods on it for receiving updates:
+A `Party` has two thread-safe methods on it for receiving updates. Note that the last two booleans are only used in re-sharing.
 ```go
 // The main entry point when updating a party's state from the wire
-UpdateFromBytes(wireBytes []byte, from *tss.PartyID, isBroadcast, isToOldCommittee bool) (ok bool, err *tss.Error)
+UpdateFromBytes(wireBytes []byte, from *tss.PartyID, isBroadcast bool) (ok bool, err *tss.Error)
 // You may use this entry point to update a party's state when running locally or in tests
 Update(msg tss.ParsedMessage) (ok bool, err *tss.Error)
 ```

ecdsa/keygen/local_party.go

@@ -117,12 +117,13 @@ func NewLocalParty(
 		end:       end,
 	}
 	// msgs init
-	p.temp.KGCs = make([]cmt.HashCommitment, partyCount)
 	p.temp.kgRound1Messages = make([]tss.ParsedMessage, partyCount)
 	p.temp.kgRound2Message1s = make([]tss.ParsedMessage, partyCount)
 	p.temp.kgRound2Message2s = make([]tss.ParsedMessage, partyCount)
 	p.temp.kgRound3Messages = make([]tss.ParsedMessage, partyCount)
-	// data init
+	// temp data init
+	p.temp.KGCs = make([]cmt.HashCommitment, partyCount)
+	// save data init
 	p.data.BigXj = make([]*crypto.ECPoint, partyCount)
 	p.data.PaillierPKs = make([]*paillier.PublicKey, partyCount)
 	p.data.NTildej = make([]*big.Int, partyCount)
@@ -142,8 +143,8 @@ func (p *LocalParty) Update(msg tss.ParsedMessage) (ok bool, err *tss.Error) {
 	return tss.BaseUpdate(p, msg, "keygen")
 }
 
-func (p *LocalParty) UpdateFromBytes(wireBytes []byte, from *tss.PartyID, isBroadcast, isToOldCommittee bool) (bool, *tss.Error) {
-	msg, err := tss.ParseWireMessage(wireBytes, from, isBroadcast, isToOldCommittee)
+func (p *LocalParty) UpdateFromBytes(wireBytes []byte, from *tss.PartyID, isBroadcast bool) (bool, *tss.Error) {
+	msg, err := tss.ParseWireMessage(wireBytes, from, isBroadcast)
 	if err != nil {
 		return false, p.WrapError(err)
 	}

ecdsa/resharing/local_party.go

@@ -8,8 +8,10 @@ package resharing
 
 import (
 	"fmt"
+	"math/big"
 
 	"github.com/binance-chain/tss-lib/common"
+	"github.com/binance-chain/tss-lib/crypto"
 	cmt "github.com/binance-chain/tss-lib/crypto/commitments"
 	"github.com/binance-chain/tss-lib/crypto/vss"
 	"github.com/binance-chain/tss-lib/ecdsa/keygen"
@@ -39,7 +41,8 @@ type (
 		dgRound2Message1s,
 		dgRound2Message2s,
 		dgRound3Message1s,
-		dgRound3Message2s []tss.ParsedMessage
+		dgRound3Message2s,
+		dgRound4Messages []tss.ParsedMessage
 	}
 
 	localTempData struct {
@@ -49,6 +52,11 @@ type (
 		NewVs     vss.Vs
 		NewShares vss.Shares
 		VD        cmt.HashDeCommitment
+
+		// temporary storage of data that is persisted by the new party in round 5 if all "ACK" messages are received
+		newXi     *big.Int
+		newKs     []*big.Int
+		newBigXjs []*crypto.ECPoint // Xj to save in round 5
 	}
 )
 
@@ -76,6 +84,7 @@ func NewLocalParty(
 	p.temp.dgRound2Message2s = make([]tss.ParsedMessage, params.NewPartyCount()) // "
 	p.temp.dgRound3Message1s = make([]tss.ParsedMessage, params.Threshold()+1)   // from t+1 of Old Committee
 	p.temp.dgRound3Message2s = make([]tss.ParsedMessage, params.Threshold()+1)   // "
+	p.temp.dgRound4Messages = make([]tss.ParsedMessage, params.NewPartyCount())  // from n of New Committee
 	return p
 }
 
@@ -91,8 +100,8 @@ func (p *LocalParty) Update(msg tss.ParsedMessage) (ok bool, err *tss.Error) {
 	return tss.BaseUpdate(p, msg, "resharing")
 }
 
-func (p *LocalParty) UpdateFromBytes(wireBytes []byte, from *tss.PartyID, isBroadcast, isToOldCommittee bool) (bool, *tss.Error) {
-	msg, err := tss.ParseWireMessage(wireBytes, from, isBroadcast, isToOldCommittee)
+func (p *LocalParty) UpdateFromBytes(wireBytes []byte, from *tss.PartyID, isBroadcast bool) (bool, *tss.Error) {
+	msg, err := tss.ParseWireMessage(wireBytes, from, isBroadcast)
 	if err != nil {
 		return false, p.WrapError(err)
 	}
@@ -124,6 +133,9 @@ func (p *LocalParty) StoreMessage(msg tss.ParsedMessage) (bool, *tss.Error) {
 	case *DGRound3Message2:
 		p.temp.dgRound3Message2s[fromPIdx] = msg
 
+	case *DGRound4Message:
+		p.temp.dgRound4Messages[fromPIdx] = msg
+
 	default: // unrecognised message, just ignore!
 		common.Logger.Warningf("unrecognised message ignored: %v", msg)
 		return false, nil

ecdsa/resharing/local_party_test.go

@@ -43,32 +43,32 @@ func TestE2EConcurrent(t *testing.T) {
 	// tss.SetCurve(elliptic.P256())
 
 	threshold, newThreshold := testThreshold, testThreshold
-	pIDs := tss.GenerateTestPartyIDs(testParticipants)
+	oldPIDs := tss.GenerateTestPartyIDs(testParticipants)
 
 	// PHASE: load keygen fixtures
 	keys, err := keygen.LoadKeygenTestFixtures(testParticipants)
 	assert.NoError(t, err, "should load keygen fixtures")
 
 	// PHASE: resharing
-	pIDs = pIDs[:threshold+1] // always resharing with old_t+1
-	p2pCtx := tss.NewPeerContext(pIDs)
-	newPIDs := tss.GenerateTestPartyIDs(testParticipants) // new group (start from new index)
+	oldPIDs = oldPIDs[:threshold+1] // always resharing with old_t+1
+	oldP2PCtx := tss.NewPeerContext(oldPIDs)
+	newPIDs := tss.GenerateTestPartyIDs(testParticipants)
 	newP2PCtx := tss.NewPeerContext(newPIDs)
 	newPCount := len(newPIDs)
 
-	oldCommittee := make([]*LocalParty, 0, len(pIDs))
+	oldCommittee := make([]*LocalParty, 0, len(oldPIDs))
 	newCommittee := make([]*LocalParty, 0, newPCount)
 	bothCommitteesPax := len(oldCommittee) + len(newCommittee)
 
 	errCh := make(chan *tss.Error, bothCommitteesPax)
 	outCh := make(chan tss.Message, bothCommitteesPax)
-	endCh := make(chan keygen.LocalPartySaveData, len(newCommittee))
+	endCh := make(chan keygen.LocalPartySaveData, bothCommitteesPax)
 
 	updater := test.SharedPartyUpdater
 
 	// init the old parties first
-	for i, pID := range pIDs {
-		params := tss.NewReSharingParameters(p2pCtx, newP2PCtx, pID, testParticipants, threshold, newPCount, newThreshold)
+	for i, pID := range oldPIDs {
+		params := tss.NewReSharingParameters(oldP2PCtx, newP2PCtx, pID, testParticipants, threshold, newPCount, newThreshold)
 		keyI := keygen.LocalPartySaveData{
 			LocalPreParams: keygen.LocalPreParams{
 				PaillierSK: keys[i].PaillierSK,
@@ -88,7 +88,7 @@ func TestE2EConcurrent(t *testing.T) {
 			Ks:          keys[i].Ks[:testThreshold+1],
 			ECDSAPub:    keys[i].ECDSAPub,
 		}
-		P := NewLocalParty(params, keyI, outCh, nil).(*LocalParty) // discard old key data
+		P := NewLocalParty(params, keyI, outCh, endCh).(*LocalParty) // discard old key data
 		oldCommittee = append(oldCommittee, P)
 	}
 	// init the new parties; re-use the fixture pre-params for speed
@@ -97,7 +97,7 @@ func TestE2EConcurrent(t *testing.T) {
 		common.Logger.Info("No test fixtures were found, so the safe primes will be generated from scratch. This may take a while...")
 	}
 	for i, pID := range newPIDs {
-		params := tss.NewReSharingParameters(p2pCtx, newP2PCtx, pID, testParticipants, threshold, newPCount, newThreshold)
+		params := tss.NewReSharingParameters(oldP2PCtx, newP2PCtx, pID, testParticipants, threshold, newPCount, newThreshold)
 		save := keygen.LocalPartySaveData{
 			BigXj:       make([]*crypto.ECPoint, newPCount),
 			PaillierPKs: make([]*paillier.PublicKey, newPCount),
@@ -140,23 +140,29 @@ func TestE2EConcurrent(t *testing.T) {
 
 		case msg := <-outCh:
 			dest := msg.GetTo()
-			destParties := newCommittee
-			if msg.IsToOldCommittee() {
-				destParties = oldCommittee
-			}
 			if dest == nil {
 				t.Fatal("did not expect a msg to have a nil destination during resharing")
 			}
-			for _, destP := range dest {
-				go updater(destParties[destP.Index], msg, errCh)
+			if msg.IsToOldCommittee() || msg.IsToOldAndNewCommittees() {
+				for _, destP := range dest[:len(oldCommittee)] {
+					go updater(oldCommittee[destP.Index], msg, errCh)
+				}
+			}
+			if !msg.IsToOldCommittee() || msg.IsToOldAndNewCommittees() {
+				for _, destP := range dest {
+					go updater(newCommittee[destP.Index], msg, errCh)
+				}
 			}
 
 		case save := <-endCh:
 			index, err := save.OriginalIndex()
 			assert.NoErrorf(t, err, "should not be an error getting a party's index from save data")
-			keys[index] = save
+			// old committee members that aren't receiving a share have their Xi zeroed
+			if save.Xi.Cmp(big.NewInt(0)) != 0 {
+				keys[index] = save
+			}
 			atomic.AddInt32(&reSharingEnded, 1)
-			if atomic.LoadInt32(&reSharingEnded) == int32(len(newCommittee)) {
+			if atomic.LoadInt32(&reSharingEnded) == int32(len(oldCommittee)+len(newCommittee)) {
 				t.Logf("Resharing done. Reshared %d participants", reSharingEnded)
 
 				// xj tests: BigXj == xj*G

ecdsa/resharing/messages.go

@@ -208,3 +208,24 @@ func (m *DGRound3Message2) UnmarshalVDeCommitment() cmt.HashDeCommitment {
 	deComBzs := m.GetVDecommitment()
 	return cmt.NewHashDeCommitmentFromBytes(deComBzs)
 }
+
+// ----- //
+
+func NewDGRound4Message(
+	to []*tss.PartyID,
+	from *tss.PartyID,
+) tss.ParsedMessage {
+	meta := tss.MessageRouting{
+		From:                    from,
+		To:                      to,
+		IsBroadcast:             true,
+		IsToOldAndNewCommittees: true,
+	}
+	content := &DGRound4Message{}
+	msg := tss.NewMessageWrapper(meta, content)
+	return tss.NewMessage(meta, content, msg)
+}
+
+func (m *DGRound4Message) ValidateBasic() bool {
+	return true
+}

ecdsa/resharing/round_2_new_step_1.go

@@ -78,7 +78,7 @@ func (round *round2) CanAccept(msg tss.ParsedMessage) bool {
 	}
 	if round.ReSharingParams().IsOldCommittee() {
 		if _, ok := msg.Content().(*DGRound2Message2); ok {
-			return msg.IsBroadcast() && msg.IsToOldCommittee()
+			return msg.IsBroadcast()
 		}
 	}
 	return false