fix: validate agent-supplied AllowedIPs in coordinator#26144
Conversation
AgentCoordinateeAuth.Authorize validated upd.Node.Addresses but applied no check to upd.Node.AllowedIps. Because AllowedIPs are installed verbatim into the WireGuard peer config and drive routing, a malicious agent could advertise a victim agent's /128 and have traffic routed to it, intercepting sessions intended for the victim workspace. Apply the same UUID-derivation validation to AllowedIps that is already used for Addresses, via a shared authorizeNodePrefixes helper. This covers both the in-memory and Postgres coordinators, which share this auth path. Fixes PLAT-264 (SEC-89).
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 4 findings (1 P3, 1 P4, 2 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 0 P0-P1, 0 P2, 1 P3, 1 P4, 2 Nit posted. 5 Nit dropped. Reviewed against f242ef0..58e6baf. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-targeted security fix. Extracting authorizeNodePrefixes and applying it to both Addresses and AllowedIPs closes the routing-hijack gap at the single chokepoint shared by both coordinators. The refactoring is mechanical, the test coverage hits three layers (unit, in-memory coordinator, PG coordinator), and the PR description traces every claim to a file and line.
As Bisky put it: "Three layers of testing, each proving a different slice of the security behavior, and not a costume gem in the bunch."
Pariston explored four framings of the vulnerability (asymmetric validation, server-side override, whitelist construction, test coverage gap) and confirmed the chosen framing is at the right causal level. Meruem verified no new concurrency hazards. Kurapika, Kite, and Knov found no security gaps in the fix itself. Hisoka probed edge cases including IPv4 injection (blocked by the /128 check) and the legacy workspace agent IP (pre-existing, no agents emit it anymore).
Severity summary: 1 P3, 1 P4, 2 Nit.
tailnet/tunnel.go:132
P4 [CRF-1] handleClientNodeRequests has the same AllowedIPs validation gap this PR fixes for agents. It validates Addresses (bit count only, no UUID derivation) but skips AllowedIps entirely. configmaps.go:619 installs AllowedIPs for all peers unconditionally, so the field reaches WireGuard routing for clients too.
Exploitability is lower for clients (ServerTailnet routes to agents, not clients), and the client path intentionally applies a weaker policy (bit count only, no UUID binding). But the complete absence of AllowedIPs validation is the same class of bug. Worth a follow-up. (Netero, Hisoka)
🤖
🤖 This review was automatically generated with Coder Agents.
Wrap the errors returned by authorizeNodePrefixes so an operator can tell whether a rejected prefix came from Addresses or AllowedIps instead of having to compare the address against both fields manually. Rename the loop variable to prefixStr to match the parameter, and correct a test comment that said "immediately" while asserting eventual closure. Addresses review feedback on #26144.
The AgentCoordinateeAuth.Authorize AllowedIPs cases are already covered at the coordinator level (in-memory AgentWithoutClients_InvalidAllowedIP and PG TestPGCoordinatorSingle_AgentInvalidAllowedIP), matching the existing InvalidAddress and InvalidBits coverage. The unit test duplicated that matrix, slowing the suite and creating two places to maintain.
f0ssel
deleted the
garrett/plat-264-agent-supplied-allowedips-not-validated-by-coordinator-sec
branch
3 participants
AgentCoordinateeAuth.Authorizevalidated every prefix inupd.Node.Addresses(each must be a/128derived from the authenticating agent's own UUID) but applied no equivalent check toupd.Node.AllowedIps. BecauseAllowedIPsare installed verbatim into the WireGuard peer config (tailnet/configmaps.go) and WireGuard routing is driven byAllowedIPs, a malicious agent could advertise a victim agent's/128and become an eligible route for that IP. WithServerTailnettunneling to many agents and routing by destination IP, this could let an attacker intercept sessions intended for the victim workspace.This applies the same UUID-derivation validation to
AllowedIpsthat already guardsAddresses, extracted into a sharedauthorizeNodePrefixeshelper. The check is the single chokepoint used by both the in-memory coordinator (tailnet/coordinator.go) and the Postgres coordinator (enterprise/tailnet/connio.go), so one fix covers both. Legitimate agents are unaffected: an agent'sAllowedIPsis a clone of itsAddresses(tailnet/node.go), which are already UUID-derived/128s.Fixes PLAT-264 (SEC-89): https://linear.app/codercom/issue/PLAT-264
Implementation notes and decision log
Root cause
Asymmetric validation in
tailnet/tunnel.go:Addresseswere bound to the agent's UUID, butAllowedIpswere trusted as-is and propagated into the WireGuard peer config, which drives routing.Why the fix is safe for legitimate agents
tailnet/node.gobuilds the node withAllowedIPs: slices.Clone(u.addresses), identical toAddresses.agent/agent.gosets those addresses toTailscaleServicePrefix.PrefixFromUUID(agentID)andCoderServicePrefix.PrefixFromUUID(agentID)(both/128, UUID-derived).Addressescheck already accepts exactly those prefixes plus the legacy workspace agent IP, so identical validation ofAllowedIPspasses for real traffic and only rejects forged prefixes.Coverage: one method, both coordinators
AgentCoordinateeAuth.Authorizeis the shared auth path. A failedAuthorizeis wrapped asAuthorizationError{Wrapped: err}and closes the agent's response stream.Tests
tailnet/tunnel_internal_test.go: fast unit tests onAuthorize(valid AllowedIPs accepted; foreign/128rejected withInvalidNodeAddressError; wrong-bits rejected withInvalidAddressBitsError).tailnet/coordinator_test.go: in-memory coordinator closes the agent stream on a forgedAllowedIp.enterprise/tailnet/pgcoord_test.go: same regression for the Postgres coordinator.Verified the regression tests fail when the new
AllowedIpscheck is disabled, then pass with it enabled. Local validation: targeted tests (in-memory, internal, and Postgres-backed enterprise), plusmake pre-commit(gen/fmt/lint/build) passing.