From a68be1973974ddeb7e1f743aabbbead0e43d01f6 Mon Sep 17 00:00:00 2001 From: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> Date: Fri, 10 Jul 2026 14:06:15 -0400 Subject: [PATCH] wgengine/netstack: reject unserved ports on Service (VIP) IPs (#20363) A connection to a Tailscale Service IP on a port the service does not serve was forwarded to the underlying host. `acceptTCP` fell through to the isTailscaleIP case (a VIP is in the Tailscale IP range), which rewrote the dial target to 127.0.0.1: and forwardTCP'd the connection onto whatever unrelated listener happened to be on the host's loopback at that port. This is reachable through the service IP by any peer which was granted access only to the service (dst: svc:foo), so it exposes host ports the peer has no ACL access to via the machine's regular IP. This happens when there tailscaled has a Tun interface and the forward bits are set. In this commit, we added a guard in acceptTCP, before the isTailscaleIP case that RSTs connections to a VIP service IP on a port with no serve handler. Served ports return earlier via TCPHandlerForDst, so only unserved ports reach the guard. Layer 3 services are unaffected: their traffic is released to the host in injectInbound and never reaches acceptTCP. Fixes #20362 Signed-off-by: kevinliang10 --- wgengine/netstack/netstack.go | 11 ++ wgengine/netstack/netstack_test.go | 285 ++++++++++++++++++++--------- 2 files changed, 211 insertions(+), 85 deletions(-) diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index 659e07924..1ed42df91 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -1683,6 +1683,17 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) { // here instead. r.Complete(true) // sends a RST return + case ns.isVIPServiceIP(dialIP): + // TCP to a VIP service IP on a port the service does not serve. A served + // port returns early above (TCPHandlerForDst is non-nil), so reaching here + // means this node has no serve handler for this port. Don't fall through + // to the isTailscaleIP case below (a VIP is in the Tailscale IP range), + // which would rewrite the dial target to 127.0.0.1: and forwardTCP + // the connection onto whatever unrelated service happens to be listening + // on the host's loopback at that port — reachable via the service IP by + // any peer, even one granted access only to the service. Reject with a RST. + r.Complete(true) // sends a RST + return case isTailscaleIP: dialIP = ipv4Loopback } diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index 8732ab782..f0e1b04b1 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -1095,6 +1095,30 @@ func TestHandleLocalPackets(t *testing.T) { t.Errorf("got filter outcome %v, want filter.DropSilently", resp) } }) + t.Run("ShouldNotHandleInactiveVIPService", func(t *testing.T) { + // Tests that packets to Tailscale Services we don't host are accepted. + inactiveVIP := netip.MustParseAddr("100.99.55.222") + impl.lb.ForTest().SetIPServiceMappings(netmap.IPServiceMappings{ + netip.MustParseAddr("100.99.55.111"): "svc:test-service", // active (shared fixture) + netip.MustParseAddr("fd7a:115c:a1e0::abcd"): "svc:test-service", + inactiveVIP: "svc:inactive-elsewhere", + }) + t.Cleanup(func() { + // Restore the shared fixture for any later/parallel subtests. + impl.lb.ForTest().SetIPServiceMappings(IPServiceMap) + }) + pkt := &packet.Parsed{ + IPVersion: 4, + IPProto: ipproto.TCP, + Src: netip.MustParseAddrPort("127.0.0.1:9999"), + Dst: netip.AddrPortFrom(inactiveVIP, 80), + TCPFlags: packet.TCPSyn, + } + resp, _ := impl.handleLocalPackets(pkt, impl.tundev, nil) + if resp != filter.Accept { + t.Errorf("inactive VIP service: got filter outcome %v, want filter.Accept (pass through to host)", resp) + } + }) t.Run("OtherNonHandled", func(t *testing.T) { t.Parallel() pkt := &packet.Parsed{ @@ -1119,97 +1143,188 @@ func TestHandleLocalPackets(t *testing.T) { }) } -// TestQuad100UnservedTCPPortDoesNotForward verifies that a TCP SYN to the -// Tailscale service IP (100.100.100.100) on a port we don't serve is -// absorbed by netstack and rejected cleanly, without triggering the -// outbound forwardTCP dialer. +// TestAcceptTCPRoutingTailscaleIPRange tests how acceptTCP behaves for TCP SYN +// packets destined to IPs in the Tailscale range (100.64.0.0/10). // -// handleLocalPackets now absorbs all quad-100 traffic regardless of -// port to prevent it leaking to WireGuard peers (which produced noisy -// "open-conn-track: timeout opening ...; no associated peer node" log -// lines). That leaves acceptTCP responsible for rejecting connections -// to ports we don't handle; without an explicit guard, execution would -// fall through to the isTailscaleIP case (quad-100 is in the tailscale -// range), rewriting the dial target to 127.0.0.1: and forwarding -// the connection to whatever random service happened to be listening -// on the host's loopback at that port. -// -// This test asserts that the forward dialer is NOT invoked for quad-100 -// SYNs on unserved ports; the guard in acceptTCP must RST instead. -func TestQuad100UnservedTCPPortDoesNotForward(t *testing.T) { - impl := makeNetstack(t, func(impl *Impl) { - impl.ProcessSubnets = false - impl.ProcessLocalIPs = false - impl.atomicIsLocalIPFunc.Store(looksLikeATailscaleSelfAddress) - }) +// - Packets to the Tailscale IP should be forwarded to loopback if there is +// no configured handler for the port. +// - Packets to the service IP (100.100.100.100) on non-served ports should +// never make it to the local host. +// - Packets to a Tailscale Service VIP on non-served ports should never make +// it to the local host. +func TestAcceptTCPLoopbackForwardVsRST(t *testing.T) { + serviceVIP := netip.MustParseAddr("100.90.1.2") + selfIP := netip.MustParseAddr("100.64.1.2") + const serviceName = "svc:test" - dialFn, gotConn := makeHangDialer(t) - impl.forwardDialFunc = dialFn - - // Use a client IP in the CGNAT range so shouldProcessInbound-adjacent - // code paths treat this as plausibly-peer-sourced traffic, matching - // what a real stray quad-100 probe from the host OS would look like. - client := netip.MustParseAddr("100.101.102.103") - quad100 := tsaddr.TailscaleServiceIP() - - // 853 is DoT, the specific case called out in the original bug - // report ("conntrack error no peer found for 100.100.100.100:853"). - // Before the fix, port 853 (and any non-{53,80,8080} port) leaked - // out to WireGuard; after the fix it is absorbed here and must NOT - // trigger forwardTCP. - pkt := tcp4syn(t, client, quad100, 1234, 853) - var parsed packet.Parsed - parsed.Decode(pkt) - - resp, _ := impl.handleLocalPackets(&parsed, impl.tundev, nil) - if resp != filter.DropSilently { - t.Fatalf("handleLocalPackets for quad-100:853: got %v, want filter.DropSilently", resp) + cases := []struct { + name string + // configure runs inside makeNetstack, before Start. + configure func(*Impl) + // afterStart runs after Start, for state that requires the backend to be + // running (prefs, service IP maps, NIC address registration). Optional. + afterStart func(t *testing.T, impl *Impl) + // dst is the SYN's destination. Its port has no registered handler. + dst netip.AddrPort + // inbound selects the injection path: true => injectInbound (a packet + // from a peer), false => handleLocalPackets (host-originated, or the + // kernel-hairpin re-entry). + inbound bool + // wantForward is whether acceptTCP should forward the connection to the + // host's loopback (true) or reject it with a RST (false). + wantForward bool + }{ + { + name: "Quad-100UnservedPortIsRST", + configure: func(impl *Impl) { + impl.ProcessSubnets = false + impl.ProcessLocalIPs = false + impl.atomicIsLocalIPFunc.Store(looksLikeATailscaleSelfAddress) + }, + // 853 is DoT, the specific case called out in the original bug + // report ("conntrack error no peer found for 100.100.100.100:853"). + // Before the fix, port 853 (and any non-{53,80,8080} port) leaked + // out to WireGuard; after the fix it is absorbed and must NOT + // trigger forwardTCP. handleLocalPackets absorbs all quad-100 + // traffic regardless of port to prevent it leaking to WireGuard + // peers (which produced noisy "open-conn-track: timeout opening ...; + // no associated peer node" log lines), leaving acceptTCP to reject + // the unserved port with a RST rather than falling through to the + // isTailscaleIP loopback rewrite. + dst: netip.AddrPortFrom(tsaddr.TailscaleServiceIP(), 853), + wantForward: false, + }, + { + name: "VIPServiceUnservedPortIsRST", + configure: func(impl *Impl) { + impl.ProcessSubnets = false + impl.ProcessLocalIPs = false + impl.atomicIsLocalIPFunc.Store(looksLikeATailscaleSelfAddress) + impl.atomicIsVIPServiceIPFunc.Store(func(addr netip.Addr) bool { + return addr == serviceVIP + }) + }, + afterStart: func(t *testing.T, impl *Impl) { + // Mark the service as one this node hosts and is actively + // serving, so handleLocalPackets absorbs its traffic into + // netstack (rather than letting a non-hosted VIP route through). + // The service serves no TCP ports here, so every port is + // "non-served". AdvertiseServices flows through to + // UpdateActiveVIPServices, marking the service active. + prefs := ipn.NewPrefs() + prefs.AdvertiseServices = []string{serviceName} + if _, err := impl.lb.EditPrefs(&ipn.MaskedPrefs{ + Prefs: *prefs, + AdvertiseServicesSet: true, + }); err != nil { + t.Fatalf("EditPrefs: %v", err) + } + impl.lb.ForTest().SetIPServiceMappings(netmap.IPServiceMappings{serviceVIP: serviceName}) + }, + dst: netip.AddrPortFrom(serviceVIP, 8001), // 8001: a port the service doesn't serve + wantForward: false, + }, + { + name: "LocalTailscaleIPUnhandledPortForwardsToLoopback", + configure: func(impl *Impl) { + impl.ProcessSubnets = false + // ProcessLocalIPs=true so an inbound packet to a local Tailscale + // IP is absorbed into netstack and dispatched to acceptTCP. + impl.ProcessLocalIPs = true + impl.atomicIsLocalIPFunc.Store(func(addr netip.Addr) bool { + return addr == selfIP + }) + }, + // 9999 has no SSH/webclient/peerapi/serve handler, so + // TCPHandlerForDst returns nil and acceptTCP falls to the + // isTailscaleIP case, which rewrites the dial to 127.0.0.1:9999 and + // calls forwardTCP. This is how local handlers reach the host, and + // the RST guards above must not swallow it. + dst: netip.AddrPortFrom(selfIP, 9999), + inbound: true, + wantForward: true, + }, } - // acceptTCP runs asynchronously in the gVisor TCP dispatcher after - // handleLocalPackets injects the packet into netstack. Use the - // in-flight connection counter as a deterministic synchronization - // point: wrapTCPProtocolHandler increments connsInFlightByClient - // when the dispatcher hands the connection off to acceptTCP, and - // acceptTCP's deferred decrementInFlightTCPForward decrements it - // on return. - // - // On the green path (RST guard fires), acceptTCP returns promptly - // and the counter reaches 0. On the red path (fall-through to - // forwardTCP), acceptTCP blocks inside the forwardDialFunc call — - // makeHangDialer signals gotConn on entry (buffered, non-blocking) - // and then blocks forever — so the counter never reaches 0 but - // gotConn fires synchronously from the dispatcher goroutine. A - // select on both races those outcomes without real-time padding. - // - // testing/synctest is not usable here: gVisor's sleep package calls - // the runtime's gopark directly rather than via the standard - // library, so synctest.Wait() cannot observe those goroutines - // becoming durably blocked and hangs indefinitely. - inFlightZero := make(chan struct{}) - go func() { - for { - impl.mu.Lock() - n := impl.connsInFlightByClient[client] - impl.mu.Unlock() - if n == 0 { - close(inFlightZero) - return + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + impl := makeNetstack(t, tc.configure) + if tc.afterStart != nil { + tc.afterStart(t, impl) } - time.Sleep(time.Millisecond) - } - }() - select { - case <-gotConn: - t.Fatalf("forwardDialFunc was called for quad-100:853; acceptTCP fell through to forwardTCP instead of sending RST. This means stray traffic to quad-100 on unserved ports is being redirected to the host's loopback at the same port.") - case <-inFlightZero: - // acceptTCP returned cleanly; the RST guard fired. - case <-time.After(5 * time.Second): - // Safety net so a regression in the in-flight counter plumbing - // doesn't hang the whole test run; both outcomes above should - // fire within milliseconds in practice. - t.Fatal("timed out waiting for acceptTCP to dispatch quad-100:853 SYN") + dialFn, gotConn := makeHangDialer(t) + impl.forwardDialFunc = dialFn + + // Use a client IP in the CGNAT range so shouldProcessInbound-adjacent + // code paths treat this as plausibly-peer-sourced traffic, matching + // what a real stray probe from a peer or the host OS would look like. + client := netip.MustParseAddr("100.101.102.103") + pkt := tcp4syn(t, client, tc.dst.Addr(), 1234, tc.dst.Port()) + var parsed packet.Parsed + parsed.Decode(pkt) + + // Both injection paths absorb the packet into netstack, returning + // filter.DropSilently (i.e. not handing it to the host); acceptTCP + // is then dispatched with the packet. + inject := impl.handleLocalPackets + if tc.inbound { + inject = impl.injectInbound + } + if resp, _ := inject(&parsed, impl.tundev, nil); resp != filter.DropSilently { + t.Fatalf("inject for %v: got filter outcome %v, want filter.DropSilently", tc.dst, resp) + } + + // acceptTCP runs asynchronously in the gVisor TCP dispatcher after + // handleLocalPackets injects the packet into netstack. Use the + // in-flight connection counter as a deterministic synchronization + // point: wrapTCPProtocolHandler increments connsInFlightByClient + // when the dispatcher hands the connection off to acceptTCP, and + // acceptTCP's deferred decrementInFlightTCPForward decrements it + // on return. + // + // On the green path (RST guard fires), acceptTCP returns promptly + // and the counter reaches 0. On the red path (fall-through to + // forwardTCP), acceptTCP blocks inside the forwardDialFunc call — + // makeHangDialer signals gotConn on entry (buffered, non-blocking) + // and then blocks forever — so the counter never reaches 0 but + // gotConn fires synchronously from the dispatcher goroutine. A + // select on both races those outcomes without real-time padding. + // + // testing/synctest is not usable here: gVisor's sleep package calls + // the runtime's gopark directly rather than via the standard + // library, so synctest.Wait() cannot observe those goroutines + // becoming durably blocked and hangs indefinitely. + inFlightZero := make(chan struct{}) + go func() { + for { + impl.mu.Lock() + n := impl.connsInFlightByClient[client] + impl.mu.Unlock() + if n == 0 { + close(inFlightZero) + return + } + time.Sleep(time.Millisecond) + } + }() + + select { + case <-gotConn: + if !tc.wantForward { + t.Fatalf("forwardDialFunc was called for %v; acceptTCP forwarded to the host's loopback instead of sending a RST", tc.dst) + } + case <-inFlightZero: + if tc.wantForward { + t.Fatalf("forwardDialFunc was NOT called for %v; acceptTCP rejected the connection instead of forwarding it to loopback", tc.dst) + } + case <-time.After(5 * time.Second): + // Safety net so a regression in the in-flight counter plumbing + // doesn't hang the whole test run; both outcomes above should + // fire within milliseconds in practice. + t.Fatalf("timed out waiting for acceptTCP to dispatch %v SYN", tc.dst) + } + }) } }