TheArchive/tailscale
1
0
Fork 0
mirror of https://github.com/tailscale/tailscale.git synced 2024-11-29 04:55:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

net/{packet,tstun},wgengine/filter: fix unknown IP protocol handling

Browse Source 
01b90df2fa added SCTP support before
(with explicit parsing for ports) and
69de3bf7bf 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 <bradfitz@tailscale.com>
This commit is contained in:
Brad Fitzpatrick 2022-12-22 12:51:06 -08:00 committed by Brad Fitzpatrick
parent 3386a59cf1
commit c1a2e2c380
4 changed files with 73 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
net/packet/packet.go
View File

@ -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
}

38
net/packet/packet_test.go
View File

@ -6,12 +6,16 @@
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 {

28
net/tstun/wrap_test.go
View File

@ -8,11 +8,13 @@
"bytes"
"context"
"encoding/binary"
"encoding/hex"
"fmt"
"net/netip"
"strconv"
"strings"
"testing"
"unicode"
"unsafe"
"github.com/google/go-cmp/cmp"
@ -30,6 +32,7 @@
"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)
}

11
wgengine/filter/filter.go
View File

@ -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")