p2p: ensure peers can't change IP of known nodes (#5136)

Closes #1581 

This fixes the error in #1581, and also documents the purpose of this line. It ensures that if a peer tells us an address we know about, whose ID is the same as our current ID, we ignore it.

This removes the previous case where the ID's matched, but the IP's did not, which could yield a potential overwrite of the IP associated with the address later on. (This then would yield an eclipse attack)

This was not a vulnerability before though, thanks to a defensive check here 95fc7e58ee/p2p/pex/addrbook.go (L522))

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)

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 {

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 {

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
 }