fix: attribute /proc scan instability to the entry's owner - #44
Merged
Merged
Conversation
processSnapshotForUID let ANY vanishing numeric /proc entry clear the whole snapshot's stable flag, and processesForUID needs two consecutive stable empty snapshots before it will report a UID absent. Unrelated background activity on a busy host therefore denied the scan, and any local account could deny it on purpose with a plain fork loop. Both call sites are load-bearing: the UID-reuse check after account creation, so invite fails, and the post-SIGKILL sweep in TerminateProcesses, so account deletion fails. A cheap first pass now records each entry's directory owner while the listing is still fresh. /proc/<pid> carries the process's real UID, and moving a thread to another UID needs privilege this threat model does not grant an unprivileged local user, so an entry owned by another unprivileged account can neither host nor fork a target-UID thread and its disappearance says nothing about whether the target UID is absent. Root, the target UID, and entries that vanished before they could be attributed all stay conservative. Accepted residual: a multi-threaded process whose leader dropped to another UID while a worker still holds root is attributed to the leader. Measured on this four-core host, 30 scans per run: continuous unrelated forking went from 27-29 failures to 2-3, a steady 50 processes per second from 8-19 to none, and an idle host stayed at none. An earlier attempt that instead spread the retries over a longer delay was measured and discarded: it made a moderate load worse (27/30), because the two confirming snapshots must be close together to fall between disturbances. The refusal now reports how many attempts were disturbed, so the operator can distinguish host churn from a fault in the account being revoked. The at-queue bound gains the same treatment: its refusal names that the count spans every account's jobs and points at atq, and its comment records why raising the bound would only trade a fast refusal for a slow timeout, and that this cleanup runs after the grants are already stripped and the login already disabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
None of the three is reachable on a released build; each is a guard that does not hold on its own terms, found while triaging the audit's low-severity list. planLogin's deferred Match-Group branch returned fixSSHD from `fix == "yes"` alone, while the blocker path eleven lines below refuses when a.SSHD is nil. The authorization survives confirmLogin and is applied in installCredentialsAndGrants, which calls a.SSHD.Grant after useradd, after the registry row is completed, and after the key is written — so an unwired manager would abort a root process mid-transaction rather than fail closed. Production always wires SSHD, so this is defence in depth, restored by gating the deferred authorization on the same condition. MatchBlock validated nothing while Grant and Remove both re-validate "even if a future caller forgets to validate", and dropIn re-validates the group name. Its output is not decorative: printSSHDFixHint renders it as a heredoc the operator is told to write into a privileged sshd config and reload. A name carrying a newline therefore paints extra directives into that paste; the added test demonstrates exactly that against the unguarded renderer. clearReusedUsername read Registry.UnitFor to recover "the only direct handle to an at job from an older run", but the Lookup above it already fails the whole invite for any username that still holds a registry row, and UnitFor scans that same row set — so it could only ever return "". The dead read and its dead error path are gone and the comment now says what actually clears an older run: Cancel sweeping by name across every namespace. Both new tests were mutation-checked: reverting each guard makes its test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… account inviteTransaction.rollback walks its cleanups in reverse, and the cancellation was registered at scheduling time — last in, so first out. It therefore ran before rollbackInviteAccount, which is the step that decides whether the account can be deleted at all: mayDelete is sudoRemovalConfirmed && sshdRemovalConfirmed, so a failed grant removal deliberately retains a disabled account. The account was then left holding a live sudo drop-in with nothing scheduled to come back for it, which is the same shape as the compact defect fixed in v2.10.5: the state that keeps the grant also destroys the mechanism that removes it. The cancellation moves into the first-registered cleanup, which reverse order runs last and which already gates on all three confirmations before releasing the registry row. That matches the rule revoke states outright when it leaves a task armed on a failed teardown: only once the account is provably gone is the fallback safe to remove. Nothing covered this ordering, which is why it survived. The new integration test drives a real invite whose expiry step fails after the task exists, with sudo removal failing during rollback, and asserts the task survives. It was mutation-checked: restoring the old registration makes it fail with "rollback cancelled the auto-revoke task while retaining an account that still holds a sudo drop-in". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
printInvite renders into a bytes.Buffer and relies on clear(out.Bytes()) to destroy the one-time key. clear reaches only the buffer's current backing array, and the render keeps writing after the private-key heredoc: the security note always, the sshd and permanent-account notes when they apply. Each write past capacity makes bytes.Buffer allocate a new array, copy, and orphan the old one — still holding the complete PEM, unreachable and unclearable, for the rest of the process's life. In menu mode that is until the operator quits, across later privileged actions, and into any swap or hibernation image. Growing once to a reservation far above any real invite keeps the key in a single array that the deferred clear actually reaches. The accompanying test is deliberately narrow and says so: Go offers no way to reach an orphaned backing array, so it cannot fail if the Grow call is removed. It bounds the thing that can rot instead — the render outgrowing its reservation as fields and notes are added — and was checked against a lowered reservation to confirm it fires. The widest real render is 1395 bytes against a 16 KiB reserve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
honorExistingQuarantine re-asserts the access gates when an old expiry task races a live quarantine. Removing the sudo grant and the sshd exception by name is safe against any target, because those files are this tool's own, but DisableLogin mutates the account itself and was issued against the username with no reference to the passwd snapshot loadAccount captured. Every other destructive-adjacent step in this file binds to that snapshot: teardownLocalAccountWith checks before and after its DisableLogin, beginIdentityQuarantine checks immediately after its own, and revalidateAndPrepareDeletion re-reads passwd and requires SameAccountIdentity before disabling anything, with the comment that spells out why — do not disable one generation and signal another. An account replaced out of band between loadAccount and this phase would have been disabled here and then reported as a successfully re-gated quarantine. The phase had no test at all, which is how the gap survived. It now has three covering the matching identity, a replaced one, and a vanished one, checked against the unguarded version to confirm the two refusals actually depend on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two boundary defects, both about state this tool creates but could then fail to account for. InspectIdentityAllocation took its floor from login.defs UID_MIN/GID_MIN and accepted anything validate.AccountID allows, i.e. 1 and up, while the deletion protection treats a UID below 1000 as system range: protected unless the registry row is present, identity-bound and marker-matched. On a host carrying the legacy 500 floor the tool would therefore mint accounts it can only delete while that row survives, and a lost row or a legacy-degraded one leaves the account permanently undeletable — the identical situation above the boundary still has explicit recovery. The floor is now one named constant shared by the allocator and both protection checks, so they cannot drift. Store.Init branched only on whether the registry data file existed. When absent it seeded the sequence with 0, which is a no-op against an existing sequence, wrote a bare v5 header, and satisfied requireIdentitySequenceCovering trivially against no rows. A host that lost only registry.tsv was therefore accepted as a fresh install, silently, while its identity sequence still recorded every allocation ever made. This package fails closed on the opposite asymmetry and ships recover-identity-sequence to repair it; this direction had nothing. Init deliberately does not fail closed. Doing so would take doctor and uninstall down with it, turning a recoverable state into an unrecoverable one, so it records the surviving high-water mark and doctor reports it. Only Init can observe the condition: once it recreates the file, an empty registry satisfies every sequence invariant. Both tests were mutation-checked against the unguarded code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
processSnapshotForUIDlet any vanishing numeric/procentry clear the whole snapshot'sstableflag, andprocessesForUIDneeds two consecutive stable empty snapshots before it will report a UID absent. Ordinary background activity on a busy host denied the scan; any local account could deny it deliberately with a plain fork loop.Both call sites are load-bearing:
user.go:947— the UID-reuse check after account creation, soinvitefailsuser.go:2083— the post-SIGKILL sweep inTerminateProcesses, so account deletion failsFix
A cheap first pass records each entry's directory owner while the listing is still fresh.
/proc/<pid>carries the process's real UID, and moving a thread to another UID needs privilege this threat model does not grant an unprivileged local user — so an entry owned by another unprivileged account can neither host nor fork a target-UID thread, and its disappearance says nothing about whether the target UID is absent.Root, the target UID, and entries that vanished before they could be attributed all stay conservative. Accepted residual, recorded in the comment: a multi-threaded process whose leader dropped to another UID while a worker still holds root is attributed to the leader.
Measured (four-core host, 30 scans per run)
An earlier attempt that instead spread the retries over a longer delay was measured and discarded: it made a moderate load worse (27/30), because the two confirming snapshots must stay close together to fall between disturbances. The tight retry is deliberate.
Diagnostics
The refusal now reports how many attempts were disturbed, so the operator can distinguish host churn from a fault in the account being revoked.
The at-queue bound gets the same treatment: its refusal names that the count spans every account's jobs and points at
atq, and its comment records why raising the bound would only trade a fast refusal for a slow timeout (each job costs one probe under a 30-second deadline, so 4096 already sits at what that deadline can complete), and that this cleanup runs only after revoke has stripped the grants and disabled the login — so a filled queue delays deletion and leaves the account retained and disabled, never privileged.Tests
New
TestProcessSnapshotAttributesInstabilityByOwnercovers third-party / root / target owners. It was mutation-checked: reverting the condition to the old logic makes it fail (stable = false, want true for owner 65534), so it is not a test that passes for the wrong reason.go vet -printf.funcs=printf,errorf,warnf ./...andgo test -race ./...pass.🤖 Generated with Claude Code