From b4d97d2532b927b38d1ba4fa7bdb9d8c498a4c5e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 30 Jul 2020 10:57:30 -0700 Subject: [PATCH] wgengine/filter: fix IPv4 IGMP spam omission, also omit ff02::16 spam And add tests. Fixes #618 Updates #402 --- wgengine/filter/filter.go | 44 +++++++++++++++++++++----- wgengine/filter/filter_test.go | 56 ++++++++++++++++++++++++++++++++++ wgengine/packet/packet.go | 2 +- wgengine/packet/packet_test.go | 2 +- 4 files changed, 95 insertions(+), 9 deletions(-) diff --git a/wgengine/filter/filter.go b/wgengine/filter/filter.go index 60c48d198..43377d564 100644 --- a/wgengine/filter/filter.go +++ b/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 diff --git a/wgengine/filter/filter_test.go b/wgengine/filter/filter_test.go index 16cd5f676..5f8c64f67 100644 --- a/wgengine/filter/filter_test.go +++ b/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())) + } + }) + } +} diff --git a/wgengine/packet/packet.go b/wgengine/packet/packet.go index 75bf0bad1..a82e84ab1 100644 --- a/wgengine/packet/packet.go +++ b/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: diff --git a/wgengine/packet/packet_test.go b/wgengine/packet/packet_test.go index 7669a2e90..8b286da51 100644 --- a/wgengine/packet/packet_test.go +++ b/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 {