From cdba0d82f51d6e0b463b9b3cc4e7039564abbc11 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Wed, 22 Jul 2020 00:56:38 -0500 Subject: [PATCH] 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 https://github.com/tendermint/tendermint/blob/95fc7e58eedd3b0a563b93a0691c9cbe6e10f017/p2p/pex/addrbook.go#L522) --- CHANGELOG_PENDING.md | 1 + p2p/pex/addrbook.go | 19 +++++++------- p2p/pex/addrbook_test.go | 55 ++++++++++++++++++++++++++++++++++++++++ p2p/pex/errors.go | 11 ++++++++ 4 files changed, 77 insertions(+), 9 deletions(-) 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 }