From 39698fc0579b0c0cab98c110c3d2432aec0c855b Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Tue, 27 Feb 2024 15:39:42 +0530 Subject: [PATCH 1/6] eth/downloader: bypass peer validation if remote peer is far away --- eth/downloader/downloader.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 22af24789a..ef705fa898 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -893,13 +893,6 @@ func (d *Downloader) getFetchHeadersByNumber(p *peerConnection) func(number uint // In the rare scenario when we ended up on a long reorganisation (i.e. none of // the head links match), we do a binary search to find the common ancestor. func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) (uint64, error) { - // Check the validity of peer from which the chain is to be downloaded - if d.ChainValidator != nil { - if _, err := d.IsValidPeer(d.getFetchHeadersByNumber(p)); err != nil { - return 0, err - } - } - // Figure out the valid ancestor range to prevent rewrite attacks var ( floor = int64(-1) @@ -917,6 +910,23 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) localHeight = d.lightchain.CurrentHeader().Number.Uint64() } + // Check the validity of peer from which the chain is to be downloaded + if d.ChainValidator != nil { + _, err := d.IsValidPeer(d.getFetchHeadersByNumber(p)) + if errors.Is(err, whitelist.ErrMismatch) { + return 0, err + } + + // Assuming that `remoteHeight` is always greater than `localHeight`, we won't + // check peer validity if the remote header is far ahead of us. + if errors.Is(err, whitelist.ErrNoRemote) && localHeight > remoteHeight-1024 { + log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + } else if errors.Is(err, whitelist.ErrNoRemote) { + log.Info("Remote peer didn't respond", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + return 0, err + } + } + p.log.Debug("Looking for common ancestor", "local", localHeight, "remote", remoteHeight) // Recap floor value for binary search From a109589dcf59c6486e3faf64eb9182bf37301cf1 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Sat, 13 Apr 2024 14:03:23 +0530 Subject: [PATCH 2/6] eth/downloader: parameterise threshold, fix condition --- eth/downloader/downloader.go | 39 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index ef705fa898..8209613e8f 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -147,7 +147,9 @@ type Downloader struct { quitCh chan struct{} // Quit channel to signal termination quitLock sync.Mutex // Lock to prevent double closes + // Validation ethereum.ChainValidator + maxValidationThreshold uint64 // Number of block difference from remote peer to start validation // Testing hooks syncInitHook func(uint64, uint64) // Method to call upon initiating a new sync run @@ -228,19 +230,20 @@ func New(stateDb ethdb.Database, mux *event.TypeMux, chain BlockChain, lightchai } dl := &Downloader{ - stateDB: stateDb, - mux: mux, - queue: newQueue(blockCacheMaxItems, blockCacheInitialItems), - peers: newPeerSet(), - blockchain: chain, - lightchain: lightchain, - dropPeer: dropPeer, - headerProcCh: make(chan *headerTask, 1), - quitCh: make(chan struct{}), - SnapSyncer: snap.NewSyncer(stateDb, chain.TrieDB().Scheme()), - stateSyncStart: make(chan *stateSync), - syncStartBlock: chain.CurrentSnapBlock().Number.Uint64(), - ChainValidator: whitelistService, + stateDB: stateDb, + mux: mux, + queue: newQueue(blockCacheMaxItems, blockCacheInitialItems), + peers: newPeerSet(), + blockchain: chain, + lightchain: lightchain, + dropPeer: dropPeer, + headerProcCh: make(chan *headerTask, 1), + quitCh: make(chan struct{}), + SnapSyncer: snap.NewSyncer(stateDb, chain.TrieDB().Scheme()), + stateSyncStart: make(chan *stateSync), + syncStartBlock: chain.CurrentSnapBlock().Number.Uint64(), + ChainValidator: whitelistService, + maxValidationThreshold: 1024, } // Create the post-merge skeleton syncer and start the process dl.skeleton = newSkeleton(stateDb, dl.peers, dropPeer, newBeaconBackfiller(dl, success)) @@ -917,13 +920,13 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) return 0, err } - // Assuming that `remoteHeight` is always greater than `localHeight`, we won't - // check peer validity if the remote header is far ahead of us. - if errors.Is(err, whitelist.ErrNoRemote) && localHeight > remoteHeight-1024 { - log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) - } else if errors.Is(err, whitelist.ErrNoRemote) { + // Don't validate the peer against whitelisted milestones until the different of + // our local height and remote peer's height is less than `maxValidationThreshold` + if errors.Is(err, whitelist.ErrNoRemote) && localHeight >= remoteHeight-d.maxValidationThreshold { log.Info("Remote peer didn't respond", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) return 0, err + } else if errors.Is(err, whitelist.ErrNoRemote) { + log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) } } From b458bf95bf7d1007b884e591abab597915ab3720 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Sat, 13 Apr 2024 14:03:44 +0530 Subject: [PATCH 3/6] eth: add tests for bypassing validation --- eth/downloader/downloader_test.go | 34 ++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index b248c860ec..e1ecd011e3 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -1649,14 +1649,42 @@ func TestFakedSyncProgress67NoRemoteCheckpoint(t *testing.T) { chainA := testChainForkLightA.blocks tester.newPeer("light", protocol, chainA[1:]) + // Set the max validation threshold equal to chain length to enforce validation + tester.downloader.maxValidationThreshold = uint64(len(chainA) - 1) + // Synchronise with the peer and make sure all blocks were retrieved // Should fail in first attempt - if err := tester.sync("light", nil, mode); err != nil { - assert.Equal(t, whitelist.ErrNoRemote, err, "failed synchronisation") - } + err := tester.sync("light", nil, mode) + assert.Equal(t, whitelist.ErrNoRemote, err, "failed synchronisation") // Try syncing again, should succeed if err := tester.sync("light", nil, mode); err != nil { t.Fatal("succeeded attacker synchronisation") } } + +// TestFakedSyncProgress67BypassWhitelistValidation tests if peer validation +// via whitelist is bypassed when remote peer is far away or not +func TestFakedSyncProgress67BypassWhitelistValidation(t *testing.T) { + protocol := uint(eth.ETH67) + mode := FullSync + + tester := newTester(t) + validate := func(count int) (bool, error) { + return false, whitelist.ErrNoRemote + } + + tester.downloader.ChainValidator = newWhitelistFake(validate) + + defer tester.terminate() + + // 1223 length chain + chainA := testChainBase.blocks + tester.newPeer("light", protocol, chainA[1:]) + + // Although the validate function above returns an error (which says that + // remote peer doesn't have that block), sync will go through as the chain + // import length is 1223 which is more than the default threshold of 1024 + err := tester.sync("light", nil, mode) + assert.NoError(t, err, "failed synchronisation") +} From f22d00bcae2b14d453425700b58fd24551986f27 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Mon, 15 Apr 2024 13:34:46 +0530 Subject: [PATCH 4/6] eth/downloader: declare max validation threshold instead of using direct value --- eth/downloader/downloader.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 8209613e8f..4e144a2d85 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -57,6 +57,8 @@ var ( fsHeaderSafetyNet = 2048 // Number of headers to discard in case a chain violation is detected fsHeaderContCheck = 3 * time.Second // Time interval to check for header continuations during state download fsMinFullBlocks = 64 // Number of blocks to retrieve fully even in snap sync + + maxValidationThreshold = uint64(1024) // Number of block difference from remote peer to start validation ) var ( @@ -243,7 +245,7 @@ func New(stateDb ethdb.Database, mux *event.TypeMux, chain BlockChain, lightchai stateSyncStart: make(chan *stateSync), syncStartBlock: chain.CurrentSnapBlock().Number.Uint64(), ChainValidator: whitelistService, - maxValidationThreshold: 1024, + maxValidationThreshold: maxValidationThreshold, } // Create the post-merge skeleton syncer and start the process dl.skeleton = newSkeleton(stateDb, dl.peers, dropPeer, newBeaconBackfiller(dl, success)) From 10c831932c0ab5817edd732af3283030c7e4a9a4 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Mon, 15 Apr 2024 13:38:25 +0530 Subject: [PATCH 5/6] address comments --- eth/downloader/downloader.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 4e144a2d85..5d4423bbb9 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -922,13 +922,15 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) return 0, err } - // Don't validate the peer against whitelisted milestones until the different of - // our local height and remote peer's height is less than `maxValidationThreshold` - if errors.Is(err, whitelist.ErrNoRemote) && localHeight >= remoteHeight-d.maxValidationThreshold { - log.Info("Remote peer didn't respond", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) - return 0, err - } else if errors.Is(err, whitelist.ErrNoRemote) { - log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + if errors.Is(err, whitelist.ErrNoRemote) { + // Don't validate the peer against whitelisted milestones until the different of + // our local height and remote peer's height is less than `maxValidationThreshold` + if localHeight >= remoteHeight-d.maxValidationThreshold { + log.Info("Remote peer didn't respond", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + return 0, err + } else { + log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + } } } From 21493206d1a0a372072ee02e5413f7fc8a51e2cf Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Mon, 15 Apr 2024 13:59:34 +0530 Subject: [PATCH 6/6] simplify --- eth/downloader/downloader.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 5d4423bbb9..a342927f7a 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -928,9 +928,9 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) if localHeight >= remoteHeight-d.maxValidationThreshold { log.Info("Remote peer didn't respond", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) return 0, err - } else { - log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) } + + log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) } }