Fix #437

netstack2/handlers.go

@@ -496,42 +496,28 @@ func (h *ICMPHandler) handleICMPPacket(id stack.TransportEndpointID, pkt *stack.
 	logger.Info("ICMP Handler: Echo Request from %s to %s (ident=%d, seq=%d)",
 		srcIP, dstIP, icmpHdr.Ident(), icmpHdr.Sequence())
 
-	// Convert to netip.Addr for subnet matching
-	srcAddr, err := netip.ParseAddr(srcIP)
-	if err != nil {
-		logger.Debug("ICMP Handler: Failed to parse source IP %s: %v", srcIP, err)
-		return false
-	}
-	dstAddr, err := netip.ParseAddr(dstIP)
-	if err != nil {
-		logger.Debug("ICMP Handler: Failed to parse dest IP %s: %v", dstIP, err)
-		return false
-	}
-
-	// Check subnet rules (use port 0 for ICMP since it doesn't have ports)
 	if h.proxyHandler == nil {
 		logger.Debug("ICMP Handler: No proxy handler configured")
 		return false
 	}
 
-	matchedRule := h.proxyHandler.subnetLookup.Match(srcAddr, dstAddr, 0, header.ICMPv4ProtocolNumber)
-	if matchedRule == nil {
-		logger.Debug("ICMP Handler: No matching subnet rule for %s -> %s", srcIP, dstIP)
-		return false
-	}
-
-	logger.Info("ICMP Handler: Matched subnet rule for %s -> %s", srcIP, dstIP)
-
-	// Determine actual destination (with possible rewrite)
+	// This packet only reached the proxy stack because it already matched a
+	// subnet rule during injection (ProxyHandler.HandleIncomingPacket), so
+	// there's no need to re-run subnet matching here for permission - doing
+	// so used to re-derive the DNAT target from a *fresh* rule lookup keyed
+	// on dstIP, but dstIP here is ambiguous: for a loopback rewrite target
+	// it's still the original (unrewritten) destination, while for a
+	// non-loopback rewrite target it's already the post-DNAT address. In
+	// the latter case (and always for a domain-name RewriteTo, which has no
+	// subnet rule of its own for the resolved IP) that re-lookup could find
+	// no rule and silently drop the ping even though the connection is
+	// legitimately allowed. Instead, resolve the same way the TCP/UDP
+	// handlers do: via destRewriteTable, which HandleIncomingPacket already
+	// populated for this exact flow keyed by the original destination.
 	actualDstIP := dstIP
-	if matchedRule.RewriteTo != "" {
-		resolvedAddr, err := h.proxyHandler.resolveRewriteAddress(matchedRule.RewriteTo)
-		if err != nil {
-			logger.Info("ICMP Handler: Failed to resolve rewrite address %s: %v", matchedRule.RewriteTo, err)
-		} else {
-			actualDstIP = resolvedAddr.String()
-			logger.Info("ICMP Handler: Using rewritten destination %s (original: %s)", actualDstIP, dstIP)
-		}
+	if rewrittenAddr, ok := h.proxyHandler.LookupDestinationRewrite(srcIP, dstIP, 0, uint8(header.ICMPv4ProtocolNumber)); ok {
+		actualDstIP = rewrittenAddr.String()
+		logger.Info("ICMP Handler: Using rewritten destination %s (original: %s)", actualDstIP, dstIP)
 	}
 
 	// Get the full ICMP payload (including the data after the header)

netstack2/icmp_dnat_test.go

@@ -0,0 +1,152 @@
+package netstack2
+
+import (
+	"bytes"
+	"io"
+	"net/netip"
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/fosrl/newt/logger"
+	"gvisor.dev/gvisor/pkg/tcpip"
+	"gvisor.dev/gvisor/pkg/tcpip/checksum"
+	"gvisor.dev/gvisor/pkg/tcpip/header"
+)
+
+// buildICMPEchoRequest builds a minimal, checksummed IPv4 ICMP echo request
+// packet from src to dst.
+func buildICMPEchoRequest(t *testing.T, src, dst netip.Addr) []byte {
+	t.Helper()
+
+	const icmpSize = header.ICMPv4MinimumSize
+	totalLen := header.IPv4MinimumSize + icmpSize
+	pkt := make([]byte, totalLen)
+
+	ip := header.IPv4(pkt)
+	ip.Encode(&header.IPv4Fields{
+		TotalLength: uint16(totalLen),
+		TTL:         64,
+		Protocol:    uint8(header.ICMPv4ProtocolNumber),
+		SrcAddr:     tcpip.AddrFrom4(src.As4()),
+		DstAddr:     tcpip.AddrFrom4(dst.As4()),
+	})
+	ip.SetChecksum(0)
+	ip.SetChecksum(^ip.CalculateChecksum())
+
+	icmp := header.ICMPv4(pkt[header.IPv4MinimumSize:])
+	icmp.SetType(header.ICMPv4Echo)
+	icmp.SetCode(0)
+	icmp.SetIdent(1)
+	icmp.SetSequence(1)
+	icmp.SetChecksum(0)
+	icmp.SetChecksum(header.ICMPv4Checksum(icmp, checksum.Checksum(icmp.Payload(), 0)))
+
+	return pkt
+}
+
+// noopNotification is a no-op channel.Notification for tests that don't
+// care about read-availability notifications.
+type noopNotification struct{}
+
+func (noopNotification) WriteNotify() {}
+
+// captureLogOutput redirects the package logger to a pipe for the duration
+// of fn, and returns everything written to it. Needed here because
+// ICMPHandler.handleICMPPacket runs on its own goroutine off of
+// HandleIncomingPacket and reports its outcome only via log lines.
+func captureLogOutput(t *testing.T, fn func()) string {
+	t.Helper()
+
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("os.Pipe: %v", err)
+	}
+
+	logger.SetOutput(w)
+	defer logger.SetOutput(os.Stdout)
+
+	done := make(chan string, 1)
+	go func() {
+		var buf bytes.Buffer
+		io.Copy(&buf, r)
+		done <- buf.String()
+	}()
+
+	fn()
+
+	// handleICMPPacket runs asynchronously (go h.proxyPing(...)); give it a
+	// moment to log its outcome before we stop capturing.
+	time.Sleep(200 * time.Millisecond)
+
+	w.Close()
+	return <-done
+}
+
+// A DNAT target whose RewriteTo isn't independently reachable as its own
+// destination (e.g. a resolved domain name, or - as here - just an IP with
+// no mirrored direct subnet rule) used to make ICMP echo requests get
+// silently dropped: the old ICMPHandler re-derived the DNAT target by
+// running a fresh SubnetLookup.Match() against whatever address the packet
+// carried by the time it reached the handler, which for a non-loopback
+// rewrite is already the post-DNAT address - and no rule exists for that
+// address on its own. The fix resolves the real target via
+// destRewriteTable (LookupDestinationRewrite) instead, exactly like the
+// TCP/UDP handlers already did, so this no longer depends on a mirrored
+// direct rule existing for the rewritten address.
+func TestICMPHandleIncomingPacket_DNATWithoutMirroredDirectRule(t *testing.T) {
+	ph, err := NewProxyHandler(ProxyHandlerOptions{EnableICMP: true, MTU: 1500})
+	if err != nil {
+		t.Fatalf("NewProxyHandler: %v", err)
+	}
+	if err := ph.Initialize(noopNotification{}); err != nil {
+		t.Fatalf("Initialize: %v", err)
+	}
+	defer ph.Close()
+
+	srcAddr := netip.MustParseAddr("10.0.0.5")
+	aliasAddr := netip.MustParseAddr("10.20.20.9")
+	realAddr := netip.MustParseAddr("203.0.113.50")
+
+	ph.AddSubnetRule(SubnetRule{
+		SourcePrefix: netip.MustParsePrefix("10.0.0.0/24"),
+		DestPrefix:   netip.PrefixFrom(aliasAddr, 32),
+		RewriteTo:    realAddr.String() + "/32",
+	})
+
+	pkt := buildICMPEchoRequest(t, srcAddr, aliasAddr)
+
+	var injected bool
+	logs := captureLogOutput(t, func() {
+		injected = ph.HandleIncomingPacket(pkt)
+	})
+
+	if !injected {
+		t.Fatal("expected ICMP echo request to be matched and injected")
+	}
+
+	// destRewriteTable is populated by HandleIncomingPacket itself, so this
+	// much holds regardless of the bug - included here to pin the mechanism
+	// the fix relies on.
+	got, ok := ph.LookupDestinationRewrite(srcAddr.String(), aliasAddr.String(), 0, uint8(header.ICMPv4ProtocolNumber))
+	if !ok {
+		t.Fatal("expected destRewriteTable to have an entry for this ICMP flow")
+	}
+	if got != realAddr {
+		t.Fatalf("expected rewritten destination %s, got %s", realAddr, got)
+	}
+
+	// This is the actual regression check: with the bug, handleICMPPacket
+	// re-matches on the already-rewritten address, finds no rule for it,
+	// and drops the echo request before ever attempting to proxy it.
+	if strings.Contains(logs, "No matching subnet rule") {
+		t.Errorf("ICMP handler dropped the echo request instead of proxying it; logs:\n%s", logs)
+	}
+	if !strings.Contains(logs, "Proxying ping from") {
+		t.Errorf("expected ICMP handler to proxy the ping to the rewritten destination; logs:\n%s", logs)
+	}
+	if !strings.Contains(logs, realAddr.String()) {
+		t.Errorf("expected logs to reference the rewritten destination %s; logs:\n%s", realAddr, logs)
+	}
+}