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:<port> 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 <kevinliang@tailscale.com>

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:<port> 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
 	}

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,51 +1143,136 @@ func TestHandleLocalPackets(t *testing.T) {
 	})
 }
 
-// TestQuad100UnservedTCPPortDoesNotForward verifies that a TCP SYN to the
+// TestAcceptTCPRoutingTailscaleIPRange tests how acceptTCP behaves for TCP SYN
-// Tailscale service IP (100.100.100.100) on a port we don't serve is
+// packets destined to IPs in the Tailscale range (100.64.0.0/10).
-// absorbed by netstack and rejected cleanly, without triggering the
-// outbound forwardTCP dialer.
 //
-// handleLocalPackets now absorbs all quad-100 traffic regardless of
+//   - Packets to the Tailscale IP should be forwarded to loopback if there is
-// port to prevent it leaking to WireGuard peers (which produced noisy
+//     no configured handler for the port.
-// "open-conn-track: timeout opening ...; no associated peer node" log
+//   - Packets to the service IP (100.100.100.100) on non-served ports should
-// lines). That leaves acceptTCP responsible for rejecting connections
+//     never make it to the local host.
-// to ports we don't handle; without an explicit guard, execution would
+//   - Packets to a Tailscale Service VIP on non-served ports should never make
-// fall through to the isTailscaleIP case (quad-100 is in the tailscale
+//     it to the local host.
-// range), rewriting the dial target to 127.0.0.1:<port> and forwarding
+func TestAcceptTCPLoopbackForwardVsRST(t *testing.T) {
-// the connection to whatever random service happened to be listening
+	serviceVIP := netip.MustParseAddr("100.90.1.2")
-// on the host's loopback at that port.
+	selfIP := netip.MustParseAddr("100.64.1.2")
-//
+	const serviceName = "svc:test"
-// This test asserts that the forward dialer is NOT invoked for quad-100
+
-// SYNs on unserved ports; the guard in acceptTCP must RST instead.
+	cases := []struct {
-func TestQuad100UnservedTCPPortDoesNotForward(t *testing.T) {
+		name string
-	impl := makeNetstack(t, func(impl *Impl) {
+		// 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,
+		},
+	}
+
+	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)
+			}
 
 			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.
+			// what a real stray probe from a peer or the host OS would look like.
 			client := netip.MustParseAddr("100.101.102.103")
-	quad100 := tsaddr.TailscaleServiceIP()
+			pkt := tcp4syn(t, client, tc.dst.Addr(), 1234, tc.dst.Port())
-
-	// 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)
+			// Both injection paths absorb the packet into netstack, returning
-	if resp != filter.DropSilently {
+			// filter.DropSilently (i.e. not handing it to the host); acceptTCP
-		t.Fatalf("handleLocalPackets for quad-100:853: got %v, want filter.DropSilently", resp)
+			// 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
@@ -1202,14 +1311,20 @@ func TestQuad100UnservedTCPPortDoesNotForward(t *testing.T) {
 
 			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.")
+				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:
-		// acceptTCP returned cleanly; the RST guard fired.
+				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.Fatal("timed out waiting for acceptTCP to dispatch quad-100:853 SYN")
+				t.Fatalf("timed out waiting for acceptTCP to dispatch %v SYN", tc.dst)
+			}
+		})
 	}
 }