From e1be34bce1ddf662bca58a37a6f38fea63a2a70f Mon Sep 17 00:00:00 2001 From: zelig Date: Sat, 28 Mar 2015 00:48:37 +0000 Subject: [PATCH 1/5] eth: SEC-29 eth wire protocol decoding invalid message data crashes client - add validate method to types.Block - validate after Decode -> error - add tests for NewBlockMsg --- core/types/block.go | 20 ++++++++ eth/protocol.go | 9 ++-- eth/protocol_test.go | 117 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 124 insertions(+), 22 deletions(-) diff --git a/core/types/block.go b/core/types/block.go index 5cdde4462..c04beae5a 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -148,6 +148,26 @@ func NewBlockWithHeader(header *Header) *Block { return &Block{header: header} } +func (self *Block) Validate() error { + if self.header == nil { + return fmt.Errorf("header is nil") + } + // check *big.Int fields + if self.header.Difficulty == nil { + return fmt.Errorf("Difficulty undefined") + } + if self.header.GasLimit == nil { + return fmt.Errorf("GasLimit undefined") + } + if self.header.GasUsed == nil { + return fmt.Errorf("GasUsed undefined") + } + if self.header.Number == nil { + return fmt.Errorf("Number undefined") + } + return nil +} + func (self *Block) DecodeRLP(s *rlp.Stream) error { var eb extblock if err := s.Decode(&eb); err != nil { diff --git a/eth/protocol.go b/eth/protocol.go index e32ea233b..0a3f67b62 100644 --- a/eth/protocol.go +++ b/eth/protocol.go @@ -105,7 +105,7 @@ type getBlockHashesMsgData struct { type statusMsgData struct { ProtocolVersion uint32 NetworkId uint32 - TD *big.Int + TD big.Int CurrentBlock common.Hash GenesisBlock common.Hash } @@ -276,6 +276,9 @@ func (self *ethProtocol) handle() error { if err := msg.Decode(&request); err != nil { return self.protoError(ErrDecode, "%v: %v", msg, err) } + if err := request.Block.Validate(); err != nil { + return self.protoError(ErrDecode, "block validation %v: %v", msg, err) + } hash := request.Block.Hash() _, chainHead, _ := self.chainManager.Status() @@ -335,7 +338,7 @@ func (self *ethProtocol) handleStatus() error { return self.protoError(ErrProtocolVersionMismatch, "%d (!= %d)", status.ProtocolVersion, self.protocolVersion) } - _, suspended := self.blockPool.AddPeer(status.TD, status.CurrentBlock, self.id, self.requestBlockHashes, self.requestBlocks, self.protoErrorDisconnect) + _, suspended := self.blockPool.AddPeer(&status.TD, status.CurrentBlock, self.id, self.requestBlockHashes, self.requestBlocks, self.protoErrorDisconnect) if suspended { return self.protoError(ErrSuspendedPeer, "") } @@ -366,7 +369,7 @@ func (self *ethProtocol) sendStatus() error { return p2p.Send(self.rw, StatusMsg, &statusMsgData{ ProtocolVersion: uint32(self.protocolVersion), NetworkId: uint32(self.networkId), - TD: td, + TD: *td, CurrentBlock: currentBlock, GenesisBlock: genesisBlock, }) diff --git a/eth/protocol_test.go b/eth/protocol_test.go index 8ca6d1be6..7831e9bc6 100644 --- a/eth/protocol_test.go +++ b/eth/protocol_test.go @@ -1,6 +1,7 @@ package eth import ( + "fmt" "log" "math/big" "os" @@ -63,6 +64,10 @@ func (self *testChainManager) GetBlockHashesFromHash(hash common.Hash, amount ui func (self *testChainManager) Status() (td *big.Int, currentBlock common.Hash, genesisBlock common.Hash) { if self.status != nil { td, currentBlock, genesisBlock = self.status() + } else { + td = common.Big1 + currentBlock = common.Hash{1} + genesisBlock = common.Hash{2} } return } @@ -163,14 +168,29 @@ func (self *ethProtocolTester) run() { self.quit <- err } +func (self *ethProtocolTester) handshake(t *testing.T, mock bool) { + td, currentBlock, genesis := self.chainManager.Status() + // first outgoing msg should be StatusMsg. + err := p2p.ExpectMsg(self, StatusMsg, &statusMsgData{ + ProtocolVersion: ProtocolVersion, + NetworkId: NetworkId, + TD: *td, + CurrentBlock: currentBlock, + GenesisBlock: genesis, + }) + if err != nil { + t.Fatalf("incorrect outgoing status: %v", err) + } + if mock { + go p2p.Send(self, StatusMsg, &statusMsgData{ProtocolVersion, NetworkId, *td, currentBlock, genesis}) + } +} + func TestStatusMsgErrors(t *testing.T) { logInit() eth := newEth(t) - td := common.Big1 - currentBlock := common.Hash{1} - genesis := common.Hash{2} - eth.chainManager.status = func() (*big.Int, common.Hash, common.Hash) { return td, currentBlock, genesis } go eth.run() + td, currentBlock, genesis := eth.chainManager.Status() tests := []struct { code uint64 @@ -182,31 +202,20 @@ func TestStatusMsgErrors(t *testing.T) { wantErrorCode: ErrNoStatusMsg, }, { - code: StatusMsg, data: statusMsgData{10, NetworkId, td, currentBlock, genesis}, + code: StatusMsg, data: statusMsgData{10, NetworkId, *td, currentBlock, genesis}, wantErrorCode: ErrProtocolVersionMismatch, }, { - code: StatusMsg, data: statusMsgData{ProtocolVersion, 999, td, currentBlock, genesis}, + code: StatusMsg, data: statusMsgData{ProtocolVersion, 999, *td, currentBlock, genesis}, wantErrorCode: ErrNetworkIdMismatch, }, { - code: StatusMsg, data: statusMsgData{ProtocolVersion, NetworkId, td, currentBlock, common.Hash{3}}, + code: StatusMsg, data: statusMsgData{ProtocolVersion, NetworkId, *td, currentBlock, common.Hash{3}}, wantErrorCode: ErrGenesisBlockMismatch, }, } for _, test := range tests { - // first outgoing msg should be StatusMsg. - err := p2p.ExpectMsg(eth, StatusMsg, &statusMsgData{ - ProtocolVersion: ProtocolVersion, - NetworkId: NetworkId, - TD: td, - CurrentBlock: currentBlock, - GenesisBlock: genesis, - }) - if err != nil { - t.Fatalf("incorrect outgoing status: %v", err) - } - + eth.handshake(t, false) // the send call might hang until reset because // the protocol might not read the payload. go p2p.Send(eth, test.code, test.data) @@ -216,3 +225,73 @@ func TestStatusMsgErrors(t *testing.T) { go eth.run() } } + +func TestNewBlockMsg(t *testing.T) { + logInit() + eth := newEth(t) + eth.blockPool.addBlock = func(block *types.Block, peerId string) (err error) { + fmt.Printf("Add Block: %v\n", block) + return + } + + var disconnected bool + eth.blockPool.removePeer = func(peerId string) { + fmt.Printf("peer <%s> is disconnected\n", peerId) + disconnected = true + } + + go eth.run() + + eth.handshake(t, true) + err := p2p.ExpectMsg(eth, TxMsg, []interface{}{}) + if err != nil { + t.Errorf("transactions expected, got %v", err) + } + + var tds = make(chan *big.Int) + eth.blockPool.addPeer = func(td *big.Int, currentBlock common.Hash, peerId string, requestHashes func(common.Hash) error, requestBlocks func([]common.Hash) error, peerError func(*errs.Error)) (best bool, suspended bool) { + tds <- td + return + } + + var delay = 1 * time.Second + // eth.reset() + block := types.NewBlock(common.Hash{1}, common.Address{1}, common.Hash{1}, common.Big1, 1, "extra") + + go p2p.Send(eth, NewBlockMsg, &newBlockMsgData{Block: block}) + timer := time.After(delay) + + select { + case td := <-tds: + if td.Cmp(common.Big0) != 0 { + t.Errorf("incorrect td %v, expected %v", td, common.Big0) + } + case <-timer: + t.Errorf("no td recorded after %v", delay) + return + case err := <-eth.quit: + t.Errorf("no error expected, got %v", err) + return + } + + go p2p.Send(eth, NewBlockMsg, &newBlockMsgData{block, common.Big2}) + timer = time.After(delay) + + select { + case td := <-tds: + if td.Cmp(common.Big2) != 0 { + t.Errorf("incorrect td %v, expected %v", td, common.Big2) + } + case <-timer: + t.Errorf("no td recorded after %v", delay) + return + case err := <-eth.quit: + t.Errorf("no error expected, got %v", err) + return + } + + go p2p.Send(eth, NewBlockMsg, []interface{}{}) + // Block.DecodeRLP: validation failed: header is nil + eth.checkError(ErrDecode, delay) + +} From d677190f3916c5bee276d9abba69814022ab967f Mon Sep 17 00:00:00 2001 From: zelig Date: Sat, 28 Mar 2015 01:54:23 +0000 Subject: [PATCH 2/5] add tests for valid blocks msg handling --- eth/protocol_test.go | 50 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/eth/protocol_test.go b/eth/protocol_test.go index 7831e9bc6..d5ac21755 100644 --- a/eth/protocol_test.go +++ b/eth/protocol_test.go @@ -229,10 +229,6 @@ func TestStatusMsgErrors(t *testing.T) { func TestNewBlockMsg(t *testing.T) { logInit() eth := newEth(t) - eth.blockPool.addBlock = func(block *types.Block, peerId string) (err error) { - fmt.Printf("Add Block: %v\n", block) - return - } var disconnected bool eth.blockPool.removePeer = func(peerId string) { @@ -295,3 +291,49 @@ func TestNewBlockMsg(t *testing.T) { eth.checkError(ErrDecode, delay) } + +func TestBlockMsg(t *testing.T) { + logInit() + eth := newEth(t) + blocks := make(chan *types.Block) + eth.blockPool.addBlock = func(block *types.Block, peerId string) (err error) { + blocks <- block + return + } + + var disconnected bool + eth.blockPool.removePeer = func(peerId string) { + fmt.Printf("peer <%s> is disconnected\n", peerId) + disconnected = true + } + + go eth.run() + + eth.handshake(t, true) + err := p2p.ExpectMsg(eth, TxMsg, []interface{}{}) + if err != nil { + t.Errorf("transactions expected, got %v", err) + } + + var delay = 3 * time.Second + // eth.reset() + newblock := func(i int64) *types.Block { + return types.NewBlock(common.Hash{byte(i)}, common.Address{byte(i)}, common.Hash{byte(i)}, big.NewInt(i), uint64(i), string(i)) + } + go p2p.Send(eth, BlocksMsg, types.Blocks{newblock(0), newblock(1), newblock(2)}) + timer := time.After(delay) + for i := int64(0); i < 3; i++ { + select { + case block := <-blocks: + if (block.ParentHash() != common.Hash{byte(i)}) { + t.Errorf("incorrect block %v, expected %v", block.ParentHash(), common.Hash{byte(i)}) + } + case <-timer: + t.Errorf("no td recorded after %v", delay) + return + case err := <-eth.quit: + t.Errorf("no error expected, got %v", err) + return + } + } +} From 82da6bf4d213784cfc7ba45432f9f96c2d6b4d9d Mon Sep 17 00:00:00 2001 From: zelig Date: Mon, 30 Mar 2015 13:48:07 +0100 Subject: [PATCH 3/5] test for invalid rlp encoding of block in BlocksMsg - rename Validate -> ValidateFields not to confure consensus block validation - add nil transaction and nil uncle header validation - remove bigint field checks: rlp already decodes *big.Int to big.NewInt(0) - add test for nil header, nil transaction --- core/types/block.go | 27 ++++++++++++--------------- eth/protocol.go | 5 ++++- eth/protocol_test.go | 34 ++++++++++++++++++++++++++++------ 3 files changed, 44 insertions(+), 22 deletions(-) diff --git a/core/types/block.go b/core/types/block.go index c04beae5a..a40bac42c 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -148,22 +148,19 @@ func NewBlockWithHeader(header *Header) *Block { return &Block{header: header} } -func (self *Block) Validate() error { +func (self *Block) ValidateFields() error { if self.header == nil { return fmt.Errorf("header is nil") } - // check *big.Int fields - if self.header.Difficulty == nil { - return fmt.Errorf("Difficulty undefined") + for i, transaction := range self.transactions { + if transaction == nil { + return fmt.Errorf("transaction %d is nil", i) + } } - if self.header.GasLimit == nil { - return fmt.Errorf("GasLimit undefined") - } - if self.header.GasUsed == nil { - return fmt.Errorf("GasUsed undefined") - } - if self.header.Number == nil { - return fmt.Errorf("Number undefined") + for i, uncle := range self.uncles { + if uncle == nil { + return fmt.Errorf("uncle %d is nil", i) + } } return nil } @@ -253,10 +250,10 @@ func (self *Block) AddReceipt(receipt *Receipt) { } func (self *Block) RlpData() interface{} { - return []interface{}{self.header, self.transactions, self.uncles} -} + // return []interface{}{self.header, self.transactions, self.uncles} + // } -func (self *Block) RlpDataForStorage() interface{} { + // func (self *Block) RlpDataForStorage() interface{} { return []interface{}{self.header, self.transactions, self.uncles, self.Td /* TODO receipts */} } diff --git a/eth/protocol.go b/eth/protocol.go index 0a3f67b62..f0a749d33 100644 --- a/eth/protocol.go +++ b/eth/protocol.go @@ -268,6 +268,9 @@ func (self *ethProtocol) handle() error { return self.protoError(ErrDecode, "msg %v: %v", msg, err) } } + if err := block.ValidateFields(); err != nil { + return self.protoError(ErrDecode, "block validation %v: %v", msg, err) + } self.blockPool.AddBlock(&block, self.id) } @@ -276,7 +279,7 @@ func (self *ethProtocol) handle() error { if err := msg.Decode(&request); err != nil { return self.protoError(ErrDecode, "%v: %v", msg, err) } - if err := request.Block.Validate(); err != nil { + if err := request.Block.ValidateFields(); err != nil { return self.protoError(ErrDecode, "block validation %v: %v", msg, err) } hash := request.Block.Hash() diff --git a/eth/protocol_test.go b/eth/protocol_test.go index d5ac21755..6a8eedadd 100644 --- a/eth/protocol_test.go +++ b/eth/protocol_test.go @@ -1,7 +1,6 @@ package eth import ( - "fmt" "log" "math/big" "os" @@ -227,12 +226,11 @@ func TestStatusMsgErrors(t *testing.T) { } func TestNewBlockMsg(t *testing.T) { - logInit() + // logInit() eth := newEth(t) var disconnected bool eth.blockPool.removePeer = func(peerId string) { - fmt.Printf("peer <%s> is disconnected\n", peerId) disconnected = true } @@ -293,7 +291,7 @@ func TestNewBlockMsg(t *testing.T) { } func TestBlockMsg(t *testing.T) { - logInit() + // logInit() eth := newEth(t) blocks := make(chan *types.Block) eth.blockPool.addBlock = func(block *types.Block, peerId string) (err error) { @@ -303,7 +301,6 @@ func TestBlockMsg(t *testing.T) { var disconnected bool eth.blockPool.removePeer = func(peerId string) { - fmt.Printf("peer <%s> is disconnected\n", peerId) disconnected = true } @@ -320,7 +317,9 @@ func TestBlockMsg(t *testing.T) { newblock := func(i int64) *types.Block { return types.NewBlock(common.Hash{byte(i)}, common.Address{byte(i)}, common.Hash{byte(i)}, big.NewInt(i), uint64(i), string(i)) } - go p2p.Send(eth, BlocksMsg, types.Blocks{newblock(0), newblock(1), newblock(2)}) + b := newblock(0) + b.Header().Difficulty = nil // check if nil as *big.Int decodes as 0 + go p2p.Send(eth, BlocksMsg, types.Blocks{b, newblock(1), newblock(2)}) timer := time.After(delay) for i := int64(0); i < 3; i++ { select { @@ -328,6 +327,9 @@ func TestBlockMsg(t *testing.T) { if (block.ParentHash() != common.Hash{byte(i)}) { t.Errorf("incorrect block %v, expected %v", block.ParentHash(), common.Hash{byte(i)}) } + if block.Difficulty().Cmp(big.NewInt(i)) != 0 { + t.Errorf("incorrect block %v, expected %v", block.Difficulty(), big.NewInt(i)) + } case <-timer: t.Errorf("no td recorded after %v", delay) return @@ -336,4 +338,24 @@ func TestBlockMsg(t *testing.T) { return } } + + go p2p.Send(eth, BlocksMsg, []interface{}{[]interface{}{}}) + eth.checkError(ErrDecode, delay) + if !disconnected { + t.Errorf("peer not disconnected after error") + } + + // test empty transaction + eth.reset() + go eth.run() + eth.handshake(t, true) + err = p2p.ExpectMsg(eth, TxMsg, []interface{}{}) + if err != nil { + t.Errorf("transactions expected, got %v", err) + } + b = newblock(0) + b.AddTransaction(nil) + go p2p.Send(eth, BlocksMsg, types.Blocks{b}) + eth.checkError(ErrDecode, delay) + } From 6ffea34d8bf19fb358c1a7f01e20bf25f1061c5e Mon Sep 17 00:00:00 2001 From: zelig Date: Mon, 30 Mar 2015 15:21:41 +0100 Subject: [PATCH 4/5] check TxMsg - add validation on TxMsg checking for nil - add test for nil transaction - add test for zero value transaction (no extra validation needed) --- core/types/block.go | 6 +++--- eth/protocol.go | 5 ++++- eth/protocol_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/core/types/block.go b/core/types/block.go index a40bac42c..d5cd8a21e 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -250,10 +250,10 @@ func (self *Block) AddReceipt(receipt *Receipt) { } func (self *Block) RlpData() interface{} { - // return []interface{}{self.header, self.transactions, self.uncles} - // } + return []interface{}{self.header, self.transactions, self.uncles} +} - // func (self *Block) RlpDataForStorage() interface{} { +func (self *Block) RlpDataForStorage() interface{} { return []interface{}{self.header, self.transactions, self.uncles, self.Td /* TODO receipts */} } diff --git a/eth/protocol.go b/eth/protocol.go index f0a749d33..214eed875 100644 --- a/eth/protocol.go +++ b/eth/protocol.go @@ -185,7 +185,10 @@ func (self *ethProtocol) handle() error { if err := msg.Decode(&txs); err != nil { return self.protoError(ErrDecode, "msg %v: %v", msg, err) } - for _, tx := range txs { + for i, tx := range txs { + if tx == nil { + return self.protoError(ErrDecode, "transaction %d is nil", i) + } jsonlogger.LogJson(&logger.EthTxReceived{ TxHash: tx.Hash().Hex(), RemoteId: self.peer.ID().String(), diff --git a/eth/protocol_test.go b/eth/protocol_test.go index 6a8eedadd..2228fa0ec 100644 --- a/eth/protocol_test.go +++ b/eth/protocol_test.go @@ -359,3 +359,42 @@ func TestBlockMsg(t *testing.T) { eth.checkError(ErrDecode, delay) } + +func TestTransactionsMsg(t *testing.T) { + logInit() + eth := newEth(t) + txs := make(chan *types.Transaction) + + eth.txPool.addTransactions = func(t []*types.Transaction) { + for _, tx := range t { + txs <- tx + } + } + go eth.run() + + eth.handshake(t, true) + err := p2p.ExpectMsg(eth, TxMsg, []interface{}{}) + if err != nil { + t.Errorf("transactions expected, got %v", err) + } + + var delay = 3 * time.Second + tx := &types.Transaction{} + + go p2p.Send(eth, TxMsg, []interface{}{tx, tx}) + timer := time.After(delay) + for i := int64(0); i < 2; i++ { + select { + case <-txs: + case <-timer: + return + case err := <-eth.quit: + t.Errorf("no error expected, got %v", err) + return + } + } + + go p2p.Send(eth, TxMsg, []interface{}{[]interface{}{}}) + eth.checkError(ErrDecode, delay) + +} From f56fc9cd9d13e88ee1a244ea590e249e324b8b84 Mon Sep 17 00:00:00 2001 From: zelig Date: Wed, 1 Apr 2015 12:36:49 +0100 Subject: [PATCH 5/5] change StatusMsgData.TD back to pointer type *big.Int --- eth/protocol.go | 6 +++--- eth/protocol_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/eth/protocol.go b/eth/protocol.go index 214eed875..a0ab177cd 100644 --- a/eth/protocol.go +++ b/eth/protocol.go @@ -105,7 +105,7 @@ type getBlockHashesMsgData struct { type statusMsgData struct { ProtocolVersion uint32 NetworkId uint32 - TD big.Int + TD *big.Int CurrentBlock common.Hash GenesisBlock common.Hash } @@ -344,7 +344,7 @@ func (self *ethProtocol) handleStatus() error { return self.protoError(ErrProtocolVersionMismatch, "%d (!= %d)", status.ProtocolVersion, self.protocolVersion) } - _, suspended := self.blockPool.AddPeer(&status.TD, status.CurrentBlock, self.id, self.requestBlockHashes, self.requestBlocks, self.protoErrorDisconnect) + _, suspended := self.blockPool.AddPeer(status.TD, status.CurrentBlock, self.id, self.requestBlockHashes, self.requestBlocks, self.protoErrorDisconnect) if suspended { return self.protoError(ErrSuspendedPeer, "") } @@ -375,7 +375,7 @@ func (self *ethProtocol) sendStatus() error { return p2p.Send(self.rw, StatusMsg, &statusMsgData{ ProtocolVersion: uint32(self.protocolVersion), NetworkId: uint32(self.networkId), - TD: *td, + TD: td, CurrentBlock: currentBlock, GenesisBlock: genesisBlock, }) diff --git a/eth/protocol_test.go b/eth/protocol_test.go index 2228fa0ec..d3466326a 100644 --- a/eth/protocol_test.go +++ b/eth/protocol_test.go @@ -173,7 +173,7 @@ func (self *ethProtocolTester) handshake(t *testing.T, mock bool) { err := p2p.ExpectMsg(self, StatusMsg, &statusMsgData{ ProtocolVersion: ProtocolVersion, NetworkId: NetworkId, - TD: *td, + TD: td, CurrentBlock: currentBlock, GenesisBlock: genesis, }) @@ -181,7 +181,7 @@ func (self *ethProtocolTester) handshake(t *testing.T, mock bool) { t.Fatalf("incorrect outgoing status: %v", err) } if mock { - go p2p.Send(self, StatusMsg, &statusMsgData{ProtocolVersion, NetworkId, *td, currentBlock, genesis}) + go p2p.Send(self, StatusMsg, &statusMsgData{ProtocolVersion, NetworkId, td, currentBlock, genesis}) } } @@ -201,15 +201,15 @@ func TestStatusMsgErrors(t *testing.T) { wantErrorCode: ErrNoStatusMsg, }, { - code: StatusMsg, data: statusMsgData{10, NetworkId, *td, currentBlock, genesis}, + code: StatusMsg, data: statusMsgData{10, NetworkId, td, currentBlock, genesis}, wantErrorCode: ErrProtocolVersionMismatch, }, { - code: StatusMsg, data: statusMsgData{ProtocolVersion, 999, *td, currentBlock, genesis}, + code: StatusMsg, data: statusMsgData{ProtocolVersion, 999, td, currentBlock, genesis}, wantErrorCode: ErrNetworkIdMismatch, }, { - code: StatusMsg, data: statusMsgData{ProtocolVersion, NetworkId, *td, currentBlock, common.Hash{3}}, + code: StatusMsg, data: statusMsgData{ProtocolVersion, NetworkId, td, currentBlock, common.Hash{3}}, wantErrorCode: ErrGenesisBlockMismatch, }, }