Commit Graph
9 Commits
Author SHA1 Message Date
Ryan Hughes 58c399de30 Revert "Drop unrelated privileged heredoc scanner"
This reverts commit 4c23077f80.
2026-08-30 11:54:31 -04:00
Ryan Hughes 4c23077f80 Drop unrelated privileged heredoc scanner 2026-08-30 11:49:50 -04:00
David Heinemeier HanssonandClaude Opus 5 96404be37b Catch a privileged heredoc redirected with >|
`>|` is a plain redirect with noclobber overridden, not a redirect followed by a pipe. command_destinations detached `>` from its target before looking at the bar, so the target read as `|` and the privileged path behind it was never examined: `cat <<EOF >| /etc/udev/rules.d/99-x.rules` with `$HOME` in the body produced no finding at all, while the same write through `>` produced one.

Normalizing `>|` to `>` alongside the existing `>>` handling closes it. The fixture fails without the normalization.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-30 08:58:12 +02:00
acrogenesis 8e41961c7d Close privileged cleanup review gaps 2026-08-29 23:55:42 -06:00
acrogenesis f91d2e5453 Restore strict migration ordering 2026-08-29 22:42:33 -06:00
acrogenesis 4d697a063c Harden privileged cleanup review fixes 2026-08-29 21:08:33 -06:00
acrogenesis 4996941513 Address privileged cleanup review findings 2026-08-29 20:03:46 -06:00
acrogenesis 394c1371c9 Model what each parser does with an empty and a dangling directive
Review of the previous commits turned up four places where the predicates
and their tests disagreed with the tools they are modelling, each checked
against udevadm verify, systemd-analyze verify and visudo -cf rather than
against reading of the sources.

An empty ExecStop= resets the list, so a unit an administrator neutralised
that way runs nothing at shutdown and is no longer ours to remove; the
predicate now tracks the last state instead of returning on the first home
path it sees. A file whose last line ends in a backslash still carries a
live directive for systemd, so the pending logical line is emitted at EOF;
udev ignores such a line and sudo rejects the file outright, so this costs
those two nothing. The scanner's taint pass now reads += appends, which its
own comment already promised: the value of an append is no use, but a name
that reaches a user root through one has to be judged on it.

Two regression guards passed against the implementations they were written
for. The udev continuation fixture put the whole RUN+= below the comment, so
it matched whether or not the pending half was carried across; the split now
falls inside the RUN+= value. The sudoers one kept its file on the strength
of a spec above the comment, so it could not fail either; the hand-written
spec now sits below. Both fail against a mutant that discards the pending
line. The comment above the second also claimed a continued comment stays a
comment, which visudo contradicts.
2026-08-29 19:23:02 -06:00
acrogenesis d593847728 Fail the suite on privileged writes through unquoted heredocs
An installer that writes a root-owned file through a heredoc with an
unquoted delimiter (<<EOF rather than <<'EOF') has the installing user's
shell expand the body first, so a user-controlled value is baked in as a
literal. Send that into /etc and root later reads or executes a path the
unprivileged user picked: a udev rule carrying
$HOME/.local/share/omarchy/bin/... resolves through a symlink that user
owns, so replacing the symlink gets their code run as root.

Add the static check. A heredoc is flagged when its delimiter is
unquoted, its body contains an install-time expansion (escaped \$VAR does
not count, since that is left for a root daemon to expand at runtime),
and its output reaches /etc, /usr, /opt, /srv, /boot or /var/lib via sudo
tee, sudo dd, a redirect, or an install/cp/mv of the generated scratch
file. Destinations written as variables are resolved from the file's own
assignments.

Sites that genuinely need install-time expansion declare it inline:

  # omarchy:heredoc-expands paths=none -- $servers is a validated IP list

paths= is machine-checked against the expansions the scanner finds to be
path-shaped, so this cannot become a rubber stamp: adding a $HOME/... to
an already-annotated heredoc makes the declaration false and trips the
check again. Path expansions anchored under a root-owned prefix, as in
"/etc/systemd/system/$unit", are correctly not path-shaped.

Annotate the sites the scan reports, each of which expands a scalar: DNS
addresses in omarchy-dns, a literal PAM line in
omarchy-setup-security-fingerprint, kernel cmdline parameters and
usernames in omarchy-upgrade-to-quattro. omarchy-provision-owner expanded
a unit name that was already a constant, so its delimiter is now quoted
and the name hardcoded; the generated unit file is byte-identical.
omarchy-windows-vm declares paths=storage,shared, the only site that
interpolates a user-chosen path.

Fixtures prove non-vacuity in both directions: the write routes other
than a pipe into sudo tee, the shapes that must stay quiet, udev rules
and a shutdown unit taken verbatim from this repository's history, and
the rubber-stamp case where a paths=none annotation on a baked $HOME path
still fails.
2026-08-29 19:23:02 -06:00