Skip to content

fix: give the release archive modes root will accept - #16

Merged
isaacslee merged 15 commits into
fix/security-reviewfrom
fix/archive-permissions
Oct 16, 2026
Merged

isaacslee merged 15 commits into
fix/security-reviewfrom
fix/archive-permissions

Conversation

@isaacslee

Copy link
Copy Markdown
Member

Why

This was found on the real ARM64 VM. Installing the snapshot archive exactly as docs/installation.md says (tar -xzf … -C /opt --no-same-owner, as root) produced a group-writable tree: files 775, root:root. runner-slice plan then refused to run, correctly: "not owned by root, or writable by others: /opt/runner-slice-0.1.0-dev …".

The cause is that git archive uses tar.umask 0002 by default. The release drill checked the archive's contents but never its modes.

Stacked on #15. The VM acceptance runs on this branch's commit.

What changed

  • scripts/release.sh builds with git -c tar.umask=0022 archive, which gives 0644 files and 0755 executables and directories.
  • tests/release-drill.sh fails on any group- or world-writable entry in the archive.

How it was verified

  • On the VM (real), before this change: after the documented install, preflight refused the tool tree, exited 1, and changed nothing.
  • Locally: the release drill passes, 10 of 10. It includes the new mode check, and the unpacked archive's own tests/run.sh.
  • On the VM after the reinstall: see the acceptance record. The tree must be root:root 755/644, and the plan must pass preflight.

GitHub-hosted CI: BLOCKED (see #3).

Evidence level

SYNTHETICALLY TESTED for the build. The defect itself was observed on a real host.

Limitations

The modes inside the archive change. That changes the archive's bytes, and so the checksum of the same commit's snapshot.

Risks

None.

Compatibility impact

None. No release exists.

Remaining work

None.

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
Copilot AI balanced review requested due to automatic review settings October 15, 2026 14:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
@isaacslee
isaacslee merged commit f24fff3 into fix/security-review Oct 16, 2026
1 check passed
@isaacslee
isaacslee deleted the fix/archive-permissions branch October 16, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants