wgengine/netstack: handle 4via6 packets when pinging (#5709)

Change-Id: Ib6ebbaa11219fb91b550ed7fc6ede61f83262e89
Signed-off-by: Andrew Dunham <andrew@tailscale.com>

wgengine/netstack/netstack.go

@@ -664,7 +664,11 @@ func (ns *Impl) injectInbound(p *packet.Parsed, t *tstun.Wrapper) filter.Respons
 	}
 
 	destIP := p.Dst.Addr()
-	if p.IsEchoRequest() && ns.ProcessSubnets && !tsaddr.IsTailscaleIP(destIP) {
+
+	// If this is an echo request and we're a subnet router, handle pings
+	// ourselves instead of forwarding the packet on.
+	pingIP, handlePing := ns.shouldHandlePing(p)
+	if handlePing {
 		var pong []byte // the reply to the ping, if our relayed ping works
 		if destIP.Is4() {
 			h := p.ICMP4Header()
@@ -675,7 +679,7 @@ func (ns *Impl) injectInbound(p *packet.Parsed, t *tstun.Wrapper) filter.Respons
 			h.ToResponse()
 			pong = packet.Generate(&h, p.Payload())
 		}
-		go ns.userPing(destIP, pong)
+		go ns.userPing(pingIP, pong)
 		return filter.DropSilently
 	}
 
@@ -704,6 +708,43 @@ func (ns *Impl) injectInbound(p *packet.Parsed, t *tstun.Wrapper) filter.Respons
 	return filter.DropSilently
 }
 
