diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index ed06d6ffe..c319abb18 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -129,4 +129,5 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [blockchain/v2] Correctly set block store base in status responses (@erikgrinaker) - [consensus] [\#4895](https://github.com/tendermint/tendermint/pull/4895) Cache the address of the validator to reduce querying a remote KMS (@joe-bowman) - [consensus] \#4970 Stricter on `LastCommitRound` check (@cuonglm) +- [p2p][\#5136](https://github.com/tendermint/tendermint/pull/5136) Fix error for peer with the same ID but different IPs (@valardragon) - [proxy] \#5078 Fix a bug, where TM does not exit when ABCI app crashes (@melekes) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 5bb934056..0f24b6683 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -518,11 +518,10 @@ func (a *addrBook) getBucket(bucketType byte, bucketIdx int) map[string]*knownAd // Adds ka to new bucket. Returns false if it couldn't do it cuz buckets full. // NOTE: currently it always returns true. -func (a *addrBook) addToNewBucket(ka *knownAddress, bucketIdx int) { - // Sanity check +func (a *addrBook) addToNewBucket(ka *knownAddress, bucketIdx int) error { + // Consistency check to ensure we don't add an already known address if ka.isOld() { - a.Logger.Error("Failed Sanity Check! Cant add old address to new bucket", "ka", ka, "bucket", bucketIdx) - return + return errAddrBookOldAddressNewBucket{ka.Addr, bucketIdx} } addrStr := ka.Addr.String() @@ -530,7 +529,7 @@ func (a *addrBook) addToNewBucket(ka *knownAddress, bucketIdx int) { // Already exists? if _, ok := bucket[addrStr]; ok { - return + return nil } // Enforce max addresses. @@ -548,6 +547,7 @@ func (a *addrBook) addToNewBucket(ka *knownAddress, bucketIdx int) { // Add it to addrLookup a.addrLookup[ka.ID()] = ka + return nil } // Adds ka to old bucket. Returns false if it couldn't do it cuz buckets full. @@ -665,8 +665,10 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error { ka := a.addrLookup[addr.ID] if ka != nil { - // If its already old and the addr is the same, ignore it. - if ka.isOld() && ka.Addr.Equals(addr) { + // If its already old and the address ID's are the same, ignore it. + // Thereby avoiding issues with a node on the network attempting to change + // the IP of a known node ID. (Which could yield an eclipse attack on the node) + if ka.isOld() && ka.Addr.ID == addr.ID { return nil } // Already in max new buckets. @@ -686,8 +688,7 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error { if err != nil { return err } - a.addToNewBucket(ka, bucket) - return nil + return a.addToNewBucket(ka, bucket) } func (a *addrBook) randomPickAddresses(bucketType byte, num int) []*p2p.NetAddress { diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index a2b68d314..ad41d5562 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -589,6 +589,61 @@ func TestMultipleAddrBookAddressSelection(t *testing.T) { } } +func TestAddrBookAddDoesNotOverwriteOldIP(t *testing.T) { + fname := createTempFileName("addrbook_test") + defer deleteTempFile(fname) + + // This test creates adds a peer to the address book and marks it good + // It then attempts to override the peer's IP, by adding a peer with the same ID + // but different IP. We distinguish the IP's by "RealIP" and "OverrideAttemptIP" + peerID := "678503e6c8f50db7279c7da3cb9b072aac4bc0d5" + peerRealIP := "1.1.1.1:26656" + peerOverrideAttemptIP := "2.2.2.2:26656" + SrcAddr := "b0dd378c3fbc4c156cd6d302a799f0d2e4227201@159.89.121.174:26656" + + // There is a chance that AddAddress will ignore the new peer its given. + // So we repeat trying to override the peer several times, + // to ensure we aren't in a case that got probabilistically ignored + numOverrideAttempts := 10 + + peerRealAddr, err := p2p.NewNetAddressString(peerID + "@" + peerRealIP) + require.Nil(t, err) + + peerOverrideAttemptAddr, err := p2p.NewNetAddressString(peerID + "@" + peerOverrideAttemptIP) + require.Nil(t, err) + + src, err := p2p.NewNetAddressString(SrcAddr) + require.Nil(t, err) + + book := NewAddrBook(fname, true) + book.SetLogger(log.TestingLogger()) + err = book.AddAddress(peerRealAddr, src) + require.Nil(t, err) + book.MarkAttempt(peerRealAddr) + book.MarkGood(peerRealAddr.ID) + + // Double check that adding a peer again doesn't error + err = book.AddAddress(peerRealAddr, src) + require.Nil(t, err) + + // Try changing ip but keeping the same node id. (change 1.1.1.1 to 2.2.2.2) + // This should just be ignored, and not error. + for i := 0; i < numOverrideAttempts; i++ { + err = book.AddAddress(peerOverrideAttemptAddr, src) + require.Nil(t, err) + } + // Now check that the IP was not overridden. + // This is done by sampling several peers from addr book + // and ensuring they all have the correct IP. + // In the expected functionality, this test should only have 1 Peer, hence will pass. + for i := 0; i < numOverrideAttempts; i++ { + selection := book.GetSelection() + for _, addr := range selection { + require.Equal(t, addr.IP, peerRealAddr.IP) + } + } +} + func TestAddrBookGroupKey(t *testing.T) { // non-strict routability testCases := []struct { diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index 8f51d4217..e60166d06 100644 --- a/p2p/pex/errors.go +++ b/p2p/pex/errors.go @@ -15,6 +15,17 @@ func (err ErrAddrBookNonRoutable) Error() string { return fmt.Sprintf("Cannot add non-routable address %v", err.Addr) } +type errAddrBookOldAddressNewBucket struct { + Addr *p2p.NetAddress + BucketID int +} + +func (err errAddrBookOldAddressNewBucket) Error() string { + return fmt.Sprintf("failed consistency check!"+ + " Cannot add pre-existing address %v into new bucket %v", + err.Addr, err.BucketID) +} + type ErrAddrBookSelf struct { Addr *p2p.NetAddress }