Ensure that peers can't change IP of known nodes

[ValarDragon]

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/master/p2p/pex/addrbook.go#L522)

I did not include a test with this code, because the relevant defensive test did not return an error but instead logged an error.

The code which one can use to manually test this against the previous code is:

func Test1581(t *testing.T) {
        fname := createTempFileName("addrbook_test")
        defer deleteTempFile(fname)

        addr, err := p2p.NewNetAddressString("678503e6c8f50db7279c7da3c$
        require.Nil(t, err)
        src, err := p2p.NewNetAddressString("b0dd378c3fbc4c156cd6d302a7$
        require.Nil(t, err)

        book := NewAddrBook(fname, true)
        book.SetLogger(log.TestingLogger())
        err = book.AddAddress(addr, src)
        require.Nil(t, err)
        book.MarkAttempt(addr)
        book.MarkGood(addr.ID)

        // Double check that adding a peer again doesn't error
        err = book.AddAddress(addr, src)
        require.Nil(t, err)

        // Try changing ip but keeping the same node id. (change it to $
        addr2, err := p2p.NewNetAddressString("678503e6c8f50db7279c7da3$
        require.Nil(t, err)
        err = book.AddAddress(addr2, src)
        require.Nil(t, err)
}

If you see a logged error after running pex$ go test -run 1581 -v a few times, the error is present.

[auto-comment]

👋 Thanks for creating a PR!

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Wrote tests
• Updated CHANGELOG_PENDING.md
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/) and code comments
• Re-reviewed Files changed in the Github PR explorer
• Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

[melekes]

💯

Thanks for fixing this @ValarDragon!

What do you think about modifying addToNewBucket to return an error

tendermint/p2p/pex/addrbook.go

Line 520 in 8cdb53c

func (a *addrBook) addToNewBucket(ka *knownAddress, bucketIdx int) {

and adding the test you wrote?

[ValarDragon]

Sure I can change that!

[xla]

Dope!

⛩ 🎄 🎲 🍘

[ValarDragon]

Maybe sending you peer's with the same ID but different IP should be deemed as malicious behaviour / worth lowering your peer score for?

Its tricky since you can't tell which peer sent you the correct IP. Maybe its worth a debug message at least? If so I can go ahead and add one

Figured I should flag this in case its worth further discussion

[xla]

This sounds more suited to be discussed outside of this PR, maybe open up an issue with the open question?

[ValarDragon]

Will do!

[ebuchman]

I feel like there may have been some initial motivation to allow the IP address associated with an ID to change. One could imaging rebooting a node eg on AWS or otherwise and keeping the same config data (ie. ID) but getting a new IP. Would such a case still be accounted for? Would such a peer now be treated as malicious?

[ValarDragon]

This is definitely tricky to handle correctly. (I've been having trouble writing the issue, due to me having unclear thoughts on any particular solution idea)

In the code as of this PR, AddAddress would ignore the new node with a different IP. I think this means it would only send messages to the old IP. However noone is marked malicious.

If we wanted to support this, we'd have to authenticate the incoming connection via secret connection before updating the IP, and maybe have an update methon on the address book.

[codecov]

Codecov Report

Merging #5136 into master will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #5136      +/-   ##
==========================================
- Coverage   62.60%   62.59%   -0.02%     
==========================================
  Files         256      256              
  Lines       27172    27175       +3     
==========================================
- Hits        17012    17010       -2     
- Misses       8665     8677      +12     
+ Partials     1495     1488       -7     

Impacted Files	Coverage Δ
p2p/pex/errors.go	0.00% <0.00%> (ø)
p2p/pex/addrbook.go	72.48% <83.33%> (+0.59%)	⬆️
p2p/switch.go	67.46% <0.00%> (-1.87%)	⬇️
blockchain/v0/reactor.go	62.32% <0.00%> (-1.87%)	⬇️
consensus/reactor.go	77.20% <0.00%> (-1.05%)	⬇️
blockchain/v0/pool.go	79.29% <0.00%> (-0.32%)	⬇️
p2p/pex/pex_reactor.go	80.95% <0.00%> (ø)
proxy/multi_app_conn.go	58.02% <0.00%> (ø)
consensus/state.go	71.78% <0.00%> (+0.27%)	⬆️
consensus/replay.go	73.41% <0.00%> (+0.84%)	⬆️
... and 3 more