Fix v-doge-vul-004 (#38)

* Add some TODO comment for sometime failed tests of protocol Syncer

* Fix crash when faulty node send nil notify to syncer

* Add unit test

* Fix crash when faulty node send nil status notify to syncer

* Add unit test

protocol/service_v1.go

@@ -14,6 +14,11 @@ import (
 	empty "google.golang.org/protobuf/types/known/emptypb"
 )
 
+var (
+	ErrNilRawRequest    = errors.New("notify request raw is nil")
+	ErrNilStatusRequest = errors.New("notify request status is nil")
+)
+
 // serviceV1 is the GRPC server implementation for the v1 protocol
 type serviceV1 struct {
 	proto.UnimplementedV1Server
@@ -30,6 +35,15 @@ type rlpObject interface {
 }
 
 func (s *serviceV1) Notify(ctx context.Context, req *proto.NotifyReq) (*empty.Empty, error) {
+	if req.Raw == nil || len(req.Raw.Value) == 0 {
+		// malicious node conducted denial of service
+		return nil, ErrNilRawRequest
+	}
+
+	if req.Status == nil {
+		return nil, ErrNilStatusRequest
+	}
+
 	var id peer.ID
 
 	if ctx, ok := ctx.(*grpc.Context); ok {

protocol/syncer.go

@@ -345,6 +345,7 @@ func (s *Syncer) Broadcast(b *types.Block) {
 
 // Start starts the syncer protocol
 func (s *Syncer) Start() {
+	// TODO: why not derived logger instead of null one?
 	s.serviceV1 = &serviceV1{syncer: s, logger: hclog.NewNullLogger(), store: s.blockchain}
 
 	// Get the current status of the syncer

protocol/syncer_test.go

@@ -10,9 +10,11 @@ import (
 	"github.com/dogechain-lab/jury/blockchain"
 	"github.com/dogechain-lab/jury/helper/tests"
 	"github.com/dogechain-lab/jury/network"
+	"github.com/dogechain-lab/jury/protocol/proto"
 	"github.com/dogechain-lab/jury/types"
 	"github.com/hashicorp/go-hclog"
 	"github.com/stretchr/testify/assert"
+	anypb "google.golang.org/protobuf/types/known/anypb"
 )
 
 func TestHandleNewPeer(t *testing.T) {
@@ -314,6 +316,99 @@ func TestWatchSyncWithPeer(t *testing.T) {
 	}
 }
 
+func TestNilPointerAttackFromFaultyPeer(t *testing.T) {
+	tests := []struct {
+		name              string
+		headers           []*types.Header
+		peerHeaders       []*types.Header
+		numNewBlocks      int
+		testBroadcastFunc func(s *Syncer, b *types.Block)
+	}{
+		{
+			name:              "should not crash even notify raw data is nil",
+			headers:           blockchain.NewTestHeaderChainWithSeed(nil, 3, 0),
+			peerHeaders:       blockchain.NewTestHeaderChainWithSeed(nil, 1, 0),
+			numNewBlocks:      1,
+			testBroadcastFunc: broadcastNilRawData,
+		},
+		{
+			name:              "should not crash even notify status is nil",
+			headers:           blockchain.NewTestHeaderChainWithSeed(nil, 5, 0),
+			peerHeaders:       blockchain.NewTestHeaderChainWithSeed(nil, 1, 0),
+			numNewBlocks:      1,
+			testBroadcastFunc: broadcastNilStatusData,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			chain, peerChain := NewMockBlockchain(tt.headers), NewMockBlockchain(tt.peerHeaders)
+
+			_, peerSyncers := SetupSyncerNetwork(t, chain, []blockchainShim{peerChain})
+			peerSyncer := peerSyncers[0]
+
+			newBlocks := GenerateNewBlocks(t, peerChain, tt.numNewBlocks)
+
+			for _, newBlock := range newBlocks {
+				assert.NoError(t, peerSyncer.blockchain.WriteBlock(newBlock))
+			}
+
+			for _, b := range newBlocks {
+				assert.NotPanics(t, func() {
+					tt.testBroadcastFunc(peerSyncer, b)
+				})
+			}
+		})
+	}
+}
+
+func broadcastNilRawData(s *Syncer, b *types.Block) {
+	// Get the chain difficulty associated with block
+	td, ok := s.blockchain.GetTD(b.Hash())
+	if !ok {
+		// not supposed to happen
+		s.logger.Error("total difficulty not found", "block number", b.Number())
+
+		return
+	}
+
+	// broadcast the new block to all the peers
+	req := &proto.NotifyReq{
+		Status: &proto.V1Status{
+			Hash:       b.Hash().String(),
+			Number:     b.Number(),
+			Difficulty: td.String(),
+		},
+		Raw: nil,
+	}
+
+	s.peers.Range(func(peerID, peer interface{}) bool {
+		if _, err := peer.(*SyncPeer).client.Notify(context.Background(), req); err != nil {
+			s.logger.Error("failed to notify", "err", err)
+		}
+
+		return true
+	})
+}
+
+func broadcastNilStatusData(s *Syncer, b *types.Block) {
+	// broadcast the new block to all the peers
+	req := &proto.NotifyReq{
+		Status: nil,
+		Raw: &anypb.Any{
+			Value: b.MarshalRLP(),
+		},
+	}
+
+	s.peers.Range(func(peerID, peer interface{}) bool {
+		if _, err := peer.(*SyncPeer).client.Notify(context.Background(), req); err != nil {
+			s.logger.Error("failed to notify", "err", err)
+		}
+
+		return true
+	})
+}
+
 func TestBulkSyncWithPeer(t *testing.T) {
 	tests := []struct {
 		name        string