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) + } + }) } }