From d96d26c22aaec65120c44d9dd6af89923bab1ecb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 28 Jul 2020 22:10:58 -0700 Subject: [PATCH] wgengine/filter: don't spam logs on dropped outgoing IPv6 ICMP or IPv4 IGMP The OS (tries) to send these but we drop them. No need to worry the user with spam that we're dropping it. Fixes #402 Signed-off-by: Brad Fitzpatrick --- wgengine/filter/filter.go | 75 ++++++++++++++++++++++++++++------ wgengine/filter/filter_test.go | 2 +- wgengine/packet/ip.go | 1 + 3 files changed, 65 insertions(+), 13 deletions(-) diff --git a/wgengine/filter/filter.go b/wgengine/filter/filter.go index 970728bd7..60c48d198 100644 --- a/wgengine/filter/filter.go +++ b/wgengine/filter/filter.go @@ -136,9 +136,13 @@ func maybeHexdump(flag RunFlags, b []byte) string { var acceptBucket = rate.NewLimiter(rate.Every(10*time.Second), 3) var dropBucket = rate.NewLimiter(rate.Every(5*time.Second), 10) -func (f *Filter) logRateLimit(runflags RunFlags, q *packet.ParsedPacket, r Response, why string) { +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) { + return + } + if r == Drop && (runflags&LogDrops) != 0 && dropBucket.Allow() { verdict = "Drop" runflags &= HexdumpDrops @@ -157,26 +161,28 @@ func (f *Filter) logRateLimit(runflags RunFlags, q *packet.ParsedPacket, r Respo // RunIn determines whether this node is allowed to receive q from a Tailscale peer. func (f *Filter) RunIn(q *packet.ParsedPacket, rf RunFlags) Response { - r := f.pre(q, rf) + dir := in + r := f.pre(q, rf, dir) if r == Accept || r == Drop { // already logged return r } r, why := f.runIn(q) - f.logRateLimit(rf, q, r, why) + f.logRateLimit(rf, q, dir, r, why) return r } // RunOut determines whether this node is allowed to send q to a Tailscale peer. func (f *Filter) RunOut(q *packet.ParsedPacket, rf RunFlags) Response { - r := f.pre(q, rf) + dir := out + r := f.pre(q, rf, dir) if r == Drop || r == Accept { // already logged return r } r, why := f.runOut(q) - f.logRateLimit(rf, q, r, why) + f.logRateLimit(rf, q, dir, r, why) return r } @@ -252,33 +258,78 @@ func (f *Filter) runOut(q *packet.ParsedPacket) (r Response, why string) { return Accept, "ok out" } -func (f *Filter) pre(q *packet.ParsedPacket, rf RunFlags) Response { +// direction is whether a packet was flowing in to this machine, or flowing out. +type direction int + +const ( + in direction = iota + out +) + +func (f *Filter) pre(q *packet.ParsedPacket, rf RunFlags, dir direction) Response { if len(q.Buffer()) == 0 { // wireguard keepalive packet, always permit. return Accept } if len(q.Buffer()) < 20 { - f.logRateLimit(rf, q, Drop, "too short") + f.logRateLimit(rf, q, dir, Drop, "too short") return Drop } if q.IPVersion == 6 { - // TODO(bradfitz): don't log about normal broadcast - // IPv6 traffic like route announcements. - f.logRateLimit(rf, q, Drop, "ipv6") + f.logRateLimit(rf, q, dir, Drop, "ipv6") return Drop } switch q.IPProto { case packet.Unknown: // Unknown packets are dangerous; always drop them. - f.logRateLimit(rf, q, Drop, "unknown") + f.logRateLimit(rf, q, dir, Drop, "unknown") return Drop case packet.Fragment: // Fragments after the first always need to be passed through. // Very small fragments are considered Junk by ParsedPacket. - f.logRateLimit(rf, q, Accept, "fragment") + f.logRateLimit(rf, q, dir, Accept, "fragment") return Accept } 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" + +// 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 { + switch dir { + case out: + switch p.IPVersion { + case 4: + // Omit logging about outgoing IGMP queries being dropped. + if p.IPProto == packet.IGMP { + return true + } + case 6: + b := p.Buffer() + if len(b) < 40 { + return false + } + src, dst := b[8:8+16], b[24:24+16] + // Omit logging for outgoing IPv6 ICMP-v6 queries to ff02::2, + // as sent by the OS, looking for routers. + if p.IPProto == packet.ICMPv6 { + if isLinkLocalV6(src) && string(dst) == ipv6AllRoutersLinkLocal { + return true + } + } + } + } + return false +} + +// isLinkLocalV6 reports whether src is in fe80::/10. +func isLinkLocalV6(src []byte) bool { + return len(src) == 16 && src[0] == 0xfe && src[1]>>6 == 0x80>>6 +} diff --git a/wgengine/filter/filter_test.go b/wgengine/filter/filter_test.go index 783b258f0..16cd5f676 100644 --- a/wgengine/filter/filter_test.go +++ b/wgengine/filter/filter_test.go @@ -219,7 +219,7 @@ func TestPreFilter(t *testing.T) { for _, testPacket := range packets { p := &ParsedPacket{} p.Decode(testPacket.b) - got := f.pre(p, LogDrops|LogAccepts) + got := f.pre(p, LogDrops|LogAccepts, in) if got != testPacket.want { t.Errorf("%q got=%v want=%v packet:\n%s", testPacket.desc, got, testPacket.want, packet.Hexdump(testPacket.b)) } diff --git a/wgengine/packet/ip.go b/wgengine/packet/ip.go index 39dfb66c7..5620f4531 100644 --- a/wgengine/packet/ip.go +++ b/wgengine/packet/ip.go @@ -47,6 +47,7 @@ const ( // Unknown represents an unknown or unsupported protocol; it's deliberately the zero value. Unknown IPProto = 0x00 ICMP IPProto = 0x01 + IGMP IPProto = 0x02 ICMPv6 IPProto = 0x3a TCP IPProto = 0x06 UDP IPProto = 0x11