Repository navigation
fix: close the gaps the pre-integration security review found - #15
Merged
Merged
Conversation
A root tool on a shared host must not run, unpack into or adopt what an unprivileged account controls, and must not leak a registration token; the review found each of these open. - The runner's version is read as data; the runner is never executed - A registered runner is never unpacked into; first unpacks start empty - Runtime roots, groups and homes are adopted only if RunnerSlice made them - Retirement checks every precondition first; the token stays off argv
What an operator types and what the threat model promises changed with the review fixes, so both say what the code now does. - The registration command passes the token in the environment - Runtime-root markers and all-or-nothing retirement are documented - New fixture assumptions to observe on the VM are listed
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
This was referenced Oct 15, 2026
Installed as documented, the archive unpacked group-writable, because git archive defaults to tar.umask 0002, and the real host's preflight then refused the tool tree; the drill never looked at modes. - The archive is built with tar.umask 0022: 0644 files, 0755 executables - The release drill fails on any group- or world-writable entry
On the ARM64 acceptance VM, systemd 255 printed RestrictNamespaces=yes for a restricted unit, where the check expected an empty value, so a correct host would have failed its own check. - baseline is checked as yes; userns is now checked as no - Drill fixtures print what systemd 255 was observed to print
The printed registration used --replace, which silently takes over any runner already registered under that name to the repository, possibly another host's; a first registration should refuse instead. - The registration command no longer passes --replace - It explains that config.sh refuses on a clash, and what --replace does
On the ARM64 VM, registration printed "ldd: ./bin/libcoreclr.so: No such file or directory" three times: config.sh checks its native libraries by ./bin paths before it changes directory, so run from elsewhere the check silently checked nothing. - Registration and deregistration run config.sh under env -C RUNNER_DIR - The printed note and the operations guide explain why
On the ARM64 VM the printed deregistration ran "config.sh remove --unattended", and the runner answered "Unrecognized command-line input arguments for command remove: 'unattended'": remove accepts only --token, --pat and --local. - The printed deregistration command no longer passes --unattended - The token still comes from the environment, which remove reads
The lifecycle has now run on one real host with real GitHub runners, so the status lines, the verified platforms and the acceptance table say what was shown there, at what level, and what still was not. - An acceptance record with commits, dogfood run ids and the five fixes - Each fixture assumption is marked as held, partly wrong or not observed - Ubuntu 24.04 aarch64 is listed as verified; x86_64 is still not
Retirement trusted pgrep, so a missing or failing pgrep read as "no process", and it removed the unit, then the root, then the account, so a userdel failure left a profile no run could finish. - read every uid in /proc/<pid>/status; an unreadable table refuses - remove the account first, then the root with its marker last, then the unit - read userdel's outcome back; remove only the account's own unused group - report a leftover home or group by name, and document each recovery
Preflight accepts systemd 249 and later, but only 255 has run the acceptance. Accepting a version read as supporting it. - preflight warns on any accepted systemd other than 255 - the documents separate supported (255) from accepted but unverified
O_NOFOLLOW covers only the last path component, so a symlinked bin/ redirected root's read of the dependency manifest. Parser errors such as deep nesting escaped as exit 1 instead of "cannot tell". - walk down from the runner directory with O_NOFOLLOW on every component - any exception, and a manifest of the wrong shape, exits 2
The printed commands exported the token into the operator's shell and relied on a later unset, and a failing gh still ran config.sh. - print registration and deregistration as one ( ... ) block - a gh failure ends the block before config.sh runs
tar gave the directory the archive's own mode for "." (0775) while it unpacked, so the comment that it stayed closed to others was not true. - unpack with --no-overwrite-dir, so the directory stays 0700 until chmod
Both guards existed with no apply-level case, so removing either passed every drill. - a person's account of the runner's name creates and downloads nothing - root's temporary directory ignores an inherited TMPDIR
The retirement and reader changes were run on the acceptance VM with a never-registered test profile, and the record must say what that covered and what stays synthetic. - add the follow-up table to the arm64 acceptance record - state in verification.md which #18 cases reached RUNTIME VERIFIED
…much An independent review of the retirement rework found that removing the root's children one by one let rm --one-file-system empty a filesystem mounted at work/ or runner/, and that some failures still read as success. - refuse, removing nothing, while anything is mounted in the runtime root or the mount table cannot be read; rm also runs with --preserve-root=all - write the root's marker back when the final rmdir fails - tell a failed account lookup from an absent account, keep a group when accounts also come from a directory service, report an undeletable unit
The re-review of the retirement fixes found three ways the new proofs could still read a host as safe when it was not. - parse "passwd:files sss" and trailing comments, and treat compat with "+" or "-" lines in /etc/passwd as a directory service - compare mounts against the root's canonical path, so a symlinked /var still finds a mount below it - cover escaped mount paths, prefix siblings and an empty table in drills
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
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.
Why
An independent read-only code and security review of the integrated tree, recorded in #14, found two high, two medium and five lower findings. This fixes all of them before any real-host run. Closes #14.
What changed
High
applyandcheckno longer runRunner.Listener --version. The version is read frombin/Runner.Listener.deps.json(Runner.Listener/<version>) as data..runneror.credentialsexists and the release is incomplete, apply reports a TODO and does not unpack.--no-same-owner, then chowns it to the account. Nothing in it was ever the runner's.Medium
export ACTIONS_RUNNER_INPUT_TOKEN=…andsudo --preserve-env=ACTIONS_RUNNER_INPUT_TOKEN …, never--tokenon argv, which sudo logs andpsshows..runner-slice-rootmarker naming its profile. An existing root without it stops the profile in the ownership step, before any account is created.useradd --user-group, so a group or home that predates the account is refused, not adopted.userdelfailures are reported, and a leftover group is reported, never deleted.Low / info
TMPDIRis unset in the root scripts.pwd -P. The ownership check covers the whole tool tree, includingbin/andVERSION.O_NOFOLLOW, regular files only, a size cap, and escaped output.runuserthat cannot run the test now fails the check instead of passing. Masks are recognised only by a mount root of/systemd/inaccessible/.rmdirs/etc/systemd/system;check --allruns preflight first;statusrunspython3 -I.Documentation (operations, threat model, architecture, verification, changelog) describes the new behaviour. Three new fixture assumptions are listed for the VM:
deps.json,ACTIONS_RUNNER_INPUT_TOKEN, and the mask root field.How it was verified
All of this was run locally on
6adadd3(tree89c1294):New failing-first cases:
--token;userdelis reported;runuserthat cannot run fails the boundary check;Mutation checks: reverting each of the 10 guards one at a time made the drills fail, 10 of 10.
The review's findings were also confirmed by reading the code before fixing them.
GitHub-hosted CI: BLOCKED. The organisation's $0 Actions budget is still in place, and the included minutes are used up (see #3).
Evidence level
SYNTHETICALLY TESTED. Several of these depend on real behaviour, and are on the VM list:
config.shhonouringACTIONS_RUNNER_INPUT_TOKEN;deps.jsonnaming the installed version;Limitations
The version check reads a runner-written manifest. A job can make it lie; it cannot make root execute anything.
Risks
Stricter refusals: an existing
/var/lib/<prefix>-<id>, or a same-named group or home, now stops apply. No installation exists yet.Compatibility impact
.runner-slice-root.Remaining work
Real-host verification on the ARM64 VM.