From 0eb38dc2e50715c535b824c1e01ddedf8fe64565 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 22 Jul 2026 08:27:15 -0600 Subject: [PATCH] ipn,magicsock: deny peer capabilities to unsigned peers (#20561) Unsigned peers aren't covered by tailnet lock, so they must never hold peer capabilities even if the packet filter grants them. This change extends the check for unsigned-peers to ensure full coverage in capabilities. Fixes tailscale/corp#45116 Change-Id: I918af24f0b9855e55921cbdad109cc68e745e125 Signed-off-by: Mike Jensen --- control/controlclient/map.go | 6 ++ control/controlclient/map_test.go | 18 ++++++ ipn/ipnlocal/local_test.go | 89 ++++++++++++++++++++++++++++ ipn/ipnlocal/node_backend.go | 23 +++++++ wgengine/magicsock/magicsock.go | 5 ++ wgengine/magicsock/magicsock_test.go | 21 +++++++ 6 files changed, 162 insertions(+) diff --git a/control/controlclient/map.go b/control/controlclient/map.go index a376d8740..77ca0e68d 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -406,6 +406,12 @@ func upgradeNode(n *tailcfg.Node) { if n.AllowedIPs == nil { n.AllowedIPs = slices.Clone(n.Addresses) } + // Unsigned peers aren't covered by tailnet lock, so a (possibly malicious) + // control server must not grant them network access via advertised routes. + // Strip any AllowedIPs beyond their own addresses. + if n.UnsignedPeerAPIOnly && !slices.Equal(n.AllowedIPs, n.Addresses) { + n.AllowedIPs = slices.Clone(n.Addresses) + } } func (ms *mapSession) tryHandleIncrementally(res *tailcfg.MapResponse) bool { diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index 6443b4b24..fb4307c39 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -1547,6 +1547,24 @@ func TestUpgradeNode(t *testing.T) { in: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{}}, want: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{}}, }, + { + // An unsigned peer is not covered by tailnet lock and must not carry advertised routes + name: "unsigned-peer-strips-extra-allowed-ips", + in: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{a1, a2, a3, a4}, UnsignedPeerAPIOnly: true}, + want: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{a1, a2}, UnsignedPeerAPIOnly: true}, + }, + { + // An unsigned peer whose AllowedIPs already equal its Addresses is left untouched + name: "unsigned-peer-allowed-ips-equal-addresses", + in: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{a1, a2}, UnsignedPeerAPIOnly: true}, + want: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{a1, a2}, UnsignedPeerAPIOnly: true}, + }, + { + // A signed peer keeps its advertised routes: the strip only applies to unsigned peers + name: "signed-peer-keeps-extra-allowed-ips", + in: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{a1, a2, a3, a4}}, + want: &tailcfg.Node{Addresses: []netip.Prefix{a1, a2}, AllowedIPs: []netip.Prefix{a1, a2, a3, a4}}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 814ec8c72..d72d59c43 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -8107,6 +8107,95 @@ func TestSrcCapPacketFilterUnsignedPeer(t *testing.T) { } } +// TestCapsGrantPacketFilterUnsignedPeer verifies that a CapGrant-style packet filter match +// (Srcs + Caps, no Dsts) whose broad Srcs covers an unsigned peer's AllowedIPs does NOT cause +// the packet filter to be discarded: a grant alone permits no traffic, so legitimate rules for +// signed peers must keep working. +func TestCapsGrantPacketFilterUnsignedPeer(t *testing.T) { + lb := newLocalBackendWithTestControl(t, false, func(tb testing.TB, opts controlclient.Options) controlclient.Client { + return newClient(tb, opts) + }) + if err := lb.Start(ipn.Options{}); err != nil { + t.Fatalf("(*LocalBackend).Start(): %v", err) + } + + var signedKey, unsignedKey key.NodePublic + must.Do(signedKey.UnmarshalText([]byte("nodekey:5c8f86d5fc70d924e55f02446165a5dae8f822994ad26bcf4b08fd841f9bf261"))) + must.Do(unsignedKey.UnmarshalText([]byte("nodekey:6c8f86d5fc70d924e55f02446165a5dae8f822994ad26bcf4b08fd841f9bf262"))) + + controlClient := lb.cc.(*mockControl) + // Send a netmap with: + // - a broad CapGrant-style match (Srcs 0.0.0.0/0) covering both peers + // - a legitimate traffic rule for the signed peer + controlClient.send(sendOpt{nm: &netmap.NetworkMap{ + SelfNode: (&tailcfg.Node{ + Addresses: []netip.Prefix{netip.MustParsePrefix("1.1.1.1/32")}, + }).View(), + Peers: []tailcfg.NodeView{ + (&tailcfg.Node{ + Addresses: []netip.Prefix{netip.MustParsePrefix("2.2.2.2/32")}, + AllowedIPs: []netip.Prefix{netip.MustParsePrefix("2.2.2.2/32")}, + ID: 2, + Key: signedKey, + }).View(), + (&tailcfg.Node{ + Addresses: []netip.Prefix{netip.MustParsePrefix("3.3.3.3/32")}, + AllowedIPs: []netip.Prefix{netip.MustParsePrefix("3.3.3.3/32")}, + ID: 3, + Key: unsignedKey, + UnsignedPeerAPIOnly: true, + }).View(), + }, + PacketFilter: []filtertype.Match{ + { + // Broad grant covering both peers: must NOT trigger vetting + Srcs: []netip.Prefix{netip.MustParsePrefix("0.0.0.0/0")}, + Caps: []filtertype.CapMatch{{ + Dst: netip.MustParsePrefix("1.1.1.1/32"), + Cap: "cap-X", + }}, + }, + { + // Legitimate traffic rule for the signed peer + IPProto: views.SliceOf([]ipproto.Proto{ipproto.TCP}), + Srcs: []netip.Prefix{netip.MustParsePrefix("2.2.2.2/32")}, + Dsts: []filtertype.NetPortRange{{ + Net: netip.MustParsePrefix("1.1.1.1/32"), + Ports: filtertype.PortRange{First: 22, Last: 22}, + }}, + }, + }, + }}) + + f := lb.ForTest().GetFilter() + + // The filter must be installed, not discarded: the signed peer's legitimate traffic rule accepts traffic + if res := f.Check(netip.MustParseAddr("2.2.2.2"), netip.MustParseAddr("1.1.1.1"), 22, ipproto.TCP); res != filter.Accept { + t.Errorf("Check(signed 2.2.2.2, ...) = %s, want %s (filter must not be discarded for a caps-only grant)", res, filter.Accept) + } + + // The unsigned peer has no traffic rule, so its packets are dropped + if res := f.Check(netip.MustParseAddr("3.3.3.3"), netip.MustParseAddr("1.1.1.1"), 22, ipproto.TCP); !res.IsDrop() { + t.Errorf("Check(unsigned 3.3.3.3, ...) = %s, want drop", res) + } + + // The unsigned peer is denied the granted capability at resolution time + if caps := lb.PeerCapsForIP(netip.MustParseAddr("3.3.3.3"), netip.MustParseAddr("1.1.1.1")); len(caps) != 0 { + t.Errorf("PeerCapsForIP(unsigned src) = %v, want empty", caps) + } + if caps := lb.PeerCaps(netip.MustParseAddr("3.3.3.3")); len(caps) != 0 { + t.Errorf("PeerCaps(unsigned src) = %v, want empty", caps) + } + + // The signed peer keeps its grant: legitimate functionality is unaffected + if caps := lb.PeerCapsForIP(netip.MustParseAddr("2.2.2.2"), netip.MustParseAddr("1.1.1.1")); !caps.HasCapability("cap-X") { + t.Errorf("PeerCapsForIP(signed src) missing cap-X: %v", caps) + } + if caps := lb.PeerCaps(netip.MustParseAddr("2.2.2.2")); !caps.HasCapability("cap-X") { + t.Errorf("PeerCaps(signed src) missing cap-X: %v", caps) + } +} + func TestDisplayMessages(t *testing.T) { b := newTestLocalBackend(t) diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index e823b3759..a0b539eb1 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/ipn/ipnlocal/node_backend.go @@ -418,7 +418,24 @@ func (nb *nodeBackend) PeerCaps(src netip.Addr) tailcfg.PeerCapMap { return nb.peerCapsLocked(src) } +// srcIsUnsignedPeerLocked reports whether src is an address of a peer with +// UnsignedPeerAPIOnly set. Such peers are not covered by tailnet lock and must +// never be granted peer capabilities. +// +// nb.mu must be held before calling. +func (nb *nodeBackend) srcIsUnsignedPeerLocked(src netip.Addr) bool { + id, ok := nb.nodeByAddr[src] + if !ok { + return false + } + n, ok := nb.nodeByIDLocked(id) + return ok && n.UnsignedPeerAPIOnly() +} + func (nb *nodeBackend) peerCapsLocked(src netip.Addr) tailcfg.PeerCapMap { + if nb.srcIsUnsignedPeerLocked(src) { + return nil + } if nb.netMap == nil { return nil } @@ -447,6 +464,9 @@ func (nb *nodeBackend) peerCapsLocked(src netip.Addr) tailcfg.PeerCapMap { func (nb *nodeBackend) PeerCapsForIP(src, dst netip.Addr) tailcfg.PeerCapMap { nb.mu.Lock() defer nb.mu.Unlock() + if nb.srcIsUnsignedPeerLocked(src) { + return nil + } if nb.netMap == nil { return nil } @@ -464,6 +484,9 @@ func (nb *nodeBackend) PeerCapsForIP(src, dst netip.Addr) tailcfg.PeerCapMap { func (nb *nodeBackend) PeerCapsForService(src netip.Addr, svcName tailcfg.ServiceName) tailcfg.PeerCapMap { nb.mu.Lock() defer nb.mu.Unlock() + if nb.srcIsUnsignedPeerLocked(src) { + return nil + } if nb.netMap == nil { return nil } diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 7e508df84..16e988cb3 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -2965,6 +2965,11 @@ func nodeHasCap(filt *filter.Filter, src, dst tailcfg.NodeView, cap tailcfg.Peer !dst.Valid() { return false } + if src.UnsignedPeerAPIOnly() { + // Unsigned peers aren't covered by tailnet lock and must never hold + // peer capabilities such as relay allocation/target. + return false + } for _, srcPrefix := range src.Addresses().All() { if !srcPrefix.IsSingleIP() { continue diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 94cb528ab..488c2e5f7 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -3821,6 +3821,9 @@ func Test_nodeHasCap(t *testing.T) { nodeDOnlyIPv6 := nodeCOnlyIPv4.Clone() nodeDOnlyIPv6.Addresses[0] = netip.MustParsePrefix("::2/128") + nodeCUnsigned := nodeCOnlyIPv4.Clone() + nodeCUnsigned.UnsignedPeerAPIOnly = true + tests := []struct { name string filt *filter.Filter @@ -3937,6 +3940,24 @@ func Test_nodeHasCap(t *testing.T) { cap: tailcfg.PeerCapabilityRelayTarget, want: false, }, + { + name: "unsigned-src", + filt: filter.New([]filtertype.Match{ + { + Srcs: []netip.Prefix{netip.MustParsePrefix("2.2.2.2/32")}, + Caps: []filtertype.CapMatch{ + { + Dst: netip.MustParsePrefix("1.1.1.1/32"), + Cap: tailcfg.PeerCapabilityRelayTarget, + }, + }, + }, + }, nil, nil, nil, nil, nil), + src: nodeCUnsigned.View(), + dst: nodeAOnlyIPv4.View(), + cap: tailcfg.PeerCapabilityRelayTarget, + want: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {