Skip to content

Commit 4a09640

Browse files
committed
wireguardfilter: follow upstream ICMP grant semantics
ICMP echo handling should match the upstream Tailscale filter rather than requiring an explicit ICMP permission. Remove the protocol-only prefilter so any grant covering the source and destination IPs admits echo requests regardless of its protocol or port range; empty and shields-up policies still block new echo. Document the exception, cover TCP, UDP, SCTP, portless, explicit ICMP, and empty policies in unit tests, and update the live namespace harness to prove granted echo plus default and shields-up rejection. Tests: go test ./wireguardfilter ./profile Tests: ./integration/wireguard_filter_e2e.sh Tests: make check RELNOTE: Raw WireGuard ICMP echo follows upstream Tailscale semantics: any grant covering the source and destination IPs admits echo, regardless of protocol or port. Change-Id: I900aa4bcec4498e3fc7fb510067ab2fa2ea9c4fa
1 parent ba0fa79 commit 4a09640

5 files changed

Lines changed: 75 additions & 65 deletions

File tree

‎docs/architecture.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,10 @@ The wrapper uses upstream Tailscale packet parsing, fragment handling, flow
307307
tracking, and filter semantics. Packets delivered by WireGuard are checked as
308308
inbound before reaching the aggregate mux; host-originated packets are checked
309309
as outbound before encryption. Each profile owns independent filter state, so
310-
flows and fragments never cross profile boundaries.
310+
flows and fragments never cross profile boundaries. Upstream's ICMP rule admits
311+
echo requests whenever any match covers the source and destination IPs,
312+
ignoring that match's protocol and port range; empty and shields-up policies
313+
therefore still block new echo requests.
311314

312315
Manifest selectors compile against WireGuard AllowedIPs ownership. Source
313316
selectors are restricted to address ranges the engine can authenticate, using

‎docs/wireguard.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,13 @@ except Tailscale's reserved protocol 99; numeric TCP, UDP, and SCTP may carry
149149
ports. Ports are decimal 0 through 65535 and ranges must ascend. Names and
150150
keywords are lowercase.
151151

152+
ICMP echo requests follow upstream Tailscale filter semantics: if any grant
153+
covers the source and destination pair, echo is accepted without considering
154+
that grant's protocol or port range. This applies to TCP, UDP, SCTP, and
155+
portless-protocol grants alike. An empty policy or shields-up has no such grant
156+
and blocks new echo requests. Explicit `icmp:*` and `ipv6-icmp:*` permissions
157+
remain supported. ICMP responses and errors remain allowed as reply traffic.
158+
152159
## Shields-up override
153160

154161
Shields-up persistently suppresses all configured grants without changing the

‎integration/wireguard_filter_e2e.sh‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,15 @@ expect_tcp_blocked() {
358358
[[ $rc == 20 ]] || fail "blocked TCP flow failed for a reason other than packet timeout (status $rc)"
359359
}
360360

