Repository navigation
sec(auth): account scope can be laundered through the group-membership endpoint (join a wider group, or leave the scoping one) #1756
Description
Activity
Correction to this issue body: it describes code that does not exist on
mainI wrote this issue from an independent review of PR #1737, and described the fix in terms of that PR's structure without checking the names against
main. Correcting it before anyone acts on it.Does not exist on
main:guardSelfEscalation NO (added by #1737) guardSelfCarvedOutGrant NO (added by #1737) addedGroups NO (added by #1737)Does exist on
main:guardGroupChange,addsNewGroup. Onmain,guardGroupChangestill carries the old inline self-escalation branch — a directHasPermission(update:users)check, no extracted helper, and no prior-membership resolution.That last point matters for the fix: "compare resulting scope against prior scope" needs a prior-membership snapshot, which #1737 introduces and
maindoes not have.The defect itself is confirmed, on current
mainReproduced live at
9bfd6822c, both routes:ROUTE1 join wider group ACCEPTED scope acct-A -> ALL ACCOUNTS ROUTE2 leave scoping group ACCEPTED scope acct-A -> [] = ALL ACCOUNTSSo the issue is real and the severity stands. Only the implementation guidance was written against the wrong tree.
Sequencing decision: this stacks on #1737
Implementing against
main's current structure would mean building the prior-scope resolution that #1737 already introduces, then reconciling two versions of the same guard at merge — two divergent implementations of one security guard, merged by hand, which is how a bypass gets introduced by the fix for a bypass.So this waits on #1737 and reuses its
accountScopeGaprather than duplicating it. #1737 is rebased, MERGEABLE, and with a reviewer.Two implementation traps, both learned on #1737
- Test the prior scope with
len(...) == 0, notIsUnrestrictedAccess(...). A scope containing the wildcard is already maximally wide, so no membership change can widen it — and all seven seeded groups carry the wildcard. Getting this wrong refuses every Administrators member. This exact predicate error blocked PR sec(auth): fail closed when a user's account scope cannot be established #1752. - Edit by function body, not first match.
guardGroupChangeandguardSelfEscalationare neighbours and share repeating mock-setup anchors; a first-match replace lands in the wrong function and the test still passes. That happened twice on sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737 and was caught only by mutation-verifying that the kill was assertion-driven rather than panic-driven.
Verification bar unchanged
Four directions, because a refusal-only suite passes against a fix that refuses every membership edit:
- scoped actor joins Administrators (wildcard scope) -> refused
- scoped actor drops the group carrying their restriction -> refused
- membership change that does not widen scope -> still allowed
- actor joins a group whose scope is a subset of their own -> still allowed
- Test the prior scope with
Three corrections to this issue's diagnosis, found while fixing it (PR #1808)
1. "A default deployment ships exactly one" broader group is wrong, and the real number is worse.
Every seeded group ships
allowed_accounts = ARRAY['*'], not just Administrators: migrations000024,000057,000059/000064,000096. So Route 1 has several viable laundering targets rather than the single one this issue named.That does not change the fix, because the guard keys off the scope value rather than a group ID, so all of them are covered by construction. But anyone reading this issue to assess exposure would have undercounted the attack surface, and anyone tempted to fix it by special-casing the Administrators group would have shipped a guard with several holes left open.
2. "The
addsNewGroupearly return has to go" was right about the effect, wrong about the disposition.Deleting it outright would also have forced removal-only self-edits to hold
update:usersinherited from prior groups, which is a behaviour change beyond what this issue asks for. It was instead moved insideguardSelfEscalation, scoping it to the permission axis, where a removal genuinely can only shrink the union. The account axis is now evaluated unconditionally, which is what actually closes Route 2.3. The realistic attacker is narrower than this issue implies.
PUT /api/users/{id}is anAuthAdminroute requiring{admin, *}. The actor is therefore a scoped administrator, not any holder ofupdate:users.I am leaving
severity/criticalin place. A scoped administrator reaching unrestricted access still crosses the exact boundary that scoping exists to enforce, in a single self-edit, and the fact that they already hold admin within their scope is what makes the escalation plausible rather than exotic. But the issue as written reads as a broader-reachability bug than it is, and anyone re-triaging should have the accurate precondition.Note on the fix's own review
The first implementation refused on any unresolved group, which would have refused legitimate edits in ordinary operation:
DeleteGroupdrops thegroupsrow without purgingusers.group_ids(an array column with no FK), so a dangling membership ID is the normal state after any custom group is deleted. Under refuse-on-any-skip, an already-unrestricted admin could not remove that dangling ID from their own membership - a change widening nothing, by a principal already at maximum scope, in a single-admin deployment with nobody able to clean up.That was corrected without failing open. A skipped group can only widen the union when what survives is empty, because empty is the value read as unrestricted. The condition is now
len(AllowedAccounts) == 0 && SkippedGroups > 0pluslen(Groups) == 0, matchinggrantCeilingAccountsandResolveAllowedAccountscharacter for character. The ambiguous empty-with-a-skip case is still refused, so #1748 is preserved rather than reintroduced, and it is pinned from both directions: one mutant tightening back to refuse-on-any-skip fails the dangling-group control, another dropping the emptiness condition fails the fail-closed test.
Found by independent adversarial review of PR #1737. Live on
main. PR #1737 bounds what an actor may grant given their account scope; this is how an actor raises their own scope in a single self-edit, through a neighbouring endpoint that #1737 already guards on the permission dimension but not the account dimension.Two independent routes, opposite directions.
Route 1 — join a wider group
guardSelfEscalationgates self-added groups onupdate:users, which the acting user already holds. It then callsguardSelfCarvedOutGrant, which checks only the carved-out money verbs of the group being joined.Account scope is never checked. So joining any broader-scoped group is permitted, and a default deployment ships exactly one: Administrators, seeded
allowed_accounts = ARRAY[*]by migrations000024/000057.Route 2 — leave the scoping group
guardGroupChangereturns early unlessaddsNewGroup(prior, next). A self-edit that only removes groups is therefore not guarded at all.Dropping the group carrying the account restriction leaves
AllowedAccounts = [], whichIsUnrestrictedAccessreads as all accounts.Removing the restriction grants the restriction. That inversion is the reason this is worth fixing at the representation and not only at the guard.
Why the existing guards do not catch it
Both routes exercise the permission dimensions guard correctly. The defect is that the account dimension has no equivalent on this endpoint — the same asymmetry PR #1737 was written to close on the group-write path, still open on the membership path.
Fix
Apply an account-scope ceiling to self-membership changes, in both directions:
Note that
guardGroupChangesaddsNewGroupearly return has to go, or the removal route stays unguarded no matter what is added inside it.Verification
Both directions, and a control — a refusal-only test proves nothing here, since a fix refusing all membership edits would pass it:
allowed_accounts = [*]) -> refusedMutation-verify per test, run individually: an aggregate package run misreports this because a mock panic kills the binary.
Related