diff --git a/p2p/netaddress.go b/p2p/netaddress.go index d11504525..f39a60543 100644 --- a/p2p/netaddress.go +++ b/p2p/netaddress.go @@ -13,7 +13,7 @@ import ( "strings" "time" - "errors" + "github.com/pkg/errors" ) // NetAddress defines information about a peer on the network @@ -40,7 +40,7 @@ func IDAddressString(id ID, protocolHostPort string) string { // NewNetAddress returns a new NetAddress using the provided TCP // address. When testing, other net.Addr (except TCP) will result in // using 0.0.0.0:0. When normal run, other net.Addr (except TCP) will -// panic. +// panic. Panics if ID is invalid. // TODO: socks proxies? func NewNetAddress(id ID, addr net.Addr) *NetAddress { tcpAddr, ok := addr.(*net.TCPAddr) @@ -53,6 +53,11 @@ func NewNetAddress(id ID, addr net.Addr) *NetAddress { return netAddr } } + + if err := validateID(id); err != nil { + panic(fmt.Sprintf("Invalid ID %v: %v (addr: %v)", id, err, addr)) + } + ip := tcpAddr.IP port := uint16(tcpAddr.Port) na := NewNetAddressIPPort(ip, port) @@ -72,18 +77,11 @@ func NewNetAddressString(addr string) (*NetAddress, error) { } // get ID - idStr := spl[0] - idBytes, err := hex.DecodeString(idStr) - if err != nil { + if err := validateID(ID(spl[0])); err != nil { return nil, ErrNetAddressInvalid{addrWithoutProtocol, err} } - if len(idBytes) != IDByteLength { - return nil, ErrNetAddressInvalid{ - addrWithoutProtocol, - fmt.Errorf("invalid hex length - got %d, expected %d", len(idBytes), IDByteLength)} - } var id ID - id, addrWithoutProtocol = ID(idStr), spl[1] + id, addrWithoutProtocol = ID(spl[0]), spl[1] // get host and port host, portStr, err := net.SplitHostPort(addrWithoutProtocol) @@ -207,22 +205,28 @@ func (na *NetAddress) DialTimeout(timeout time.Duration) (net.Conn, error) { // Routable returns true if the address is routable. func (na *NetAddress) Routable() bool { + if err := na.Valid(); err != nil { + return false + } // TODO(oga) bitcoind doesn't include RFC3849 here, but should we? - return na.Valid() && !(na.RFC1918() || na.RFC3927() || na.RFC4862() || + return !(na.RFC1918() || na.RFC3927() || na.RFC4862() || na.RFC4193() || na.RFC4843() || na.Local()) } // For IPv4 these are either a 0 or all bits set address. For IPv6 a zero // address or one that matches the RFC3849 documentation address format. -func (na *NetAddress) Valid() bool { - if string(na.ID) != "" { - data, err := hex.DecodeString(string(na.ID)) - if err != nil || len(data) != IDByteLength { - return false - } +func (na *NetAddress) Valid() error { + if err := validateID(na.ID); err != nil { + return errors.Wrap(err, "invalid ID") } - return na.IP != nil && !(na.IP.IsUnspecified() || na.RFC3849() || - na.IP.Equal(net.IPv4bcast)) + + if na.IP == nil { + return errors.New("no IP") + } + if na.IP.IsUnspecified() || na.RFC3849() || na.IP.Equal(net.IPv4bcast) { + return errors.New("invalid IP") + } + return nil } // HasID returns true if the address has an ID. @@ -329,3 +333,17 @@ func removeProtocolIfDefined(addr string) string { return addr } + +func validateID(id ID) error { + if len(id) == 0 { + return errors.New("no ID") + } + idBytes, err := hex.DecodeString(string(id)) + if err != nil { + return err + } + if len(idBytes) != IDByteLength { + return fmt.Errorf("invalid hex length - got %d, expected %d", len(idBytes), IDByteLength) + } + return nil +} diff --git a/p2p/netaddress_test.go b/p2p/netaddress_test.go index 7afcab131..e7d82cd77 100644 --- a/p2p/netaddress_test.go +++ b/p2p/netaddress_test.go @@ -11,9 +11,13 @@ import ( func TestNewNetAddress(t *testing.T) { tcpAddr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:8080") require.Nil(t, err) - addr := NewNetAddress("", tcpAddr) - assert.Equal(t, "127.0.0.1:8080", addr.String()) + assert.Panics(t, func() { + NewNetAddress("", tcpAddr) + }) + + addr := NewNetAddress("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", tcpAddr) + assert.Equal(t, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", addr.String()) assert.NotPanics(t, func() { NewNetAddress("", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8000}) @@ -106,7 +110,12 @@ func TestNetAddressProperties(t *testing.T) { addr, err := NewNetAddressString(tc.addr) require.Nil(t, err) - assert.Equal(t, tc.valid, addr.Valid()) + err = addr.Valid() + if tc.valid { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } assert.Equal(t, tc.local, addr.Local()) assert.Equal(t, tc.routable, addr.Routable()) } diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 85dd05248..cfe2569ba 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -586,8 +586,8 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error { return ErrAddrBookNilAddr{addr, src} } - if !addr.HasID() { - return ErrAddrBookInvalidAddrNoID{addr} + if err := addr.Valid(); err != nil { + return ErrAddrBookInvalidAddr{Addr: addr, AddrErr: err} } if _, ok := a.privateIDs[addr.ID]; ok { @@ -607,10 +607,6 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error { return ErrAddrBookNonRoutable{addr} } - if !addr.Valid() { - return ErrAddrBookInvalidAddr{addr} - } - ka := a.addrLookup[addr.ID] if ka != nil { // If its already old and the addr is the same, ignore it. diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index 543056af5..911389a9e 100644 --- a/p2p/pex/errors.go +++ b/p2p/pex/errors.go @@ -56,17 +56,10 @@ func (err ErrAddrBookNilAddr) Error() string { } type ErrAddrBookInvalidAddr struct { - Addr *p2p.NetAddress + Addr *p2p.NetAddress + AddrErr error } func (err ErrAddrBookInvalidAddr) Error() string { - return fmt.Sprintf("Cannot add invalid address %v", err.Addr) -} - -type ErrAddrBookInvalidAddrNoID struct { - Addr *p2p.NetAddress -} - -func (err ErrAddrBookInvalidAddrNoID) Error() string { - return fmt.Sprintf("Cannot add address with no ID %v", err.Addr) + return fmt.Sprintf("Cannot add invalid address %v: %v", err.Addr, err.AddrErr) } diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 20862d323..557e7ca75 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -350,22 +350,8 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error { } for _, netAddr := range addrs { - // Validate netAddr. Disconnect from a peer if it sends us invalid data. - if netAddr == nil { - return errors.New("nil address in pexAddrsMessage") - } - // TODO: extract validating logic from NewNetAddressString - // and put it in netAddr#Valid (#2722) - na, err := p2p.NewNetAddressString(netAddr.String()) - if err != nil { - return fmt.Errorf("%s address in pexAddrsMessage is invalid: %v", - netAddr.String(), - err, - ) - } - // NOTE: we check netAddr validity and routability in book#AddAddress. - err = r.book.AddAddress(na, srcAddr) + err = r.book.AddAddress(netAddr, srcAddr) if err != nil { r.logErrAddrBook(err) // XXX: should we be strict about incoming data and disconnect from a