[FAB-3530] Gossip - add block seq# validation

When a peer receives a data message (contains a block) it doesn't verify that the sequence of the block matches the sequence inside the marshaled block, and just calls VerifyBlock. The implementation already unmarshals the block, so there is no point in un-marshalling it outside. I extended the interface VerifyBlock to accept also a sequence number. Change-Id: I85c6c57f26f5e404fe8e7ece2abffa58eaded625 Signed-off-by: Yacov Manevich <yacovm@il.ibm.com>
hyperledger · May 1, 2017 · d21cd6d · d21cd6d
1 parent 1195f2f
commit d21cd6d
Show file tree

Hide file tree

Showing 17 changed files with 50 additions and 36 deletions.
diff --git a/core/deliverservice/blocksprovider/blocksprovider.go b/core/deliverservice/blocksprovider/blocksprovider.go
@@ -135,7 +135,7 @@ func (b *blocksProviderImpl) DeliverBlocks() {
 				logger.Errorf("Error serializing block with sequence number %d, due to %s", seqNum, err)
 				continue
 			}
-			if err := b.mcs.VerifyBlock(gossipcommon.ChainID(b.chainID), marshaledBlock); err != nil {
+			if err := b.mcs.VerifyBlock(gossipcommon.ChainID(b.chainID), seqNum, marshaledBlock); err != nil {
 				logger.Errorf("Error verifying block with sequnce number %d, due to %s", seqNum, err)
 				continue
 			}

diff --git a/core/deliverservice/blocksprovider/blocksprovider_test.go b/core/deliverservice/blocksprovider/blocksprovider_test.go
@@ -39,7 +39,7 @@ func (*mockMCS) GetPKIidOfCert(peerIdentity api.PeerIdentityType) common2.PKIidT
 	return common2.PKIidType("pkiID")
 }
 
