Add block hash verification

This adds block integrity checks among the skipchain service
when blocks are requested from a peer which means it could
be malicious data.

Fixes #1703

skipchain/api.go

@@ -16,6 +16,11 @@ import (
 	"go.dedis.ch/onet/v3/network"
 )
 
+// mock replies until we have proper interfaces
+var testCorruptSSBResponse *StoreSkipBlockReply
+var testCorruptSkipBlock *SkipBlock
+var testCorruptGSBIReply *GetSingleBlockByIndexReply
+
 // Client is a structure to communicate with the Skipchain
 // service from the outside
 type Client struct {
@@ -81,6 +86,23 @@ func (c *Client) StoreSkipBlockSignature(target *SkipBlock, ro *onet.Roster, d n
 	if err != nil {
 		return nil, err
 	}
+
+	// corrupted response for testing purpose
+	if testCorruptSSBResponse != nil {
+		reply = testCorruptSSBResponse
+	}
+
+	if reply.Latest != nil {
+		if err = reply.Latest.VerifyForwardSignatures(); err != nil {
+			return nil, err
+		}
+	}
+	if reply.Previous != nil {
+		if err = reply.Previous.VerifyForwardSignatures(); err != nil {
+			return nil, err
+		}
+	}
+
 	return reply, nil
 }
 
@@ -224,6 +246,7 @@ func (c *Client) GetUpdateChainLevel(roster *onet.Roster, latest SkipBlockID,
 				}
 			}
 
+			// Check the integrity of the block
 			if err := b.VerifyForwardSignatures(); err != nil {
 				return nil, err
 			}
