From fb63833cacb59db697c80edaba6bfadb49a64db6 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 25 Aug 2021 12:51:31 +0100 Subject: [PATCH 1/3] set both read and write deadline in the stream handler A malicious peer could also block writes to a stream (for example by withholding flow control credit). --- svc.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/svc.go b/svc.go index c237aa4..6bd72f1 100644 --- a/svc.go +++ b/svc.go @@ -7,17 +7,16 @@ import ( "sync" "time" + pb "github.com/libp2p/go-libp2p-autonat/pb" "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" "github.com/libp2p/go-libp2p-core/peerstore" - pb "github.com/libp2p/go-libp2p-autonat/pb" - "github.com/libp2p/go-msgio/protoio" ma "github.com/multiformats/go-multiaddr" ) -var streamReadTimeout = 60 * time.Second +var streamTimeout = 60 * time.Second // AutoNATService provides NAT autodetection services to other peers type autoNATService struct { @@ -49,8 +48,7 @@ func newAutoNATService(ctx context.Context, c *config) (*autoNATService, error) } func (as *autoNATService) handleStream(s network.Stream) { - s.SetReadDeadline(time.Now().Add(streamReadTimeout)) - + s.SetDeadline(time.Now().Add(streamTimeout)) defer s.Close() pid := s.Conn().RemotePeer() From 297d9e9365e320490141b4a897077f1d1cc16d3e Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 25 Aug 2021 12:53:57 +0100 Subject: [PATCH 2/3] also set a deadline on the stream opened by the client --- client.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/client.go b/client.go index b9ce1bf..e078ee3 100644 --- a/client.go +++ b/client.go @@ -3,23 +3,17 @@ package autonat import ( "context" "fmt" + "time" + pb "github.com/libp2p/go-libp2p-autonat/pb" "github.com/libp2p/go-libp2p-core/host" "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" + "github.com/libp2p/go-msgio/protoio" - pb "github.com/libp2p/go-libp2p-autonat/pb" - - protoio "github.com/libp2p/go-msgio/protoio" ma "github.com/multiformats/go-multiaddr" ) -// Error wraps errors signalled by AutoNAT services -type Error struct { - Status pb.Message_ResponseStatus - Text string -} - // NewAutoNATClient creates a fresh instance of an AutoNATClient // If addrFunc is nil, h.Addrs will be used func NewAutoNATClient(h host.Host, addrFunc AddrFunc) Client { @@ -34,11 +28,14 @@ type client struct { addrFunc AddrFunc } +// DialBack asks peer p to dial us back on all addresses returned by the addrFunc. +// It blocks until we've received a response from the peer. func (c *client) DialBack(ctx context.Context, p peer.ID) (ma.Multiaddr, error) { s, err := c.h.NewStream(ctx, p, AutoNATProto) if err != nil { return nil, err } + s.SetDeadline(time.Now().Add(streamTimeout)) // Might as well just reset the stream. Once we get to this point, we // don't care about being nice. defer s.Close() @@ -47,15 +44,13 @@ func (c *client) DialBack(ctx context.Context, p peer.ID) (ma.Multiaddr, error) w := protoio.NewDelimitedWriter(s) req := newDialMessage(peer.AddrInfo{ID: c.h.ID(), Addrs: c.addrFunc()}) - err = w.WriteMsg(req) - if err != nil { + if err := w.WriteMsg(req); err != nil { s.Reset() return nil, err } var res pb.Message - err = r.ReadMsg(&res) - if err != nil { + if err := r.ReadMsg(&res); err != nil { s.Reset() return nil, err } @@ -69,12 +64,17 @@ func (c *client) DialBack(ctx context.Context, p peer.ID) (ma.Multiaddr, error) case pb.Message_OK: addr := res.GetDialResponse().GetAddr() return ma.NewMultiaddrBytes(addr) - default: return nil, Error{Status: status, Text: res.GetDialResponse().GetStatusText()} } } +// Error wraps errors signalled by AutoNAT services +type Error struct { + Status pb.Message_ResponseStatus + Text string +} + func (e Error) Error() string { return fmt.Sprintf("AutoNAT error: %s (%s)", e.Text, e.Status.String()) } From 2e692c3f27019af572b71773441add4df8885c0f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 25 Aug 2021 12:54:32 +0100 Subject: [PATCH 3/3] add missing stream reset when the server sent the wrong message type --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index e078ee3..3c4c563 100644 --- a/client.go +++ b/client.go @@ -54,8 +54,8 @@ func (c *client) DialBack(ctx context.Context, p peer.ID) (ma.Multiaddr, error) s.Reset() return nil, err } - if res.GetType() != pb.Message_DIAL_RESPONSE { + s.Reset() return nil, fmt.Errorf("unexpected response: %s", res.GetType().String()) }