-func (m *mockMCS) VerifyBlock(chainID common2.ChainID, signedBlock []byte) error {
+func (m *mockMCS) VerifyBlock(chainID common2.ChainID, seqNum uint64, signedBlock []byte) error {
 	args := m.Called()
 	if args.Get(0) != nil {
 		return args.Get(0).(error)

diff --git a/core/deliverservice/deliveryclient_test.go b/core/deliverservice/deliveryclient_test.go
@@ -60,7 +60,7 @@ func (*mockMCS) GetPKIidOfCert(peerIdentity api.PeerIdentityType) common.PKIidTy
 	return common.PKIidType("pkiID")
 }
 
-func (*mockMCS) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (*mockMCS) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/api/crypto.go b/gossip/api/crypto.go
@@ -33,9 +33,10 @@ type MessageCryptoService interface {
 	// This validation is supposed to be done appropriately during the execution flow.
 	GetPKIidOfCert(peerIdentity PeerIdentityType) common.PKIidType
 
-	// VerifyBlock returns nil if the block is properly signed,
+	// VerifyBlock returns nil if the block is properly signed, and the claimed seqNum is the
+	// sequence number that the block's header contains.
 	// else returns error
-	VerifyBlock(chainID common.ChainID, signedBlock []byte) error
+	VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error
 
 	// Sign signs msg with this peer's signing key and outputs
 	// the signature if no error occurred.

diff --git a/gossip/comm/comm_test.go b/gossip/comm/comm_test.go
@@ -72,7 +72,7 @@ func (*naiveSecProvider) GetPKIidOfCert(peerIdentity api.PeerIdentityType) commo
 
 // VerifyBlock returns nil if the block is properly signed,
 // else returns error
-func (*naiveSecProvider) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (*naiveSecProvider) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/gossip/channel/channel.go b/gossip/gossip/channel/channel.go
@@ -522,11 +522,14 @@ func (gc *gossipChannel) verifyBlock(msg *proto.GossipMessage, sender common.PKI
 		gc.logger.Warning("Received from ", sender, "a DataUpdate message that contains a non-block GossipMessage:", msg)
 		return false
 	}
-	if msg.GetDataMsg().Payload == nil {
+	payload := msg.GetDataMsg().Payload
+	if payload == nil {
 		gc.logger.Warning("Received empty payload from", sender)
 		return false
 	}
-	err := gc.mcs.VerifyBlock(msg.Channel, msg.GetDataMsg().Payload.Data)
+	seqNum := payload.SeqNum
+	rawBlock := payload.Data
+	err := gc.mcs.VerifyBlock(msg.Channel, seqNum, rawBlock)
 	if err != nil {
 		gc.logger.Warning("Received fabricated block from", sender, "in DataUpdate:", err)
 		return false

diff --git a/gossip/gossip/channel/channel_test.go b/gossip/gossip/channel/channel_test.go
@@ -125,7 +125,7 @@ func (cs *cryptoService) VerifyByChannel(channel common.ChainID, identity api.Pe
 	return args.Get(0).(error)
 }
 
-func (cs *cryptoService) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (cs *cryptoService) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	args := cs.Called(signedBlock)
 	if args.Get(0) == nil {
 		return nil

diff --git a/gossip/gossip/gossip_impl.go b/gossip/gossip/gossip_impl.go
@@ -431,7 +431,9 @@ func (g *gossipServiceImpl) validateMsg(msg proto.ReceivedMessage) bool {
 			return true
 		}
 
-		if err := g.mcs.VerifyBlock(msg.GetGossipMessage().Channel, blockMsg.Payload.Data); err != nil {
+		seqNum := blockMsg.Payload.SeqNum
+		rawBlock := blockMsg.Payload.Data
+		if err := g.mcs.VerifyBlock(msg.GetGossipMessage().Channel, seqNum, rawBlock); err != nil {
 			g.logger.Warning("Could not verify block", blockMsg.Payload.SeqNum, ":", err)
 			return false
 		}

diff --git a/gossip/gossip/gossip_test.go b/gossip/gossip/gossip_test.go
@@ -158,7 +158,7 @@ func (*naiveCryptoService) GetPKIidOfCert(peerIdentity api.PeerIdentityType) com
 
 // VerifyBlock returns nil if the block is properly signed,
 // else returns error
-func (*naiveCryptoService) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (*naiveCryptoService) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/gossip/orgs_test.go b/gossip/gossip/orgs_test.go
@@ -77,7 +77,7 @@ func (*configurableCryptoService) GetPKIidOfCert(peerIdentity api.PeerIdentityTy
 
 // VerifyBlock returns nil if the block is properly signed,
 // else returns error
-func (*configurableCryptoService) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (*configurableCryptoService) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/identity/identity_test.go b/gossip/identity/identity_test.go
@@ -54,7 +54,7 @@ func (*naiveCryptoService) GetPKIidOfCert(peerIdentity api.PeerIdentityType) com
 
 // VerifyBlock returns nil if the block is properly signed,
 // else returns error
-func (*naiveCryptoService) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (*naiveCryptoService) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/integration/integration_test.go b/gossip/integration/integration_test.go
@@ -119,7 +119,7 @@ func (s *cryptoService) GetPKIidOfCert(peerIdentity api.PeerIdentityType) common
 	return common.PKIidType(peerIdentity)
 }
 
-func (s *cryptoService) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (s *cryptoService) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/service/gossip_service_test.go b/gossip/service/gossip_service_test.go
@@ -673,7 +673,7 @@ func (*naiveCryptoService) GetPKIidOfCert(peerIdentity api.PeerIdentityType) gos
 
 // VerifyBlock returns nil if the block is properly signed,
 // else returns error
-func (*naiveCryptoService) VerifyBlock(chainID gossipCommon.ChainID, signedBlock []byte) error {
+func (*naiveCryptoService) VerifyBlock(chainID gossipCommon.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/gossip/state/state.go b/gossip/state/state.go
@@ -366,7 +366,7 @@ func (s *GossipStateProviderImpl) handleStateResponse(msg proto.ReceivedMessage)
 	}
 	for _, payload := range response.GetPayloads() {
 		logger.Debugf("Received payload with sequence number %d.", payload.SeqNum)
-		if err := s.mcs.VerifyBlock(common2.ChainID(s.chainID), payload.Data); err != nil {
+		if err := s.mcs.VerifyBlock(common2.ChainID(s.chainID), payload.SeqNum, payload.Data); err != nil {
 			logger.Warningf("Error verifying block with sequence number %d, due to %s", payload.SeqNum, err)
 			return uint64(0), err
 		}

diff --git a/gossip/state/state_test.go b/gossip/state/state_test.go
@@ -106,7 +106,7 @@ func (*cryptoServiceMock) GetPKIidOfCert(peerIdentity api.PeerIdentityType) comm
 
 // VerifyBlock returns nil if the block is properly signed,
 // else returns error
-func (*cryptoServiceMock) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (*cryptoServiceMock) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	return nil
 }
 

diff --git a/peer/gossip/mcs.go b/peer/gossip/mcs.go
@@ -113,9 +113,10 @@ func (s *mspMessageCryptoService) GetPKIidOfCert(peerIdentity api.PeerIdentityTy
 	return digest
 }
 
-// VerifyBlock returns nil if the block is properly signed,
+// VerifyBlock returns nil if the block is properly signed, and the claimed seqNum is the
+// sequence number that the block's header contains.
 // else returns error
-func (s *mspMessageCryptoService) VerifyBlock(chainID common.ChainID, signedBlock []byte) error {
+func (s *mspMessageCryptoService) VerifyBlock(chainID common.ChainID, seqNum uint64, signedBlock []byte) error {
 	// - Convert signedBlock to common.Block.
 	block, err := utils.GetBlockFromBlockBytes(signedBlock)
 	if err != nil {
@@ -126,6 +127,11 @@ func (s *mspMessageCryptoService) VerifyBlock(chainID common.ChainID, signedBloc
 		return fmt.Errorf("Invalid Block on channel [%s]. Header must be different from nil.", chainID)
 	}
 
+	blockSeqNum := block.Header.Number
+	if seqNum != blockSeqNum {
+		return fmt.Errorf("Claimed seqNum is [%d] but actual seqNum inside block is [%d]", seqNum, blockSeqNum)
+	}
+
 	// - Extract channelID and compare with chainID
 	channelID, err := utils.GetChainIDFromBlock(block)
 	if err != nil {

diff --git a/peer/gossip/mcs_test.go b/peer/gossip/mcs_test.go
@@ -17,10 +17,8 @@ limitations under the License.
 package gossip
 
 import (
-	"fmt"
-	"testing"
-
 	"reflect"
+	"testing"
 
 	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric/bccsp"
@@ -152,7 +150,7 @@ func TestVerify(t *testing.T) {
 	assert.NoError(t, err)
 	err = msgCryptoService.Verify(api.PeerIdentityType("Dave"), sigma, msg)
 	assert.Error(t, err)
-	assert.Contains(t, fmt.Sprintf("%v", err), "Could not acquire policy manager")
+	assert.Contains(t, err.Error(), "Could not acquire policy manager")
 
 	// Check invalid args
 	assert.Error(t, msgCryptoService.Verify(nil, sigma, msg))
@@ -182,34 +180,38 @@ func TestVerifyBlock(t *testing.T) {
 	)
 
 	// - Prepare testing valid block, Alice signs it.
-	blockRaw, msg := mockBlock(t, "C", aliceSigner, nil)
+	blockRaw, msg := mockBlock(t, "C", 42, aliceSigner, nil)
 	policyManagerGetter.Managers["C"].(*mocks.ChannelPolicyManager).Policy.(*mocks.Policy).Deserializer.(*mocks.IdentityDeserializer).Msg = msg
-	blockRaw2, msg2 := mockBlock(t, "D", aliceSigner, nil)
+	blockRaw2, msg2 := mockBlock(t, "D", 42, aliceSigner, nil)
 	policyManagerGetter.Managers["D"].(*mocks.ChannelPolicyManager).Policy.(*mocks.Policy).Deserializer.(*mocks.IdentityDeserializer).Msg = msg2
 
 	// - Verify block
-	assert.NoError(t, msgCryptoService.VerifyBlock([]byte("C"), blockRaw))
+	assert.NoError(t, msgCryptoService.VerifyBlock([]byte("C"), 42, blockRaw))
+	// Wrong sequence number claimed
+	err := msgCryptoService.VerifyBlock([]byte("C"), 43, blockRaw)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "but actual seqNum inside block is")
 	delete(policyManagerGetter.Managers, "D")
-	nilPolMgrErr := msgCryptoService.VerifyBlock([]byte("D"), blockRaw2)
-	assert.Contains(t, fmt.Sprintf("%v", nilPolMgrErr), "Could not acquire policy manager")
+	nilPolMgrErr := msgCryptoService.VerifyBlock([]byte("D"), 42, blockRaw2)
+	assert.Contains(t, nilPolMgrErr.Error(), "Could not acquire policy manager")
 	assert.Error(t, nilPolMgrErr)
-	assert.Error(t, msgCryptoService.VerifyBlock([]byte("A"), blockRaw))
-	assert.Error(t, msgCryptoService.VerifyBlock([]byte("B"), blockRaw))
+	assert.Error(t, msgCryptoService.VerifyBlock([]byte("A"), 42, blockRaw))
+	assert.Error(t, msgCryptoService.VerifyBlock([]byte("B"), 42, blockRaw))
 
 	// - Prepare testing invalid block (wrong data has), Alice signs it.
-	blockRaw, msg = mockBlock(t, "C", aliceSigner, []byte{0})
+	blockRaw, msg = mockBlock(t, "C", 42, aliceSigner, []byte{0})
 	policyManagerGetter.Managers["C"].(*mocks.ChannelPolicyManager).Policy.(*mocks.Policy).Deserializer.(*mocks.IdentityDeserializer).Msg = msg
 
 	// - Verify block
-	assert.Error(t, msgCryptoService.VerifyBlock([]byte("C"), blockRaw))
+	assert.Error(t, msgCryptoService.VerifyBlock([]byte("C"), 42, blockRaw))
 
 	// Check invalid args
-	assert.Error(t, msgCryptoService.VerifyBlock([]byte("C"), []byte{0, 1, 2, 3, 4}))
-	assert.Error(t, msgCryptoService.VerifyBlock([]byte("C"), nil))
+	assert.Error(t, msgCryptoService.VerifyBlock([]byte("C"), 42, []byte{0, 1, 2, 3, 4}))
+	assert.Error(t, msgCryptoService.VerifyBlock([]byte("C"), 42, nil))
 }
 
-func mockBlock(t *testing.T, channel string, localSigner crypto.LocalSigner, dataHash []byte) ([]byte, []byte) {
-	block := common.NewBlock(0, nil)
+func mockBlock(t *testing.T, channel string, seqNum uint64, localSigner crypto.LocalSigner, dataHash []byte) ([]byte, []byte) {
+	block := common.NewBlock(seqNum, nil)
 
 	// Add a fake transaction to the block referring channel "C"
 	sProp, _ := utils.MockSignedEndorserProposalOrPanic(channel, &protospeer.ChaincodeSpec{}, []byte("transactor"), []byte("transactor's signature"))