From c1a2e2c3802568be16480e13f549b1aaab59f1f4 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 22 Dec 2022 12:51:06 -0800 Subject: [PATCH] net/{packet,tstun},wgengine/filter: fix unknown IP protocol handling 01b90df2fa4f9101e4f0ae8334b00dd9c3ccc148 added SCTP support before (with explicit parsing for ports) and 69de3bf7bfddb37b4c0e076c93115f82a51ec407 tried to add support for arbitrary IP protocols (as long as the ACL permited a port of "*", since we might not know how to find ports from an arbitrary IP protocol, if it even has such a concept). But apparently that latter commit wasn't tested end-to-end enough. It had a lot of tests, but the tests made assumptions about layering that either weren't true, or regressed since 1.20. Notably, it didn't remove the (*Filter).pre bidirectional filter that dropped all "unknown" protocol packets both leaving and entering, even if there were explicit protocol matches allowing them in. Also, don't map all unknown protocols to 0. Keep their IP protocol number parsed so it's matchable by later layers. Only reject illegal things. Fixes #6423 Updates #2162 Updates #2163 Change-Id: I9659b3ece86f4db51d644f9b34df78821758842c Signed-off-by: Brad Fitzpatrick --- net/packet/packet.go | 11 ++++++++--- net/packet/packet_test.go | 38 ++++++++++++++++++++++++++++++++++++++ net/tstun/wrap_test.go | 28 ++++++++++++++++++++++++---- wgengine/filter/filter.go | 11 +++-------- 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/net/packet/packet.go b/net/packet/packet.go index 001e469ba..5ec015ef3 100644 --- a/net/packet/packet.go +++ b/net/packet/packet.go @@ -211,9 +211,11 @@ func (q *Parsed) decode4(b []byte) { // Inter-tailscale messages. q.dataofs = q.subofs return - default: + case ipproto.Fragment: + // An IPProto value of 0xff (our Fragment constant for internal use) + // should never actually be used in the wild; if we see it, + // something's suspicious and we map it back to zero (unknown). q.IPProto = unknown - return } } else { // This is a fragment other than the first one. @@ -312,7 +314,10 @@ func (q *Parsed) decode6(b []byte) { // Inter-tailscale messages. q.dataofs = q.subofs return - default: + case ipproto.Fragment: + // An IPProto value of 0xff (our Fragment constant for internal use) + // should never actually be used in the wild; if we see it, + // something's suspicious and we map it back to zero (unknown). q.IPProto = unknown return } diff --git a/net/packet/packet_test.go b/net/packet/packet_test.go index 9931e8ceb..fadbf97ab 100644 --- a/net/packet/packet_test.go +++ b/net/packet/packet_test.go @@ -6,12 +6,16 @@ package packet import ( "bytes" + "encoding/hex" "net/netip" "reflect" + "strings" "testing" + "unicode" "tailscale.com/tstest" "tailscale.com/types/ipproto" + "tailscale.com/util/must" ) const ( @@ -440,6 +444,17 @@ func TestParsedString(t *testing.T) { } } +// mustHexDecode is like hex.DecodeString, but panics on error +// and ignores whitespcae in s. +func mustHexDecode(s string) []byte { + return must.Get(hex.DecodeString(strings.Map(func(r rune) rune { + if unicode.IsSpace(r) { + return -1 + } + return r + }, s))) +} + func TestDecode(t *testing.T) { tests := []struct { name string @@ -459,6 +474,29 @@ func TestDecode(t *testing.T) { {"ipv4_sctp", sctpBuffer, sctpDecode}, {"ipv4_frag", tcp4MediumFragmentBuffer, tcp4MediumFragmentDecode}, {"ipv4_fragtooshort", tcp4ShortFragmentBuffer, tcp4ShortFragmentDecode}, + + {"ip97", mustHexDecode("4500 0019 d186 4000 4061 751d 644a 4603 6449 e549 6865 6c6c 6f"), Parsed{ + IPVersion: 4, + IPProto: 97, + Src: netip.MustParseAddrPort("100.74.70.3:0"), + Dst: netip.MustParseAddrPort("100.73.229.73:0"), + b: mustHexDecode("4500 0019 d186 4000 4061 751d 644a 4603 6449 e549 6865 6c6c 6f"), + length: 25, + subofs: 20, + }}, + + // This packet purports to use protocol 0xFF, which is verboten and + // used internally as a sentinel value for fragments. So test that + // we map packets using 0xFF to Unknown (0) instead. + {"bogus_proto_ff", mustHexDecode("4500 0019 d186 4000 40" + "FF" /* bogus FF */ + " 751d 644a 4603 6449 e549 6865 6c6c 6f"), Parsed{ + IPVersion: 4, + IPProto: ipproto.Unknown, // 0, not bogus 0xFF + Src: netip.MustParseAddrPort("100.74.70.3:0"), + Dst: netip.MustParseAddrPort("100.73.229.73:0"), + b: mustHexDecode("4500 0019 d186 4000 40" + "FF" /* bogus FF */ + " 751d 644a 4603 6449 e549 6865 6c6c 6f"), + length: 25, + subofs: 20, + }}, } for _, tt := range tests { diff --git a/net/tstun/wrap_test.go b/net/tstun/wrap_test.go index 1c5b005ce..41b8be32b 100644 --- a/net/tstun/wrap_test.go +++ b/net/tstun/wrap_test.go @@ -8,11 +8,13 @@ import ( "bytes" "context" "encoding/binary" + "encoding/hex" "fmt" "net/netip" "strconv" "strings" "testing" + "unicode" "unsafe" "github.com/google/go-cmp/cmp" @@ -30,6 +32,7 @@ import ( "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/netlogtype" + "tailscale.com/util/must" "tailscale.com/wgengine/filter" ) @@ -293,6 +296,17 @@ func TestWriteAndInject(t *testing.T) { } } +// mustHexDecode is like hex.DecodeString, but panics on error +// and ignores whitespcae in s. +func mustHexDecode(s string) []byte { + return must.Get(hex.DecodeString(strings.Map(func(r rune) rune { + if unicode.IsSpace(r) { + return -1 + } + return r + }, s))) +} + func TestFilter(t *testing.T) { chtun, tun := newChannelTUN(t.Logf, true) defer tun.Close() @@ -310,8 +324,9 @@ func TestFilter(t *testing.T) { drop bool data []byte }{ - {"junk_in", in, true, []byte("\x45not a valid IPv4 packet")}, - {"junk_out", out, true, []byte("\x45not a valid IPv4 packet")}, + {"short_in", in, true, []byte("\x45xxx")}, + {"short_out", out, true, []byte("\x45xxx")}, + {"ip97_out", out, false, mustHexDecode("4500 0019 d186 4000 4061 751d 644a 4603 6449 e549 6865 6c6c 6f")}, {"bad_port_in", in, true, udp4("5.6.7.8", "1.2.3.4", 22, 22)}, {"bad_port_out", out, false, udp4("1.2.3.4", "5.6.7.8", 22, 22)}, {"bad_ip_in", in, true, udp4("8.1.1.1", "1.2.3.4", 89, 89)}, @@ -386,9 +401,11 @@ func TestFilter(t *testing.T) { got, _ := stats.TestExtract() want := map[netlogtype.Connection]netlogtype.Counts{} + var wasUDP bool if !tt.drop { var p packet.Parsed p.Decode(tt.data) + wasUDP = p.IPProto == ipproto.UDP switch tt.dir { case in: conn := netlogtype.Connection{Proto: ipproto.UDP, Src: p.Dst, Dst: p.Src} @@ -398,8 +415,10 @@ func TestFilter(t *testing.T) { want[conn] = netlogtype.Counts{TxPackets: 1, TxBytes: uint64(len(tt.data))} } } - if diff := cmp.Diff(got, want, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("stats.TestExtract (-got +want):\n%s", diff) + if wasUDP { + if diff := cmp.Diff(got, want, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("stats.TestExtract (-got +want):\n%s", diff) + } } }) } @@ -525,6 +544,7 @@ func TestPeerAPIBypass(t *testing.T) { p.Decode(tt.pkt) tt.w.SetFilter(tt.filter) tt.w.disableTSMPRejected = true + tt.w.logf = t.Logf if got := tt.w.filterIn(p); got != tt.want { t.Errorf("got = %v; want %v", got, tt.want) } diff --git a/wgengine/filter/filter.go b/wgengine/filter/filter.go index f9520b8b2..f573a2d07 100644 --- a/wgengine/filter/filter.go +++ b/wgengine/filter/filter.go @@ -452,7 +452,7 @@ func (f *Filter) runIn4(q *packet.Parsed) (r Response, why string) { return Accept, "tsmp ok" default: if f.matches4.matchProtoAndIPsOnlyIfAllPorts(q) { - return Accept, "otherproto ok" + return Accept, "other-portless ok" } return Drop, unknownProtoString(q.IPProto) } @@ -512,7 +512,7 @@ func (f *Filter) runIn6(q *packet.Parsed) (r Response, why string) { return Accept, "tsmp ok" default: if f.matches6.matchProtoAndIPsOnlyIfAllPorts(q) { - return Accept, "otherproto ok" + return Accept, "other-portless ok" } return Drop, unknownProtoString(q.IPProto) } @@ -577,12 +577,7 @@ func (f *Filter) pre(q *packet.Parsed, rf RunFlags, dir direction) Response { return Drop } - switch q.IPProto { - case ipproto.Unknown: - // Unknown packets are dangerous; always drop them. - f.logRateLimit(rf, q, dir, Drop, "unknown") - return Drop - case ipproto.Fragment: + if q.IPProto == ipproto.Fragment { // Fragments after the first always need to be passed through. // Very small fragments are considered Junk by Parsed. f.logRateLimit(rf, q, dir, Accept, "fragment")