Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eth: check propagated block malformation on receiption #20546

Merged
merged 1 commit into from
Jan 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions eth/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,14 @@ func (pm *ProtocolManager) handleMsg(p *peer) error {
if err := msg.Decode(&request); err != nil {
return errResp(ErrDecode, "%v: %v", msg, err)
}
if hash := types.CalcUncleHash(request.Block.Uncles()); hash != request.Block.UncleHash() {
log.Warn("Propagated block has invalid uncles", "have", hash, "exp", request.Block.UncleHash())
break // TODO(karalabe): return error eventually, but wait a few releases
}
if hash := types.DeriveSha(request.Block.Transactions()); hash != request.Block.TxHash() {
log.Warn("Propagated block has invalid body", "have", hash, "exp", request.Block.TxHash())
break // TODO(karalabe): return error eventually, but wait a few releases
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, once we do add that, it might make sense to put this check into request.sanityCheck().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of which, shouldn't you do this check after the sanity check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really matter imho. Sanity check just checks that the numbers aren't enormous. Those shouldn't impact the uncle/tx hash in any way.

}
if err := request.sanityCheck(); err != nil {
return err
}
Expand Down
62 changes: 62 additions & 0 deletions eth/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -634,3 +634,65 @@ outer:
t.Errorf("block broadcast to %d peers, expected %d", receivedCount, broadcastExpected)
}
}

// Tests that a propagated malformed block (uncles or transactions don't match
// with the hashes in the header) gets discarded and not broadcast forward.
func TestBroadcastMalformedBlock(t *testing.T) {
// Create a live node to test propagation with
var (
engine = ethash.NewFaker()
db = rawdb.NewMemoryDatabase()
config = &params.ChainConfig{}
gspec = &core.Genesis{Config: config}
genesis = gspec.MustCommit(db)
)
blockchain, err := core.NewBlockChain(db, nil, config, engine, vm.Config{}, nil)
if err != nil {
t.Fatalf("failed to create new blockchain: %v", err)
}
pm, err := NewProtocolManager(config, nil, downloader.FullSync, DefaultConfig.NetworkId, new(event.TypeMux), new(testTxPool), engine, blockchain, db, 1, nil)
if err != nil {
t.Fatalf("failed to start test protocol manager: %v", err)
}
pm.Start(2)
defer pm.Stop()

// Create two peers, one to send the malformed block with and one to check
// propagation
source, _ := newTestPeer("source", eth63, pm, true)
defer source.close()

sink, _ := newTestPeer("sink", eth63, pm, true)
defer sink.close()

// Create various combinations of malformed blocks
chain, _ := core.GenerateChain(gspec.Config, genesis, ethash.NewFaker(), db, 1, func(i int, gen *core.BlockGen) {})

malformedUncles := chain[0].Header()
malformedUncles.UncleHash[0]++
malformedTransactions := chain[0].Header()
malformedTransactions.TxHash[0]++
malformedEverything := chain[0].Header()
malformedEverything.UncleHash[0]++
malformedEverything.TxHash[0]++

// Keep listening to broadcasts and notify if any arrives
notify := make(chan struct{})
go func() {
if _, err := sink.app.ReadMsg(); err == nil {
notify <- struct{}{}
}
}()
// Try to broadcast all malformations and ensure they all get discarded
for _, header := range []*types.Header{malformedUncles, malformedTransactions, malformedEverything} {
block := types.NewBlockWithHeader(header).WithBody(chain[0].Transactions(), chain[0].Uncles())
if err := p2p.Send(source.app, NewBlockMsg, []interface{}{block, big.NewInt(131136)}); err != nil {
t.Fatalf("failed to broadcast block: %v", err)
}
select {
case <-notify:
t.Fatalf("malformed block forwarded")
case <-time.After(100 * time.Millisecond):
}
}
}