From 189d86cce586a6be5aa48af02f604808e01f534f Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 22 Jul 2020 18:08:08 +0000 Subject: [PATCH] wgengine/router: don't use 88 or 8888 as table/rule numbers. We originally picked those numbers somewhat at random, but with the idea that 8 is a traditionally lucky number in Chinese culture. Unfortunately, "88" is also neo-nazi shorthand language. Use 52 instead, because those are the digits above the letters "TS" (tailscale) on a qwerty keyboard, so we're unlikely to collide with other users. 5, 2 and 52 are also pleasantly culturally meaningless. Signed-off-by: David Anderson --- wgengine/router/router_linux.go | 55 ++++++++++++++++++---------- wgengine/router/router_linux_test.go | 40 ++++++++++---------- 2 files changed, 55 insertions(+), 40 deletions(-) diff --git a/wgengine/router/router_linux.go b/wgengine/router/router_linux.go index f440faffe..44ef9b3d3 100644 --- a/wgengine/router/router_linux.go +++ b/wgengine/router/router_linux.go @@ -46,6 +46,22 @@ const ( tailscaleBypassMark = "0x80000" ) +// tailscaleRouteTable is the routing table number for Tailscale +// network routes. See addIPRules for the detailed policy routing +// logic that ends up doing lookups within that table. +// +// NOTE(danderson): We chose 52 because those are the digits above the +// letters "TS" on a qwerty keyboard, and 52 is sufficiently unlikely +// to be picked by other software. +// +// NOTE(danderson): You might wonder why we didn't pick some high +// table number like 5252, to further avoid the potential for +// collisions with other software. Unfortunately, Busybox's `ip` +// implementation believes that table numbers are 8-bit integers, so +// for maximum compatibility we have to stay in the 0-255 range even +// though linux itself supports larger numbers. +const tailscaleRouteTable = "52" + // netfilterRunner abstracts helpers to run netfilter commands. It // exists purely to swap out go-iptables for a fake implementation in // tests. @@ -378,7 +394,7 @@ func (r *linuxRouter) addRoute(cidr netaddr.IPPrefix) error { "dev", r.tunname, } if r.ipRuleAvailable { - args = append(args, "table", "88") + args = append(args, "table", tailscaleRouteTable) } return r.cmd.run(args...) } @@ -393,7 +409,7 @@ func (r *linuxRouter) delRoute(cidr netaddr.IPPrefix) error { "dev", r.tunname, } if r.ipRuleAvailable { - args = append(args, "table", "88") + args = append(args, "table", tailscaleRouteTable) } return r.cmd.run(args...) } @@ -431,7 +447,7 @@ func (r *linuxRouter) addIPRules() error { // NOTE(apenwarr): This sequence seems complicated, right? // If we could simply have a rule that said "match packets that // *don't* have this fwmark", then we would only need to add one - // link to table 88 and we'd be done. Unfortunately, older kernels + // link to table 52 and we'd be done. Unfortunately, older kernels // and 'ip rule' implementations (including busybox), don't support // checking for the lack of a fwmark, only the presence. The technique // below works even on very old kernels. @@ -440,7 +456,7 @@ func (r *linuxRouter) addIPRules() error { // main routing table. rg.Run( "ip", "rule", "add", - "pref", "8810", + "pref", tailscaleRouteTable+"10", "fwmark", tailscaleBypassMark, "table", "main", ) @@ -448,7 +464,7 @@ func (r *linuxRouter) addIPRules() error { // even though it's been empty on every Linux system I've ever seen. rg.Run( "ip", "rule", "add", - "pref", "8830", + "pref", tailscaleRouteTable+"30", "fwmark", tailscaleBypassMark, "table", "default", ) @@ -457,23 +473,22 @@ func (r *linuxRouter) addIPRules() error { // to the tailscale routes, because that would create routing loops. rg.Run( "ip", "rule", "add", - "pref", "8850", + "pref", tailscaleRouteTable+"50", "fwmark", tailscaleBypassMark, "type", "unreachable", ) // If we get to this point, capture all packets and send them - // through to table 88, the set of tailscale routes. - // For apps other than us (ie. with no fwmark set), this is the - // first routing table, so it takes precedence over all the others, - // ie. VPN routes always beat non-VPN routes. + // through to the tailscale route table. For apps other than us + // (ie. with no fwmark set), this is the first routing table, so + // it takes precedence over all the others, ie. VPN routes always + // beat non-VPN routes. // - // NOTE(apenwarr): tables >255 are not supported in busybox. - // I really wanted to use table 8888 here for symmetry, but no luck - // with busybox alas. + // NOTE(apenwarr): tables >255 are not supported in busybox, so we + // can't use a table number that aligns with the rule preferences. rg.Run( "ip", "rule", "add", - "pref", "8888", - "table", "88", + "pref", tailscaleRouteTable+"70", + "table", tailscaleRouteTable, ) // If that didn't match, then non-fwmark packets fall through to the // usual rules (pref 32766 and 32767, ie. main and default). @@ -514,23 +529,23 @@ func (r *linuxRouter) delIPRules() error { // Delete new-style tailscale rules. rg.Run( "ip", "rule", "del", - "pref", "8810", + "pref", tailscaleRouteTable+"10", "table", "main", ) rg.Run( "ip", "rule", "del", - "pref", "8830", + "pref", tailscaleRouteTable+"30", "table", "default", ) rg.Run( "ip", "rule", "del", - "pref", "8850", + "pref", tailscaleRouteTable+"50", "type", "unreachable", ) rg.Run( "ip", "rule", "del", - "pref", "8888", - "table", "88", + "pref", tailscaleRouteTable+"70", + "table", tailscaleRouteTable, ) return rg.ErrAcc } diff --git a/wgengine/router/router_linux_test.go b/wgengine/router/router_linux_test.go index 6f5a26196..1ecfbf250 100644 --- a/wgengine/router/router_linux_test.go +++ b/wgengine/router/router_linux_test.go @@ -34,10 +34,10 @@ func mustCIDRs(ss ...string) []netaddr.IPPrefix { func TestRouterStates(t *testing.T) { basic := ` -ip rule add pref 8810 fwmark 0x80000 table main -ip rule add pref 8830 fwmark 0x80000 table default -ip rule add pref 8850 fwmark 0x80000 type unreachable -ip rule add pref 8888 table 88 +ip rule add pref 5210 fwmark 0x80000 table main +ip rule add pref 5230 fwmark 0x80000 table default +ip rule add pref 5250 fwmark 0x80000 type unreachable +ip rule add pref 5270 table 52 ` states := []struct { name string @@ -71,8 +71,8 @@ ip addr add 100.101.102.103/10 dev tailscale0` + basic, want: ` up ip addr add 100.101.102.103/10 dev tailscale0 -ip route add 100.100.100.100/32 dev tailscale0 table 88 -ip route add 192.168.16.0/24 dev tailscale0 table 88` + basic, +ip route add 100.100.100.100/32 dev tailscale0 table 52 +ip route add 192.168.16.0/24 dev tailscale0 table 52` + basic, }, { @@ -86,8 +86,8 @@ ip route add 192.168.16.0/24 dev tailscale0 table 88` + basic, want: ` up ip addr add 100.101.102.103/10 dev tailscale0 -ip route add 100.100.100.100/32 dev tailscale0 table 88 -ip route add 192.168.16.0/24 dev tailscale0 table 88` + basic, +ip route add 100.100.100.100/32 dev tailscale0 table 52 +ip route add 192.168.16.0/24 dev tailscale0 table 52` + basic, }, { @@ -102,8 +102,8 @@ ip route add 192.168.16.0/24 dev tailscale0 table 88` + basic, want: ` up ip addr add 100.101.102.104/10 dev tailscale0 -ip route add 10.0.0.0/8 dev tailscale0 table 88 -ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic + +ip route add 10.0.0.0/8 dev tailscale0 table 52 +ip route add 100.100.100.100/32 dev tailscale0 table 52` + basic + `filter/FORWARD -j ts-forward filter/INPUT -j ts-input filter/ts-forward -i tailscale0 -j MARK --set-mark 0x40000 @@ -127,8 +127,8 @@ nat/ts-postrouting -m mark --mark 0x40000 -j MASQUERADE want: ` up ip addr add 100.101.102.104/10 dev tailscale0 -ip route add 10.0.0.0/8 dev tailscale0 table 88 -ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic + +ip route add 10.0.0.0/8 dev tailscale0 table 52 +ip route add 100.100.100.100/32 dev tailscale0 table 52` + basic + `filter/FORWARD -j ts-forward filter/INPUT -j ts-input filter/ts-forward -i tailscale0 -j MARK --set-mark 0x40000 @@ -154,8 +154,8 @@ nat/POSTROUTING -j ts-postrouting want: ` up ip addr add 100.101.102.104/10 dev tailscale0 -ip route add 10.0.0.0/8 dev tailscale0 table 88 -ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic + +ip route add 10.0.0.0/8 dev tailscale0 table 52 +ip route add 100.100.100.100/32 dev tailscale0 table 52` + basic + `filter/FORWARD -j ts-forward filter/INPUT -j ts-input filter/ts-forward -i tailscale0 -j MARK --set-mark 0x40000 @@ -178,8 +178,8 @@ nat/POSTROUTING -j ts-postrouting want: ` up ip addr add 100.101.102.104/10 dev tailscale0 -ip route add 10.0.0.0/8 dev tailscale0 table 88 -ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic + +ip route add 10.0.0.0/8 dev tailscale0 table 52 +ip route add 100.100.100.100/32 dev tailscale0 table 52` + basic + `filter/FORWARD -j ts-forward filter/INPUT -j ts-input filter/ts-forward -i tailscale0 -j MARK --set-mark 0x40000 @@ -203,8 +203,8 @@ nat/POSTROUTING -j ts-postrouting want: ` up ip addr add 100.101.102.104/10 dev tailscale0 -ip route add 10.0.0.0/8 dev tailscale0 table 88 -ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic + +ip route add 10.0.0.0/8 dev tailscale0 table 52 +ip route add 100.100.100.100/32 dev tailscale0 table 52` + basic + `filter/ts-forward -i tailscale0 -j MARK --set-mark 0x40000 filter/ts-forward -m mark --mark 0x40000 -j ACCEPT filter/ts-forward -o tailscale0 -s 100.64.0.0/10 -j DROP @@ -224,8 +224,8 @@ filter/ts-input ! -i tailscale0 -s 100.64.0.0/10 -j DROP want: ` up ip addr add 100.101.102.104/10 dev tailscale0 -ip route add 10.0.0.0/8 dev tailscale0 table 88 -ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic + +ip route add 10.0.0.0/8 dev tailscale0 table 52 +ip route add 100.100.100.100/32 dev tailscale0 table 52` + basic + `filter/FORWARD -j ts-forward filter/INPUT -j ts-input filter/ts-forward -i tailscale0 -j MARK --set-mark 0x40000