diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index 976f0c958..72cc55c44 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/client/firewall/iptables/manager_linux.go @@ -528,7 +528,11 @@ const ( // not always available, so a lookup failure is not an error here. func (m *Manager) cleanupNoTrackChain() error { exists, err := m.ipv4Client.ChainExists(tableRaw, chainNameRaw) - if err != nil || !exists { + if err != nil { + log.Debugf("look up %s chain: %v", chainNameRaw, err) + return nil + } + if !exists { return nil } diff --git a/client/iface/wgproxy/loopback/proxy.go b/client/iface/wgproxy/loopback/proxy.go index 975bb3d58..ea92cf367 100644 --- a/client/iface/wgproxy/loopback/proxy.go +++ b/client/iface/wgproxy/loopback/proxy.go @@ -38,6 +38,7 @@ type Proxy struct { conn *net.UDPConn packetConn *ipv4.PacketConn + loIndex int rawConnIPv4 net.PacketConn rawConnIPv6 net.PacketConn @@ -73,6 +74,12 @@ func (p *Proxy) Listen() error { log.Warnf("failed to prepare IPv6 raw socket, continuing with IPv4 only: %v", err) } + loopback, err := net.InterfaceByName(loopbackDevice) + if err != nil { + return fmt.Errorf("look up %s: %w", loopbackDevice, err) + } + p.loIndex = loopback.Index + if err := p.listen(); err != nil { if freeErr := p.Free(); freeErr != nil { log.Errorf("failed to free the wgproxy: %s", freeErr) @@ -149,11 +156,12 @@ func (p *Proxy) listenOn(proxyPort int) error { } // AddRelayedConn assigns an endpoint address to the relayed connection and -// returns the address WireGuard should send to. -func (p *Proxy) AddRelayedConn(relayedConn net.Conn) (*net.UDPAddr, error) { +// returns the address WireGuard should send to, along with the key the +// connection is stored under. +func (p *Proxy) AddRelayedConn(relayedConn net.Conn) (*net.UDPAddr, netip.Addr, error) { addr, err := p.storeRelayedConn(relayedConn) if err != nil { - return nil, err + return nil, netip.Addr{}, err } log.Infof("relayed conn added to wg proxy store: %s, endpoint address: %s", relayedConn.RemoteAddr(), addr) @@ -161,7 +169,7 @@ func (p *Proxy) AddRelayedConn(relayedConn net.Conn) (*net.UDPAddr, error) { return &net.UDPAddr{ IP: addr.AsSlice(), Port: p.proxyPort, - }, nil + }, addr, nil } // Free releases the proxy resources. The relayed connections are left open. @@ -197,7 +205,6 @@ func (p *Proxy) Free() error { return nberrors.FormatErrorOrNil(result) } - // proxyToRemote reads packets from the local WireGuard instance and forwards // them to the relayed connection the destination address belongs to. func (p *Proxy) proxyToRemote() { @@ -222,6 +229,11 @@ func (p *Proxy) readAndForwardPacket(buf []byte) error { return fmt.Errorf("no control message on packet") } + if cm.IfIndex != p.loIndex { + log.Tracef("dropping packet received on interface %d instead of %s", cm.IfIndex, loopbackDevice) + return nil + } + dst, ok := netip.AddrFromSlice(cm.Dst.To4()) if !ok || !inRange(dst) { log.Tracef("dropping packet for unexpected destination %s", cm.Dst) @@ -260,12 +272,17 @@ func (p *Proxy) storeRelayedConn(relayedConn net.Conn) (netip.Addr, error) { return addr, nil } -func (p *Proxy) removeRelayedConn(addr netip.Addr) { +// removeRelayedConn releases an endpoint address. It only removes the entry +// while it still belongs to relayedConn, so a late release cannot take an +// address away from the peer it was handed to next. +func (p *Proxy) removeRelayedConn(addr netip.Addr, relayedConn net.Conn) { p.relayedConnMutex.Lock() defer p.relayedConnMutex.Unlock() - if _, ok := p.relayedConnStore[addr]; ok { - log.Debugf("remove relayed conn from store by address: %s", addr) + if stored, ok := p.relayedConnStore[addr]; !ok || stored != relayedConn { + return } + + log.Debugf("remove relayed conn from store by address: %s", addr) delete(p.relayedConnStore, addr) } diff --git a/client/iface/wgproxy/loopback/proxy_privileged_test.go b/client/iface/wgproxy/loopback/proxy_privileged_test.go index 3fc3f75b2..e7a7692ff 100644 --- a/client/iface/wgproxy/loopback/proxy_privileged_test.go +++ b/client/iface/wgproxy/loopback/proxy_privileged_test.go @@ -3,6 +3,7 @@ package loopback import ( + "context" "net" "strconv" "testing" @@ -58,7 +59,7 @@ func TestProxyDemuxesByDestinationAddress(t *testing.T) { readers := make([]*net.UDPConn, 0, peers) for i := 0; i < peers; i++ { proxySide, testSide := relayEnd(t) - endpoint, err := proxy.AddRelayedConn(proxySide) + endpoint, _, err := proxy.AddRelayedConn(proxySide) if err != nil { t.Fatalf("add relayed conn %d: %v", i, err) } @@ -135,7 +136,7 @@ func TestProxyDropsPacketsOutsideTheRange(t *testing.T) { }() proxySide, testSide := relayEnd(t) - if _, err := proxy.AddRelayedConn(proxySide); err != nil { + if _, _, err := proxy.AddRelayedConn(proxySide); err != nil { t.Fatalf("add relayed conn: %v", err) } @@ -161,3 +162,35 @@ func TestProxyDropsPacketsOutsideTheRange(t *testing.T) { t.Error("packet addressed to 127.0.0.1 was forwarded to a relayed peer") } } + +// A wrapper that is closed before it starts forwarding still has to give its +// endpoint address back, otherwise the range leaks an address per attempt. +func TestClosingBeforeWorkReleasesTheAddress(t *testing.T) { + proxy := NewProxy(testWGPort+2, 1280) + if err := proxy.Listen(); err != nil { + t.Fatalf("listen: %v", err) + } + defer func() { + if err := proxy.Free(); err != nil { + t.Errorf("free proxy: %v", err) + } + }() + + proxySide, _ := relayEnd(t) + wrapper := NewProxyWrapper(proxy) + if err := wrapper.AddRelayedConn(context.Background(), nil, proxySide); err != nil { + t.Fatalf("add relayed conn: %v", err) + } + + if got := len(proxy.relayedConnStore); got != 1 { + t.Fatalf("store holds %d entries after adding one conn, want 1", got) + } + + if err := wrapper.CloseConn(); err != nil { + t.Fatalf("close conn: %v", err) + } + + if got := len(proxy.relayedConnStore); got != 0 { + t.Errorf("store holds %d entries after close, want 0", got) + } +} diff --git a/client/iface/wgproxy/loopback/wrapper.go b/client/iface/wgproxy/loopback/wrapper.go index 278e5b323..a9cb1ab59 100644 --- a/client/iface/wgproxy/loopback/wrapper.go +++ b/client/iface/wgproxy/loopback/wrapper.go @@ -124,26 +124,28 @@ func NewProxyWrapper(proxy *Proxy) *ProxyWrapper { } func (p *ProxyWrapper) AddRelayedConn(ctx context.Context, _ *net.UDPAddr, remoteConn net.Conn) error { - addr, err := p.proxy.AddRelayedConn(remoteConn) + addr, peerAddr, err := p.proxy.AddRelayedConn(remoteConn) if err != nil { return fmt.Errorf("add relayed conn: %w", err) } - peerAddr, ok := netip.AddrFromSlice(addr.IP.To4()) - if !ok { - return fmt.Errorf("unexpected endpoint address %s", addr.IP) - } + // the endpoint address is otherwise only released by the forwarding + // goroutine, which never starts when the setup below fails + release := func() { p.proxy.removeRelayedConn(peerAddr, remoteConn) } headers, err := NewPacketHeaders(p.proxy.localWGListenPort, addr) if err != nil { + release() return fmt.Errorf("create packet sender: %w", err) } // Check if required raw connection is available if !headers.isIPv4 && p.proxy.rawConnIPv6 == nil { + release() return errIPv6ConnNotAvailable } if headers.isIPv4 && p.proxy.rawConnIPv4 == nil { + release() return errIPv4ConnNotAvailable } @@ -248,6 +250,10 @@ func (p *ProxyWrapper) CloseConn() error { p.closeListener.SetCloseListener(nil) + // releases the endpoint address for a wrapper that was never started, and + // is a no-op once the forwarding goroutine has released it + p.proxy.removeRelayedConn(p.peerAddr, p.remoteConn) + p.pausedCond.L.Lock() p.paused = false p.pausedCond.Signal() @@ -260,7 +266,7 @@ func (p *ProxyWrapper) CloseConn() error { } func (p *ProxyWrapper) proxyToLocal(ctx context.Context) { - defer p.proxy.removeRelayedConn(p.peerAddr) + defer p.proxy.removeRelayedConn(p.peerAddr, p.remoteConn) buf := make([]byte, p.proxy.mtu+bufsize.WGBufferOverhead) for {