security/p2p: prevent peers who errored being added to the peer_set (…

…backport #9500) (#9516)

* security/p2p: prevent peers who errored being added to the peer_set (#9500)

* Mark failed removal of peer to address security bug

Co-authored-by: Callum Waters <cmwaters19@gmail.com>
(cherry picked from commit c0bdb2423acef508372a3750d0db3e0dd9982178)

* Changelong entry and added missing functions for implementations of Peer

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

blockchain/v2/reactor_test.go

@@ -62,6 +62,9 @@ func (mp mockPeer) Get(string) interface{}  { return struct{}{} }
 
 func (mp mockPeer) String() string { return fmt.Sprintf("%v", mp.id) }
 
+func (mp mockPeer) SetRemovalFailed()      {}
+func (mp mockPeer) GetRemovalFailed() bool { return false }
+
 // nolint:unused // ignore
 type mockBlockStore struct {
 	blocks map[int64]*types.Block

p2p/errors.go

@@ -145,6 +145,13 @@ func (e ErrTransportClosed) Error() string {
 	return "transport has been closed"
 }
 
+// ErrPeerRemoval is raised when attempting to remove a peer results in an error.
+type ErrPeerRemoval struct{}
+
+func (e ErrPeerRemoval) Error() string {
+	return "peer removal failed"
+}
+
 //-------------------------------------------------------------------
 
 type ErrNetAddressNoID struct {

p2p/mock/peer.go

@@ -68,3 +68,5 @@ func (mp *Peer) RemoteIP() net.IP            { return mp.ip }
 func (mp *Peer) SocketAddr() *p2p.NetAddress { return mp.addr }
 func (mp *Peer) RemoteAddr() net.Addr        { return &net.TCPAddr{IP: mp.ip, Port: 8800} }
 func (mp *Peer) CloseConn() error            { return nil }
+func (mp *Peer) SetRemovalFailed()           {}
+func (mp *Peer) GetRemovalFailed() bool      { return false }

p2p/peer.go

@@ -38,6 +38,9 @@ type Peer interface {
 
 	Set(string, interface{})
 	Get(string) interface{}
+
+	SetRemovalFailed()
+	GetRemovalFailed() bool
 }
 
 type BufferedMsg struct {
@@ -122,6 +125,9 @@ type peer struct {
 
 	metrics       *Metrics
 	metricsTicker *time.Ticker
+
+	// When removal of a peer fails, we set this flag
+	removalAttemptFailed bool
 }
 
 type PeerOption func(*peer)
@@ -321,6 +327,14 @@ func (p *peer) CloseConn() error {
 	return p.peerConn.conn.Close()
 }
 
+func (p *peer) SetRemovalFailed() {
+	p.removalAttemptFailed = true
+}
+
+func (p *peer) GetRemovalFailed() bool {
+	return p.removalAttemptFailed
+}
+
 //---------------------------------------------------
 // methods only used for testing
 // TODO: can we remove these?

p2p/peer_set.go

@@ -47,6 +47,9 @@ func (ps *PeerSet) Add(peer Peer) error {
 	if ps.lookup[peer.ID()] != nil {
 		return ErrSwitchDuplicatePeerID{peer.ID()}
 	}
+	if peer.GetRemovalFailed() {
+		return ErrPeerRemoval{}
+	}
 
 	index := len(ps.list)
 	// Appending is safe even with other goroutines
@@ -107,6 +110,12 @@ func (ps *PeerSet) Remove(peer Peer) bool {
 
 	item := ps.lookup[peer.ID()]
 	if item == nil {
+		// Removing the peer has failed so we set a flag to mark that a removal was attempted.
+		// This can happen when the peer add routine from the switch is running in
+		// parallel to the receive routine of MConn.
+		// There is an error within MConn but the switch has not actually added the peer to the peer set yet.
+		// Setting this flag will prevent a peer from being added to a node's peer set afterwards.
+		peer.SetRemovalFailed()
 		return false
 	}
 

p2p/peer_set_test.go

@@ -32,6 +32,8 @@ func (mp *mockPeer) RemoteIP() net.IP                        { return mp.ip }
 func (mp *mockPeer) SocketAddr() *NetAddress                 { return nil }
 func (mp *mockPeer) RemoteAddr() net.Addr                    { return &net.TCPAddr{IP: mp.ip, Port: 8800} }
 func (mp *mockPeer) CloseConn() error                        { return nil }
+func (mp *mockPeer) SetRemovalFailed()                       {}
+func (mp *mockPeer) GetRemovalFailed() bool                  { return false }
 
 // Returns a mock peer
 func newMockPeer(ip net.IP) *mockPeer {

p2p/switch.go

@@ -371,6 +371,10 @@ func (sw *Switch) stopAndRemovePeer(peer Peer, reason interface{}) {
 	// https://github.com/tendermint/tendermint/issues/3338
 	if sw.peers.Remove(peer) {
 		sw.metrics.Peers.Add(float64(-1))
+	} else {
+		// Removal of the peer has failed. The function above sets a flag within the peer to mark this.
+		// We keep this message here as information to the developer.
+		sw.Logger.Debug("error on peer removal", ",", "peer", peer.ID())
 	}
 }
 
@@ -825,6 +829,12 @@ func (sw *Switch) addPeer(p Peer) error {
 	// so that if Receive errors, we will find the peer and remove it.
 	// Add should not err since we already checked peers.Has().
 	if err := sw.peers.Add(p); err != nil {
+		switch err.(type) {
+		case ErrPeerRemoval:
+			sw.Logger.Error("Error starting peer ",
+				" err ", "Peer has already errored and removal was attempted.",
+				"peer", p.ID())
+		}
 		return err
 	}
 	sw.metrics.Peers.Add(float64(1))

p2p/switch_test.go

@@ -837,6 +837,19 @@ func BenchmarkSwitchBroadcast(b *testing.B) {
 	b.Logf("success: %v, failure: %v", numSuccess, numFailure)
 }
 
+func TestSwitchRemovalErr(t *testing.T) {
+
+	sw1, sw2 := MakeSwitchPair(t, func(i int, sw *Switch, config *config.P2PConfig) *Switch {
+		return initSwitchFunc(i, sw, config)
+	})
+	assert.Equal(t, len(sw1.Peers().List()), 1)
+	p := sw1.Peers().List()[0]
+
+	sw2.StopPeerForError(p, fmt.Errorf("peer should error"))
+
+	assert.Equal(t, sw2.peers.Add(p).Error(), ErrPeerRemoval{}.Error())
+}
+
 type addrBookMock struct {
 	addrs    map[string]struct{}
 	ourAddrs map[string]struct{}

test/fuzz/p2p/pex/reactor_receive.go

@@ -84,3 +84,5 @@ func (fp *fuzzPeer) Send(byte, []byte) bool            { return true }
 func (fp *fuzzPeer) TrySend(byte, []byte) bool         { return true }
 func (fp *fuzzPeer) Set(key string, value interface{}) { fp.m[key] = value }
 func (fp *fuzzPeer) Get(key string) interface{}        { return fp.m[key] }
+func (fp *fuzzPeer) SetRemovalFailed()                 {}
+func (fp *fuzzPeer) GetRemovalFailed() bool            { return false }