+// shouldHandlePing returns whether or not netstack should handle an incoming
+// ICMP echo request packet, and the IP address that should be pinged from this
+// process. The IP address can be different from the destination in the packet
+// if the destination is a 4via6 address.
+func (ns *Impl) shouldHandlePing(p *packet.Parsed) (_ netip.Addr, ok bool) {
+	if !p.IsEchoRequest() {
+		return netip.Addr{}, false
+	}
+	if !ns.ProcessSubnets {
+		return netip.Addr{}, false
+	}
+
+	destIP := p.Dst.Addr()
+
+	// For non-4via6 addresses, we don't handle pings if they're destined
+	// for a Tailscale IP.
+	if !viaRange.Contains(destIP) {
+		if tsaddr.IsTailscaleIP(destIP) {
+			return netip.Addr{}, false
+		}
+
+		return destIP, true
+	}
+
+	// The input echo request was to a 4via6 address, which we cannot
+	// simply ping as-is from this process. Translate the destination to an
+	// IPv4 address, so that our relayed ping (in userPing) is pinging the
+	// underlying destination IP.
+	//
+	// ICMPv4 and ICMPv6 are different protocols with different on-the-wire
+	// representations, so normally you can't send an ICMPv6 message over
+	// IPv4 and expect to get a useful result. However, in this specific
+	// case things are safe because the 'userPing' function doesn't make
+	// use of the input packet.
+	return tsaddr.UnmapVia(destIP), true
+}
+
 func netaddrIPFromNetstackIP(s tcpip.Address) netip.Addr {
 	switch len(s) {
 	case 4:

wgengine/netstack/netstack_test.go

@@ -13,6 +13,7 @@ import (
 	"tailscale.com/net/packet"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/net/tstun"
+	"tailscale.com/types/ipproto"
 	"tailscale.com/wgengine"
 	"tailscale.com/wgengine/filter"
 )
@@ -84,3 +85,167 @@ func TestNetstackLeakMode(t *testing.T) {
 		t.Fatalf("refs.leakMode is 0, want a non-zero value")
 	}
 }
+
+func makeNetstack(t *testing.T, config func(*Impl)) *Impl {
+	tunDev := tstun.NewFake()
+	dialer := new(tsdial.Dialer)
+	logf := func(format string, args ...any) {
+		if !t.Failed() {
+			t.Helper()
+			t.Logf(format, args...)
+		}
+	}
+	eng, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{
+		Tun:    tunDev,
+		Dialer: dialer,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { eng.Close() })
+	ig, ok := eng.(wgengine.InternalsGetter)
+	if !ok {
+		t.Fatal("not an InternalsGetter")
+	}
+	tunWrap, magicSock, dns, ok := ig.GetInternals()
+	if !ok {
+		t.Fatal("failed to get internals")
+	}
+
+	ns, err := Create(logf, tunWrap, eng, magicSock, dialer, dns)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { ns.Close() })
+
+	ns.atomicIsLocalIPFunc.Store(func(netip.Addr) bool { return true })
+	config(ns)
+
+	if err := ns.Start(); err != nil {
+		t.Fatalf("Start: %v", err)
+	}
+	return ns
+}
+
+func TestShouldHandlePing(t *testing.T) {
+	srcIP := netip.AddrFrom4([4]byte{1, 2, 3, 4})
+
+	t.Run("ICMP4", func(t *testing.T) {
+		dst := netip.MustParseAddr("5.6.7.8")
+		icmph := packet.ICMP4Header{
+			IP4Header: packet.IP4Header{
+				IPProto: ipproto.ICMPv4,
+				Src:     srcIP,
+				Dst:     dst,
+			},
+			Type: packet.ICMP4EchoRequest,
+			Code: packet.ICMP4NoCode,
+		}
+		_, payload := packet.ICMPEchoPayload(nil)
+		icmpPing := packet.Generate(icmph, payload)
+		pkt := &packet.Parsed{}
+		pkt.Decode(icmpPing)
+
+		impl := makeNetstack(t, func(impl *Impl) {
+			impl.ProcessSubnets = true
+		})
+		pingDst, ok := impl.shouldHandlePing(pkt)
+		if !ok {
+			t.Errorf("expected shouldHandlePing==true")
+		}
+		if pingDst != dst {
+			t.Errorf("got dst %s; want %s", pingDst, dst)
+		}
+	})
+
+	t.Run("ICMP6-no-via", func(t *testing.T) {
+		dst := netip.MustParseAddr("2a09:8280:1::4169")
+		icmph := packet.ICMP6Header{
+			IP6Header: packet.IP6Header{
+				IPProto: ipproto.ICMPv6,
+				Src:     srcIP,
+				Dst:     dst,
+			},
+			Type: packet.ICMP6EchoRequest,
+			Code: packet.ICMP6NoCode,
+		}
+		_, payload := packet.ICMPEchoPayload(nil)
+		icmpPing := packet.Generate(icmph, payload)
+		pkt := &packet.Parsed{}
+		pkt.Decode(icmpPing)
+
+		impl := makeNetstack(t, func(impl *Impl) {
+			impl.ProcessSubnets = true
+		})
+		pingDst, ok := impl.shouldHandlePing(pkt)
+
+		// Expect that we handle this since it's going out onto the
+		// network.
+		if !ok {
+			t.Errorf("expected shouldHandlePing==true")
+		}
+		if pingDst != dst {
+			t.Errorf("got dst %s; want %s", pingDst, dst)
+		}
+	})
+
+	t.Run("ICMP6-tailscale-addr", func(t *testing.T) {
+		dst := netip.MustParseAddr("fd7a:115c:a1e0:ab12::1")
+		icmph := packet.ICMP6Header{
+			IP6Header: packet.IP6Header{
+				IPProto: ipproto.ICMPv6,
+				Src:     srcIP,
+				Dst:     dst,
+			},
+			Type: packet.ICMP6EchoRequest,
+			Code: packet.ICMP6NoCode,
+		}
+		_, payload := packet.ICMPEchoPayload(nil)
+		icmpPing := packet.Generate(icmph, payload)
+		pkt := &packet.Parsed{}
+		pkt.Decode(icmpPing)
+
+		impl := makeNetstack(t, func(impl *Impl) {
+			impl.ProcessSubnets = true
+		})
+		_, ok := impl.shouldHandlePing(pkt)
+
+		// We don't handle this because it's a Tailscale IP and not 4via6
+		if ok {
+			t.Errorf("expected shouldHandlePing==false")
+		}
+	})
+
+	t.Run("ICMP6-4via6", func(t *testing.T) {
+		// The 4via6 route 10.1.1.0/24 siteid 7, and then the IP
+		// 10.1.1.9 within that route.
+		dst := netip.MustParseAddr("fd7a:115c:a1e0:b1a:0:7:a01:109")
+		expectedPingDst := netip.MustParseAddr("10.1.1.9")
+		icmph := packet.ICMP6Header{
+			IP6Header: packet.IP6Header{
+				IPProto: ipproto.ICMPv6,
+				Src:     srcIP,
+				Dst:     dst,
+			},
+			Type: packet.ICMP6EchoRequest,
+			Code: packet.ICMP6NoCode,
+		}
+		_, payload := packet.ICMPEchoPayload(nil)
+		icmpPing := packet.Generate(icmph, payload)
+		pkt := &packet.Parsed{}
+		pkt.Decode(icmpPing)
+
+		impl := makeNetstack(t, func(impl *Impl) {
+			impl.ProcessSubnets = true
+		})
+		pingDst, ok := impl.shouldHandlePing(pkt)
+
+		// Handled due to being 4via6
+		if !ok {
+			t.Errorf("expected shouldHandlePing==true")
+		}
+		if pingDst != expectedPingDst {
+			t.Errorf("got dst %s; want %s", pingDst, expectedPingDst)
+		}
+	})
+}