diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index 7a8475843..fdbc05145 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -716,33 +716,49 @@ func (ns *Impl) shouldHandlePing(p *packet.Parsed) (_ netip.Addr, ok bool) { if !p.IsEchoRequest() { return netip.Addr{}, false } + + destIP := p.Dst.Addr() + + // We need to handle pings for all 4via6 addresses, even if this + // netstack instance normally isn't responsible for processing subnets. + // + // For example, on Linux, subnet router traffic could be handled via + // tun+iptables rules for most packets, but we still need to handle + // ICMP echo requests over 4via6 since the host networking stack + // doesn't know what to do with a 4via6 address. + // + // shouldProcessInbound returns 'true' to say that we should process + // all IPv6 packets with a destination address in the 'via' range, so + // check before we check the "ProcessSubnets" boolean below. + if viaRange.Contains(destIP) { + // 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 + } + + // If we get here, we don't do anything unless this netstack instance + // is responsible for processing subnet traffic. 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 + if tsaddr.IsTailscaleIP(destIP) { + return netip.Addr{}, false } - // 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 + // This netstack instance is processing subnet traffic, so handle the + // ping ourselves. + return destIP, true } func netaddrIPFromNetstackIP(s tcpip.Address) netip.Addr { diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index 1d21f378b..6ad92ceec 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -5,6 +5,7 @@ package netstack import ( + "fmt" "net/netip" "runtime" "testing" @@ -216,36 +217,38 @@ func TestShouldHandlePing(t *testing.T) { } }) - 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 + // Handle pings for 4via6 addresses regardless of ProcessSubnets + for _, subnets := range []bool{true, false} { + t.Run("ICMP6-4via6-ProcessSubnets-"+fmt.Sprint(subnets), 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 = subnets + }) + pingDst, ok := impl.shouldHandlePing(pkt) + + // Handled due to being 4via6 + if !ok { + t.Errorf("expected shouldHandlePing==true") + } else if pingDst != expectedPingDst { + t.Errorf("got dst %s; want %s", pingDst, expectedPingDst) + } }) - 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) - } - }) + } }