@@ -297,11 +320,27 @@ func (c *Client) GetAllSkipChainIDs(si *network.ServerIdentity) (reply *GetAllSk
 
 // GetSingleBlock searches for a block with the given ID and returns that block,
 // or an error if that block is not found.
-func (c *Client) GetSingleBlock(roster *onet.Roster, id SkipBlockID) (reply *SkipBlock, err error) {
-	reply = &SkipBlock{}
-	err = c.SendProtobuf(roster.RandomServerIdentity(),
-		&GetSingleBlock{id}, reply)
-	return
+func (c *Client) GetSingleBlock(roster *onet.Roster, id SkipBlockID) (*SkipBlock, error) {
+	var reply = &SkipBlock{}
+	err := c.SendProtobuf(roster.RandomServerIdentity(), &GetSingleBlock{id}, reply)
+	if err != nil {
+		return nil, err
+	}
+
+	// for testing purpose
+	if testCorruptSkipBlock != nil {
+		reply = testCorruptSkipBlock
+	}
+
+	if err = reply.VerifyForwardSignatures(); err != nil {
+		return nil, err
+	}
+
+	if !reply.Hash.Equal(id) {
+		return nil, errors.New("Got the wrong block in return")
+	}
+
+	return reply, nil
 }
 
 // GetSingleBlockByIndex searches for a block with the given index following the genesis-block.
@@ -311,8 +350,7 @@ func (c *Client) GetSingleBlockByIndex(roster *onet.Roster, genesis SkipBlockID,
 	perms := rand.Perm(len(roster.List))
 	var errs []string
 	for _, ind := range perms {
-		err = c.SendProtobuf(roster.List[ind],
-			&GetSingleBlockByIndex{genesis, index}, reply)
+		reply, err = c.getBlockByIndex(roster.List[ind], genesis, index)
 		if err == nil {
 			return
 		}
@@ -321,6 +359,35 @@ func (c *Client) GetSingleBlockByIndex(roster *onet.Roster, genesis SkipBlockID,
 	return nil, errors.New("all nodes failed to return block: " + strings.Join(errs, " :: "))
 }
 
+func (c *Client) getBlockByIndex(si *network.ServerIdentity, genesis SkipBlockID, index int) (reply *GetSingleBlockByIndexReply, err error) {
+	reply = &GetSingleBlockByIndexReply{}
+
+	err = c.SendProtobuf(si, &GetSingleBlockByIndex{genesis, index}, reply)
+	if err != nil {
+		return
+	}
+
+	// for testing purpose
+	if testCorruptGSBIReply != nil {
+		reply = testCorruptGSBIReply
+	}
+
+	if reply.SkipBlock == nil {
+		err = errors.New("Got an empty reply")
+		return
+	}
+
+	if err = reply.SkipBlock.VerifyForwardSignatures(); err != nil {
+		return
+	}
+
+	if reply.SkipBlock.Index != index {
+		err = errors.New("Got the wrong block in reply")
+	}
+
+	return
+}
+
 // CreateLinkPrivate asks the conode to create a link by sending a public
 // key of the client, signed by the private key of the conode. The reasoning is
 // that an administrator should well be able to copy the private.toml-file from

skipchain/api_test.go

@@ -153,6 +153,36 @@ func TestClient_StoreSkipBlock(t *testing.T) {
 	c.Close()
 }
 
+func TestClient_StoreSkipBlockCorrupted(t *testing.T) {
+	defer func() {
+		testCorruptSSBResponse = nil
+	}()
+
+	nbrHosts := 3
+	l := onet.NewTCPTest(cothority.Suite)
+	_, ro, _ := l.GenTree(nbrHosts, true)
+	defer l.CloseAll()
+
+	c := newTestClient(l)
+	genesis, err := c.CreateGenesis(ro, 1, 1, VerificationNone, nil)
+
+	testCorruptSSBResponse = &StoreSkipBlockReply{}
+
+	_, err = c.StoreSkipBlock(genesis, ro, []byte{})
+	require.Nil(t, err) // empty reply
+
+	testCorruptSSBResponse.Previous = NewSkipBlock()
+	_, err = c.StoreSkipBlock(genesis, ro, []byte{})
+	require.NotNil(t, err)
+	require.Equal(t, "Calculated hash does not match", err.Error())
+
+	testCorruptSSBResponse.Previous = nil
+	testCorruptSSBResponse.Latest = NewSkipBlock()
+	_, err = c.StoreSkipBlock(genesis, ro, []byte{})
+	require.NotNil(t, err)
+	require.Equal(t, "Calculated hash does not match", err.Error())
+}
+
 func TestClient_GetAllSkipchains(t *testing.T) {
 	nbrHosts := 3
 	l := onet.NewTCPTest(cothority.Suite)
@@ -211,6 +241,37 @@ func TestClient_GetAllSkipChainIDs(t *testing.T) {
 	}
 }
 
+func TestClient_GetSingleBlock(t *testing.T) {
+	defer func() {
+		testCorruptSkipBlock = nil
+	}()
+
+	nbrHosts := 3
+	l := onet.NewTCPTest(cothority.Suite)
+	_, ro, _ := l.GenTree(nbrHosts, true)
+	defer l.CloseAll()
+
+	c := newTestClient(l)
+
+	sb, err := c.CreateGenesis(ro, 1, 1, VerificationNone, nil)
+	require.Nil(t, err)
+
+	ret, err := c.GetSingleBlock(ro, sb.Hash)
+	require.Nil(t, err)
+	require.Equal(t, ret.Hash, sb.Hash)
+
+	testCorruptSkipBlock = NewSkipBlock()
+	testCorruptSkipBlock.Roster = ro
+	_, err = c.GetSingleBlock(ro, sb.Hash)
+	require.NotNil(t, err)
+	require.Equal(t, "Calculated hash does not match", err.Error())
+
+	testCorruptSkipBlock.updateHash()
+	_, err = c.GetSingleBlock(ro, sb.Hash)
+	require.NotNil(t, err)
+	require.Equal(t, "Got the wrong block in return", err.Error())
+}
+
 func TestClient_GetSingleBlockByIndex(t *testing.T) {
 	nbrHosts := 3
 	l := onet.NewTCPTest(cothority.Suite)
@@ -260,6 +321,37 @@ func TestClient_GetSingleBlockByIndex(t *testing.T) {
 	require.NotNil(t, err)
 }
 
+func TestClient_GetSingleBlockByIndexCorrupted(t *testing.T) {
+	defer func() {
+		testCorruptGSBIReply = nil
+	}()
+
+	nbrHosts := 3
+	l := onet.NewTCPTest(cothority.Suite)
+	_, roster, _ := l.GenTree(nbrHosts, true)
+	defer l.CloseAll()
+
+	c := newTestClient(l)
+
+	sb, err := c.CreateGenesis(roster, 2, 4, VerificationNone, nil)
+	require.Nil(t, err)
+
+	testCorruptGSBIReply = &GetSingleBlockByIndexReply{}
+	_, err = c.GetSingleBlockByIndex(roster, sb.Hash, 0)
+	require.NotNil(t, err)
+	require.Contains(t, err.Error(), "Got an empty reply")
+
+	testCorruptGSBIReply.SkipBlock = NewSkipBlock()
+	_, err = c.GetSingleBlockByIndex(roster, sb.Hash, 0)
+	require.Contains(t, err.Error(), "Calculated hash does not match")
+
+	testCorruptGSBIReply.SkipBlock.Index = 1
+	testCorruptGSBIReply.SkipBlock.Roster = roster
+	testCorruptGSBIReply.SkipBlock.updateHash()
+	_, err = c.GetSingleBlockByIndex(roster, sb.Hash, 0)
+	require.Contains(t, err.Error(), "Got the wrong block in reply")
+}
+
 func TestClient_CreateLinkPrivate(t *testing.T) {
 	ls := linked(1)
 	defer ls.local.CloseAll()

skipchain/skipchain.go

@@ -499,6 +499,9 @@ func (s *Service) getBlocks(roster *onet.Roster, id SkipBlockID, n int) ([]*Skip
 	}
 	select {
 	case result := <-pisc.GetBlocksReply:
+		if err := Proof(result).VerifyFromID(id); err != nil {
+			return nil, err
+		}
 		return result, nil
 	case <-time.After(s.propTimeout):
 		return nil, errors.New("timeout waiting for GetBlocks reply")
@@ -1231,6 +1234,9 @@ func (s *Service) bftForwardLink(msg, data []byte) bool {
 		// Retrieve the src and dst blocks and make sure the basic parameters
 		// are ok.
 		dst := fs.Newest
+		if !dst.CalculateHash().Equal(dst.Hash) {
+			return errors.New("Newest block does not match its hash")
+		}
 		if fs.TargetHeight > len(dst.BackLinkIDs) {
 			return errors.New("Asked for signing too high a backlink")
 		}

skipchain/struct.go

@@ -343,6 +343,16 @@ func NewSkipBlock() *SkipBlock {
 // VerifyForwardSignatures returns whether all signatures in the forward-links
 // are correctly signed by the aggregate public key of the roster.
 func (sb *SkipBlock) VerifyForwardSignatures() error {
+	if !sb.Hash.Equal(sb.CalculateHash()) {
+		// Because we extract the public keys from the block, we need to insure
+		// the hash is correct with respect to what is stored
+		return errors.New("Calculated hash does not match")
+	}
+
+	if sb.Roster == nil {
+		return errors.New("Missing roster in the block")
+	}
+
 	publics := sb.Roster.ServicePublics(ServiceName)
 
 	for _, fl := range sb.ForwardLink {
@@ -467,6 +477,25 @@ func (sbs Proof) Verify() error {
 		return errors.New("First element must be a genesis")
 	}
 
+	return sbs.verifyChain()
+}
+
+// VerifyFromID checks that the proof is correct starting from a given
+// block and verifies the back and forward links up to the last block
+func (sbs Proof) VerifyFromID(id SkipBlockID) error {
+	if len(sbs) == 0 {
+		return errors.New("Empty list of blocks")
+	}
+
+	// the hash will be checked afterwards
+	if !sbs[0].Hash.Equal(id) {
+		return errors.New("Proof does not start with the correct block")
+	}
+
+	return sbs.verifyChain()
+}
+
+func (sbs Proof) verifyChain() error {
 	for i, sb := range sbs {
 		if !sb.CalculateHash().Equal(sb.Hash) {
 			return errors.New("Wrong hash")

skipchain/struct_test.go

@@ -79,8 +79,16 @@ func TestSkipBlock_VerifySignatures(t *testing.T) {
 	block1.BackLinkIDs = append(block1.BackLinkIDs, root.Hash)
 	block1.Index++
 	db.Store(block1)
+	require.NotNil(t, block1.VerifyForwardSignatures())
+	block1.updateHash()
 	require.Nil(t, block1.VerifyForwardSignatures())
 	require.NotNil(t, db.VerifyLinks(block1))
+
+	block1.Roster = nil
+	block1.updateHash()
+	err := block1.VerifyForwardSignatures()
+	require.NotNil(t, err)
+	require.Equal(t, "Missing roster in the block", err.Error())
 }
 
 func TestSkipBlock_Hash1(t *testing.T) {
@@ -257,6 +265,19 @@ func TestGetProof(t *testing.T) {
 	require.NotNil(t, err)
 }
 
+// Test the edge cases of the verification function
+func TestProof_Verify(t *testing.T) {
+	sb := NewSkipBlock()
+	sb.updateHash()
+
+	require.NotNil(t, Proof{}.Verify())
+	sb.Index = 1
+	require.NotNil(t, Proof{sb}.Verify())
+
+	require.NotNil(t, Proof{}.VerifyFromID(sb.Hash))
+	require.NotNil(t, Proof{sb}.VerifyFromID(SkipBlockID{}))
+}
+
 // setupSkipBlockDB initialises a database with a bucket called 'skipblock-test' inside.
 // The caller is responsible to close and remove the database file after using it.
 func setupSkipBlockDB(t *testing.T) (*SkipBlockDB, string) {