Orphan sweep cross-checks recorded namespaces before SIGKILL (refs #285) - #294
Merged
Conversation
…#285) killOrphanInit fired on pid>1 and a starttime match alone. The starttime guard proves only that the pid was not RECYCLED since the state was written; it does NOT prove the process is a sandbox init. A state file whose init_pid/ init_starttime name any live same-uid process — a forged one, or a hostile one in a future where the uid-private state dir is exposed — was honoured, turning the sweep into an arbitrary-pid kill (measured in the redteam round: a forged target-<sha>.json naming a plain `sleep` in host namespaces got the sleep killed, though its real namespaces did not match the fabricated inodes and the named target did not exist). Not an escape today: the state dir is 0700, uid-private, and not reachable from inside the sandbox, and a same-uid host process can kill() directly. Hardening — closes the gap between "the sweep trusts the file" and "the sweep verifies the process is what the file claims", with data the file already carries and attach already trusts. Fix: after the starttime check, read /proc/<pid>/ns/<kind> for the six recorded namespace kinds (procNamespaceInodes, the helper attach already uses) and require every inode to equal st.Sandbox.Namespaces[k] before unix.Kill. Fail CLOSED: a read error or any mismatch means "not provably our init", and leaving an orphan is the less-bad outcome than killing an unconfirmed process. Test: TestSweepDoesNotKillAPidInForeignNamespaces — two victims differing only in whether their recorded namespaces match their real ones. The foreign one (fabricated inodes, correct pid+starttime+filename) survives; the own one (real inodes) is killed as the control, so the foreign survival is attributable to the namespace mismatch and not to an inert sweep. The stale file is still removed either way. Mutation-checked: neutering the comparison kills the foreign pid and fails the test. The existing kill fixtures now record the victim's real namespace inodes (via liveProcess) rather than {mnt:1,pid:2,...}, which the new cross-check would otherwise reject. make gate green (gofmt, vet, full unit suite). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Maintainer review on #294: this project already depends on modern-Linux features (userns, netns, seccomp, open_tree), so the sweep should pin the process with a pidfd rather than signal a bare pid number. pidfd_open pins the task: while the fd is held the kernel cannot recycle that pid number, so the identity checks (starttime, namespace inodes) and the kill all refer to the same task. A numeric-pid kill after a separate check carries a TOCTOU — the pid can be reaped and reused between the check and the SIGKILL; pidfd_send_signal on the held fd cannot. The starttime check still runs, because pidfd_open will pin a pid that was ALREADY reused before we opened it, so it proves the pinned pid is the one the state file named; the namespace check still runs, because starttime alone does not prove the process is a sandbox init. unix.PidfdOpen + unix.PidfdSendSignal (SIGKILL); ESRCH on open is the ordinary "already gone" case. Tests unchanged and green — the victim is a real host process the fixtures start, killed the same way — including the mutation check (neutering the namespace comparison still kills the foreign-ns victim and fails TestSweepDoesNotKillAPidInForeignNamespaces). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Aug 21, 2026
vyskocilm
added a commit
that referenced
this pull request
Aug 22, 2026
…efs #298) (#303) engine/reap.go signalOwned scanned /proc for processes whose command line names this run's socket, then syscall.Kill(pid) each by number. Foreign-pid case: between the scan and the kill the number can be reaped and handed to an unrelated process, which snug then SIGKILLs -- the reuse TOCTOU #294 removed from the orphan sweep, still live here. signalOwned now pins each discovered pid with unix.PidfdOpen, re-reads the cmdline through the pinned number to confirm it still names the engine, and only then unix.PidfdSendSignal. The pidfd pins the task the number named at open time, so the signal can never land on a later reuse; the re-read drops a number the scan matched but that no longer names us (recycled, or an engine that exited between scan and signal). Returns the pids actually signalled. The pids are HOST-namespace: ownedPIDs reads the host /proc and matches the socket path in a host cmdline, so pidfd_open (caller's namespace) refers to the same task. It never sees the engine's own pids, numbered in its own namespace (#167). cmdlineNamesPath factored out and shared with ownedPIDs; reading /proc/<pid>/cmdline for DATA by number stays -- the rule is about signalling. TestSignalOwnedReverifiesThroughAPidfdBeforeKilling: a live process not naming the socket, handed to signalPinned by pid, must survive (the reuse case); mutation-checked (dropping the re-verify kills it). Positive control: a process that names the socket is signalled and dies from SIGKILL. go-implementer.md gets the standing rule: pin with a pidfd to signal or track a process; a bare numeric pid is only safe for your own not-yet-waited child, and only ever safe to READ /proc for data. Namespace caveat (#167) noted. Own-child cmd.Process.Kill() sites left as-is.
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.
What
killOrphanInit(internal/cli/orphansweep.go) fired onpid>1+ starttime match alone. Starttime proves only the pid was not RECYCLED, not that it is a sandbox init. A state file whoseinit_pid/init_starttimename any live same-uid process is honoured — the sweep becomes an arbitrary-pid kill primitive (redteam round, #285: a forgedtarget-<sha>.jsonnaming a plainsleepin host namespaces got the sleep killed, though its real namespaces did not match the fabricated inodes and the named target did not exist).Not an escape: state dir is
0700, uid-private, unreachable from inside the sandbox; a same-uid host process cankill()directly. sev:low hardening — closes the gap between "trusts the file" and "verifies the process is what the file claims", using data the file already carries andattachalready trusts.Change
After the starttime check, read
/proc/<pid>/ns/<kind>for the six recorded namespace kinds (procNamespaceInodes, the helperattachalready uses) and require every inode to equalst.Sandbox.Namespaces[k]beforeunix.Kill. Fail CLOSED: a read error or any mismatch means "not provably our init", and leaving an orphan is the less-bad outcome than killing an unconfirmed process.Test
TestSweepDoesNotKillAPidInForeignNamespaces— two victims differing ONLY in whether recorded namespaces match real ones:Mutation-checked: neutering the comparison kills the foreign pid and fails the test. Existing kill fixtures now record the victim's real namespace inodes (via
liveProcess) rather than{mnt:1,pid:2,...}, which the new cross-check would otherwise reject.make gategreen (gofmt, vet, full unit suite).Not in scope
The general "forged file in the state dir" question — the dir is uid-private and unreachable from the sandbox, so no payload route exists today; this hardens the kill path regardless.