From 0607832397046fd0acb73daf8e00ef17b171a5c6 Mon Sep 17 00:00:00 2001
From: Andrew Dunham <andrew@tailscale.com>
Date: Wed, 21 Sep 2022 18:07:57 -0400
Subject: [PATCH] wgengine/netstack: always respond to 4via6 echo requests
(#5712)
As the comment in the code says, netstack should always respond to ICMP
echo requests to a 4via6 address, even if the netstack instance isn't
normally processing subnet traffic.
Follow-up to #5709
Change-Id: I504d0776c5824071b2a2e0e687bc33e24f6c4746
Signed-off-by: Andrew Dunham <andrew@tailscale.com>
---
wgengine/netstack/netstack.go | 54 ++++++++++++++++---------
wgengine/netstack/netstack_test.go | 63 ++++++++++++++++--------------
2 files changed, 68 insertions(+), 49 deletions(-)
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)
+ // 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 = true
+ 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)
- }
- })
+ }
}