wgengine/filter: fix IPv4 IGMP spam omission, also omit ff02::16 spam

And add tests.

Fixes #618
Updates #402

wgengine/filter/filter.go

@@ -6,6 +6,9 @@
 package filter
 
 import (
+	"fmt"
+	"os"
+	"strconv"
 	"sync"
 	"time"
 
@@ -139,7 +142,7 @@ var dropBucket = rate.NewLimiter(rate.Every(5*time.Second), 10)
 func (f *Filter) logRateLimit(runflags RunFlags, q *packet.ParsedPacket, dir direction, r Response, why string) {
 	var verdict string
 
-	if r == Drop && f.omitDropLogging(q, dir) {
+	if r == Drop && omitDropLogging(q, dir) {
 		return
 	}
 
@@ -266,6 +269,17 @@ const (
 	out
 )
 
+func (d direction) String() string {
+	switch d {
+	case in:
+		return "in"
+	case out:
+		return "out"
+	default:
+		return fmt.Sprintf("[??dir=%d]", int(d))
+	}
+}
+
 func (f *Filter) pre(q *packet.ParsedPacket, rf RunFlags, dir direction) Response {
 	if len(q.Buffer()) == 0 {
 		// wireguard keepalive packet, always permit.
@@ -295,24 +309,36 @@ func (f *Filter) pre(q *packet.ParsedPacket, rf RunFlags, dir direction) Respons
 	return noVerdict
 }
 
-// ipv6AllRoutersLinkLocal is ff02::2 (All link-local routers).
-const ipv6AllRoutersLinkLocal = "\xff\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02"
+const (
+	// ipv6AllRoutersLinkLocal is ff02::2 (All link-local routers)
+	ipv6AllRoutersLinkLocal = "\xff\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02"
+	// ipv6AllMLDv2CapableRouters is ff02::16 (All MLDv2-capable routers)
+	ipv6AllMLDv2CapableRouters = "\xff\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x16"
+)
+
+var debugLogDroppedPackets, _ = strconv.ParseBool(os.Getenv("TS_DEBUG_LOG_DROPPED_PACKETS"))
 
 // omitDropLogging reports whether packet p, which has already been
 // deemded a packet to Drop, should bypass the [rate-limited] logging.
 // We don't want to log scary & spammy reject warnings for packets that
 // are totally normal, like IPv6 route announcements.
-func (f *Filter) omitDropLogging(p *packet.ParsedPacket, dir direction) bool {
+func omitDropLogging(p *packet.ParsedPacket, dir direction) bool {
+	b := p.Buffer()
 	switch dir {
 	case out:
 		switch p.IPVersion {
 		case 4:
-			// Omit logging about outgoing IGMP queries being dropped.
-			if p.IPProto == packet.IGMP {
+			// ParsedPacket.Decode zeros out ParsedPacket.IPProtocol for protocols
+			// it doesn't know about, so parse it out ourselves if needed.
+			ipProto := p.IPProto
+			if ipProto == 0 && len(b) > 8 {
+				ipProto = packet.IPProto(b[9])
+			}
+			// Omit logging about outgoing IGMP.
+			if ipProto == packet.IGMP {
 				return true
 			}
 		case 6:
-			b := p.Buffer()
 			if len(b) < 40 {
 				return false
 			}
@@ -324,6 +350,10 @@ func (f *Filter) omitDropLogging(p *packet.ParsedPacket, dir direction) bool {
 					return true
 				}
 			}
+			if string(dst) == ipv6AllMLDv2CapableRouters {
+				return true
+			}
+			panic(fmt.Sprintf("Got proto=%2x; src=%x dst=%x", int(p.IPProto), src, dst))
 		}
 	}
 	return false

wgengine/filter/filter_test.go

@@ -6,7 +6,9 @@ package filter
 
 import (
 	"encoding/binary"
+	"encoding/hex"
 	"encoding/json"
+	"strings"
 	"testing"
 
 	"tailscale.com/types/logger"
@@ -298,3 +300,57 @@ func rawdefault(proto packet.IPProto, trimLength int) []byte {
 	port := uint16(53)
 	return rawpacket(proto, ip, ip, port, port, trimLength)
 }
+
+func parseHexPkt(t *testing.T, h string) *packet.ParsedPacket {
+	t.Helper()
+	b, err := hex.DecodeString(strings.ReplaceAll(h, " ", ""))
+	if err != nil {
+		t.Fatalf("failed to read hex %q: %v", h, err)
+	}
+	p := new(packet.ParsedPacket)
+	p.Decode(b)
+	return p
+}
+
+func TestOmitDropLogging(t *testing.T) {
+	tests := []struct {
+		name string
+		pkt  *packet.ParsedPacket
+		dir  direction
+		want bool
+	}{
+		{
+			name: "v4_tcp_out",
+			pkt:  &packet.ParsedPacket{IPVersion: 4, IPProto: packet.TCP},
+			dir:  out,
+			want: false,
+		},
+		{
+			name: "v6_icmp_out", // as seen on Linux
+			pkt:  parseHexPkt(t, "60 00 00 00 00 00 3a 00   fe800000000000000000000000000000 ff020000000000000000000000000002"),
+			dir:  out,
+			want: true,
+		},
+		{
+			name: "v6_to_MLDv2_capable_routers", // as seen on Windows
+			pkt:  parseHexPkt(t, "60 00 00 00 00 24 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff 02 00 00 00 00 00 00 00 00 00 00 00 00 00 16 3a 00 05 02 00 00 01 00 8f 00 6e 80 00 00 00 01 04 00 00 00 ff 02 00 00 00 00 00 00 00 00 00 00 00 00 00 0c"),
+			dir:  out,
+			want: true,
+		},
+		{
+			name: "v4_igmp_out", // on Windows, from https://github.com/tailscale/tailscale/issues/618
+			pkt:  parseHexPkt(t, "46 00 00 30 37 3a 00 00 01 02 10 0e a9 fe 53 6b e0 00 00 16 94 04 00 00 22 00 14 05 00 00 00 02 04 00 00 00 e0 00 00 fb 04 00 00 00 e0 00 00 fc"),
+			dir:  out,
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := omitDropLogging(tt.pkt, tt.dir)
+			if got != tt.want {
+				t.Errorf("got %v; want %v\npacket: %#v\n%s", got, tt.want, tt.pkt, packet.Hexdump(tt.pkt.Buffer()))
+			}
+		})
+	}
+}

wgengine/packet/packet.go

@@ -57,7 +57,7 @@ type NextHeader uint8
 
 func (p *ParsedPacket) String() string {
 	if p.IPVersion == 6 {
-		return "IPv6{???}"
+		return fmt.Sprintf("IPv6{Proto=%d}", p.IPProto)
 	}
 	switch p.IPProto {
 	case Unknown:

wgengine/packet/packet_test.go

@@ -200,7 +200,7 @@ func TestParsedPacket(t *testing.T) {
 		{"tcp", tcpPacketDecode, "TCP{1.2.3.4:123 > 5.6.7.8:567}"},
 		{"icmp", icmpRequestDecode, "ICMP{1.2.3.4:0 > 5.6.7.8:0}"},
 		{"unknown", unknownPacketDecode, "Unknown{???}"},
-		{"ipv6", ipv6PacketDecode, "IPv6{???}"},
+		{"ipv6", ipv6PacketDecode, "IPv6{Proto=58}"},
 	}
 
 	for _, tt := range tests {