361+
expect_ping_allowed() {
362+
local source=$1 destination=$2 target ns
363+
[[ $destination == a ]] && target=$effective_a || target=$effective_b
364+
ns=$(namespace_for "$source")
365+
if ! sudo ip netns exec "$ns" ping -n -c 2 -W 1 "$target" >/dev/null 2>&1; then
366+
fail "ICMP flow covered by an upstream source/destination grant was blocked"
367+
fi
368+
}
369+
361370
expect_ping_blocked() {
362371
local source=$1 destination=$2 target ns
363372
[[ $destination == a ]] && target=$effective_a || target=$effective_b
@@ -392,20 +401,22 @@ effective_a=$(jq -er '.peers[0].effectiveAddresses[0]' "$work_dir/b-profile.json
392401
# with the roles reversed so each default policy is exercised.
393402
expect_tcp_allowed a b 18080 default-a-outbound
394403
expect_tcp_blocked b a 18081
404+
expect_ping_blocked b a
395405
apply_profile a tcp:18081
396406
apply_profile b default
397407
expect_tcp_allowed b a 18081 default-b-outbound
398408
expect_tcp_blocked a b 18080
399409
pass "default policy is outbound-and-replies only in both directions"
400410

401-
# A single named-peer TCP grant admits its exact port and rejects both another
402-
# TCP port and a different protocol.
411+
# A single named-peer TCP grant admits its exact TCP port, rejects another TCP
412+
# port, and follows upstream behavior by admitting ICMP for the covered
413+
# source/destination pair.
403414
apply_profile a tcp:18082
404415
apply_profile b default
405416
expect_tcp_allowed b a 18082 exact-grant
406417
expect_tcp_blocked b a 18083
407-
expect_ping_blocked b a
408-
pass "named-peer TCP grant permits only its selected port and protocol"
418+
expect_ping_allowed b a
419+
pass "named-peer TCP grant limits TCP ports and admits upstream ICMP"
409420

410421
# Keep a TCP connection open while removing the destination grant. Continuation
411422
# packets on that established flow must keep working through the policy swap.
@@ -436,6 +447,7 @@ apply_profile b tcp:18085
436447
expect_tcp_allowed a b 18085 before-shields
437448
set_shields b on
438449
expect_tcp_blocked a b 18085
450+
expect_ping_blocked a b
439451
apply_profile a tcp:18086
440452
expect_tcp_allowed b a 18086 shields-outbound
441453
set_shields b off

‎wireguardfilter/filter.go‎

Lines changed: 12 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -98,19 +98,6 @@ func prefixDestinationState(prefix netip.Prefix, local *netipx.IPSet) (string, e
9898
// outbound-flow state across ordinary policy replacements.
9999
type Policy struct {
100100
filter *filter.Filter
101-
icmp *filter.Filter
102-
}
103-
104-
func (p *Policy) runIn(parsed *packet.Parsed, rf filter.RunFlags) filter.Response {
105-
// Upstream accepts ICMP requests when a source has any open port. Raw
106-
// WireGuard grants make protocols exact, so require an ICMP-bearing grant
107-
// first; upstream still recognizes response and error types in both filters.
108-
if parsed.IPProto == ipproto.ICMPv4 || parsed.IPProto == ipproto.ICMPv6 {
109-
if p.icmp.RunIn(parsed, rf) != filter.Accept {
110-
return filter.Drop
111-
}
112-
}
113-
return p.filter.RunIn(parsed, rf)
114101
}
115102

116103
// Compile compiles cfg into an inbound allow policy. When restrictive is true,
@@ -126,9 +113,9 @@ func Compile(cfg wireguardcfg.Config, exitIP netip.Addr, restrictive bool, share
126113
return nil, err
127114
}
128115

129-
var matches, icmpMatches []filter.Match
116+
var matches []filter.Match
130117
if !restrictive {
131-
matches, icmpMatches, err = compileMatches(cfg, ownership, localNets)
118+
matches, err = compileMatches(cfg, ownership, localNets)
132119
if err != nil {
133120
return nil, err
134121
}
@@ -137,10 +124,7 @@ func Compile(cfg wireguardcfg.Config, exitIP netip.Addr, restrictive bool, share
137124
if shareStateWith != nil {
138125
shared = shareStateWith.filter
139126
}
140-
return &Policy{
141-
filter: filter.New(matches, nil, localNets, nil, shared, logger.Discard),
142-
icmp: filter.New(icmpMatches, nil, localNets, nil, nil, logger.Discard),
143-
}, nil
127+
return &Policy{filter: filter.New(matches, nil, localNets, nil, shared, logger.Discard)}, nil
144128
}
145129

146130
// Device filters packets read from and written to an underlying TUN. Its
@@ -155,7 +139,7 @@ func NewDevice(underlying tun.Device, initial *Policy) (*Device, error) {
155139
if underlying == nil {
156140
return nil, fmt.Errorf("filtered TUN requires an underlying device")
157141
}
158-
if initial == nil || initial.filter == nil || initial.icmp == nil {
142+
if initial == nil || initial.filter == nil {
159143
return nil, fmt.Errorf("filtered TUN requires an initial policy")
160144
}
161145
d := &Device{underlying: underlying}
@@ -166,7 +150,7 @@ func NewDevice(underlying tun.Device, initial *Policy) (*Device, error) {
166150
// Install atomically replaces the current policy. A nil policy is rejected so
167151
// the device can never accidentally become allow-all.
168152
func (d *Device) Install(policy *Policy) error {
169-
if policy == nil || policy.filter == nil || policy.icmp == nil {
153+
if policy == nil || policy.filter == nil {
170154
return fmt.Errorf("cannot install a nil packet filter")
171155
}
172156
d.policy.Store(policy)
@@ -215,7 +199,7 @@ func (d *Device) Write(bufs [][]byte, offset int) (int, error) {
215199
policy := d.policy.Load()
216200
var parsed packet.Parsed
217201
parsed.Decode(buf[offset:])
218-
if policy.runIn(&parsed, 0) == filter.Accept {
202+
if policy.filter.RunIn(&parsed, 0) == filter.Accept {
219203
accepted = append(accepted, buf)
220204
}
221205
}
@@ -334,24 +318,24 @@ func buildOwnership(cfg wireguardcfg.Config, exitIP netip.Addr) (ownership, erro
334318
return result, nil
335319
}
336320

337-
func compileMatches(cfg wireguardcfg.Config, owned ownership, local *netipx.IPSet) ([]filter.Match, []filter.Match, error) {
338-
var matches, icmpMatches []filter.Match
321+
func compileMatches(cfg wireguardcfg.Config, owned ownership, local *netipx.IPSet) ([]filter.Match, error) {
322+
var matches []filter.Match
339323
for grantIndex, grant := range cfg.PacketFilter.Grants {
340324
sources, err := resolveSources(grant.Src, owned)
341325
if err != nil {
342-
return nil, nil, fmt.Errorf("grants[%d]: %w", grantIndex, err)
326+
return nil, fmt.Errorf("grants[%d]: %w", grantIndex, err)
343327
}
344328
destinations, err := resolveDestinations(grant.Dst, local)
345329
if err != nil {
346-
return nil, nil, fmt.Errorf("grants[%d]: %w", grantIndex, err)
330+
return nil, fmt.Errorf("grants[%d]: %w", grantIndex, err)
347331
}
348332
if len(destinations.Prefixes()) == 0 {
349333
continue
350334
}
351335
for _, permissionText := range grant.IP {
352336
permission, err := wireguardcfg.ParsePermission(permissionText)
353337
if err != nil {
354-
return nil, nil, fmt.Errorf("grants[%d]: ip: %w", grantIndex, err)
338+
return nil, fmt.Errorf("grants[%d]: ip: %w", grantIndex, err)
355339
}
356340
protocols := permissionProtocols(permission)
357341
ports := filter.PortRange{First: 0, Last: 65535}
@@ -363,20 +347,9 @@ func compileMatches(cfg wireguardcfg.Config, owned ownership, local *netipx.IPSe
363347
match.Dsts = append(match.Dsts, filter.NetPortRange{Net: prefix, Ports: ports})
364348
}
365349
matches = append(matches, match)
366-
var icmpProtocols []ipproto.Proto
367-
for _, protocol := range protocols {
368-
if protocol == ipproto.ICMPv4 || protocol == ipproto.ICMPv6 {
369-
icmpProtocols = append(icmpProtocols, protocol)
370-
}
371-
}
372-
if len(icmpProtocols) > 0 {
373-
icmpMatch := match
374-
icmpMatch.IPProto = views.SliceOf(icmpProtocols)
375-
icmpMatches = append(icmpMatches, icmpMatch)
376-
}
377350
}
378351
}
379-
return matches, icmpMatches, nil
352+
return matches, nil
380353
}
381354

382355
func resolveSources(values []string, owned ownership) (*netipx.IPSet, error) {

‎wireguardfilter/filter_test.go‎

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func TestCompileEnforcesSelectorsAndLongestPrefixOwnership(t *testing.T) {
6262
}
6363
for _, test := range tests {
6464
t.Run(test.name, func(t *testing.T) {
65-
if got := policy.runIn(&test.pkt, 0); got != test.want {
65+
if got := policy.filter.RunIn(&test.pkt, 0); got != test.want {
6666
t.Fatalf("RunIn() = %v, want %v", got, test.want)
6767
}
6868
})
@@ -76,7 +76,7 @@ func TestCompileInactiveDestinationsAndExplicitSourceValidation(t *testing.T) {
7676
}}}
7777
policy := mustCompile(t, cfg, netip.Addr{}, false, nil)
7878
pkt := parsedPacket(ipproto.TCP, "10.0.0.2", "10.0.0.1", 40000, 22, packet.TCPSyn)
79-
if got := policy.runIn(&pkt, 0); got != filter.Drop {
79+
if got := policy.filter.RunIn(&pkt, 0); got != filter.Drop {
8080
t.Fatalf("inactive destination grant accepted packet: %v", got)
8181
}
8282

@@ -104,41 +104,56 @@ func TestPolicyStateSharingAndIsolation(t *testing.T) {
104104
t.Fatalf("RunOut() = %v, want accept", got)
105105
}
106106
reply := parsedPacket(ipproto.UDP, "10.0.0.2", "10.0.0.1", 53, 41000, 0)
107-
if got := base.runIn(&reply, 0); got != filter.Accept {
107+
if got := base.filter.RunIn(&reply, 0); got != filter.Accept {
108108
t.Fatalf("reply through base = %v, want accept", got)
109109
}
110110

111111
shared := mustCompile(t, cfg, netip.Addr{}, true, base)
112-
if got := shared.runIn(&reply, 0); got != filter.Accept {
112+
if got := shared.filter.RunIn(&reply, 0); got != filter.Accept {
113113
t.Fatalf("reply through shared restrictive policy = %v, want accept", got)
114114
}
115115
isolated := mustCompile(t, cfg, netip.Addr{}, true, nil)
116-
if got := isolated.runIn(&reply, 0); got != filter.Drop {
116+
if got := isolated.filter.RunIn(&reply, 0); got != filter.Drop {
117117
t.Fatalf("reply through isolated restrictive policy = %v, want drop", got)
118118
}
119119
otherProfile := mustCompile(t, cfg, netip.Addr{}, false, nil)
120-
if got := otherProfile.runIn(&reply, 0); got != filter.Drop {
120+
if got := otherProfile.filter.RunIn(&reply, 0); got != filter.Drop {
121121
t.Fatalf("independent profile reused flow state: %v", got)
122122
}
123123
}
124124

125-
func TestPolicyRequiresExplicitICMPGrant(t *testing.T) {
125+
func TestPolicyUsesUpstreamICMPSourceDestinationSemantics(t *testing.T) {
126126
cfg := filterTestConfig()
127-
cfg.PacketFilter = wireguardcfg.PacketFilter{Grants: []wireguardcfg.Grant{{
128-
Src: []string{"peer:alpha"}, Dst: []string{"self"}, IP: []string{"tcp:22"},
129-
}}}
130127
requestBytes := icmp4Packet(8, "10.0.0.2", "10.0.0.1")
131128
var request packet.Parsed
132129
request.Decode(requestBytes)
133-
policy := mustCompile(t, cfg, netip.Addr{}, false, nil)
134-
if got := policy.runIn(&request, 0); got != filter.Drop {
135-
t.Fatalf("ICMP request through TCP-only grant = %v, want drop", got)
136-
}
137130

138-
cfg.PacketFilter.Grants[0].IP = []string{"icmp:*"}
139-
policy = mustCompile(t, cfg, netip.Addr{}, false, nil)
140-
if got := policy.runIn(&request, 0); got != filter.Accept {
141-
t.Fatalf("ICMP request through explicit grant = %v, want accept", got)
131+
tests := []struct {
132+
name string
133+
permission string
134+
want filter.Response
135+
}{
136+
{name: "no grants", want: filter.Drop},
137+
{name: "TCP grant", permission: "tcp:22", want: filter.Accept},
138+
{name: "UDP grant", permission: "udp:53", want: filter.Accept},
139+
{name: "SCTP grant", permission: "sctp:9", want: filter.Accept},
140+
{name: "portless grant", permission: "gre:*", want: filter.Accept},
141+
{name: "explicit ICMP grant", permission: "icmp:*", want: filter.Accept},
142+
}
143+
for _, test := range tests {
144+
t.Run(test.name, func(t *testing.T) {
145+
if test.permission == "" {
146+
cfg.PacketFilter = wireguardcfg.PacketFilter{}
147+
} else {
148+
cfg.PacketFilter = wireguardcfg.PacketFilter{Grants: []wireguardcfg.Grant{{
149+
Src: []string{"peer:alpha"}, Dst: []string{"self"}, IP: []string{test.permission},
150+
}}}
151+
}
152+
policy := mustCompile(t, cfg, netip.Addr{}, false, nil)
153+
if got := policy.filter.RunIn(&request, 0); got != test.want {
154+
t.Fatalf("ICMP request with %q = %v, want %v", test.permission, got, test.want)
155+
}
156+
})
142157
}
143158
}
144159

@@ -148,19 +163,19 @@ func TestPolicyPreservesUpstreamTCPAndICMPSemantics(t *testing.T) {
148163
policy := mustCompile(t, cfg, netip.Addr{}, false, nil)
149164

150165
tcpContinuation := parsedPacket(ipproto.TCP, "10.0.0.2", "10.0.0.1", 22, 40000, packet.TCPAck)
151-
if got := policy.runIn(&tcpContinuation, 0); got != filter.Accept {
166+
if got := policy.filter.RunIn(&tcpContinuation, 0); got != filter.Accept {
152167
t.Fatalf("TCP continuation = %v, want accept", got)
153168
}
154169
tcpSYN := parsedPacket(ipproto.TCP, "10.0.0.2", "10.0.0.1", 22, 40000, packet.TCPSyn)
155-
if got := policy.runIn(&tcpSYN, 0); got != filter.Drop {
170+
if got := policy.filter.RunIn(&tcpSYN, 0); got != filter.Drop {
156171
t.Fatalf("TCP SYN = %v, want drop", got)
157172
}
158173
icmpReply := parsedPacket(ipproto.ICMPv4, "10.0.0.2", "10.0.0.1", 0, 0, 0)
159174
icmpReply.StuffForTesting(28)
160175
// ICMP echo reply type.
161176
icmpReplyBytes := icmp4Packet(0, "10.0.0.2", "10.0.0.1")
162177
icmpReply.Decode(icmpReplyBytes)
163-
if got := policy.runIn(&icmpReply, 0); got != filter.Accept {
178+
if got := policy.filter.RunIn(&icmpReply, 0); got != filter.Accept {
164179
t.Fatalf("ICMP response = %v, want accept", got)
165180
}
166181
}

0 commit comments

Comments
 (0)