Harden privileged cleanup review fixes

This commit is contained in:
acrogenesis
2026-08-29 21:08:33 -06:00
parent 4996941513
commit 4d697a063c
13 changed files with 219 additions and 32 deletions
+1 -1
View File
@@ -126,7 +126,7 @@ New migration format:
- Start with an `echo` describing what the migration does.
- Use `$OMARCHY_PATH` to reference the Omarchy directory.
- Be idempotent. Check existing state before changing it.
- Exit 75 when a temporary condition must leave the migration pending without blocking later migrations. `omarchy-migrate` continues the queue, does not write that migration's completion marker, and retries it on a later run. Other non-zero statuses still abort the migration run.
- When a temporary condition must leave the migration pending without blocking later migrations, write `$OMARCHY_MIGRATION_DEFER_TOKEN` to `$OMARCHY_MIGRATION_DEFER_FILE`, then exit 75. `omarchy-migrate` requires both signals before it continues the queue without writing that migration's completion marker. An unmarked exit 75 is an ordinary failure and aborts the run, so a child command returning `EX_TEMPFAIL` cannot accidentally defer a migration forever.
- Use helper commands such as `omarchy-cmd-present`, `omarchy-cmd-missing`,
`omarchy-pkg-add`, `omarchy-pkg-drop`, `omarchy-pkg-present`, and
`omarchy-pkg-missing` when appropriate.
+12 -2
View File
@@ -91,18 +91,28 @@ while IFS=$'\t' read -r name file marker; do
if [[ ! -f $marker ]]; then
echo -e "\e[32m\nRunning migration (${name%.sh})\e[0m"
if OMARCHY_PATH="$OMARCHY_PATH" bash -euo pipefail "$file"; then
defer_file=$(mktemp "$STATE_DIR/.defer.XXXXXX")
defer_token="$BASHPID-$RANDOM-$RANDOM"
migration_status=0
if OMARCHY_PATH="$OMARCHY_PATH" \
OMARCHY_MIGRATION_DEFER_FILE="$defer_file" \
OMARCHY_MIGRATION_DEFER_TOKEN="$defer_token" \
bash -euo pipefail "$file"; then
mkdir -p "$(dirname "$marker")"
touch "$marker"
else
migration_status=$?
if (( migration_status == 75 )); then
if (( migration_status == 75 )) && [[ $(<"$defer_file") == "$defer_token" ]]; then
deferred+=("$name")
echo "Migration ${name%.sh} was deferred and will be retried later."
else
rm -f "$defer_file"
exit "$migration_status"
fi
fi
rm -f "$defer_file"
fi
done < <(migration_entries)
+2 -2
View File
@@ -788,7 +788,7 @@ configure_login() {
# After=) is what makes it deterministic — no sleep/race against SDDM's startup.
install_autologin_once_cleanup() {
local unit=omarchy-provision-autologin-once.service
cat >"/etc/systemd/system/$unit" <<'UNIT'
sed "s|@UNIT@|$unit|g" >"/etc/systemd/system/$unit" <<'UNIT'
[Unit]
Description=Drop the first-boot autologin before the next login
Before=display-manager.service
@@ -797,7 +797,7 @@ ConditionPathExists=/etc/sddm.conf.d/autologin.conf
[Service]
Type=oneshot
ExecStart=/usr/bin/rm -f /etc/sddm.conf.d/autologin.conf
ExecStartPost=/usr/bin/rm -f /etc/systemd/system/graphical.target.wants/omarchy-provision-autologin-once.service /etc/systemd/system/omarchy-provision-autologin-once.service
ExecStartPost=/usr/bin/rm -f /etc/systemd/system/graphical.target.wants/@UNIT@ /etc/systemd/system/@UNIT@
[Install]
WantedBy=graphical.target
+15 -5
View File
@@ -10,9 +10,17 @@ as_root() {
fi
}
defer_privileged_repair() {
echo "Cannot remove the legacy privileged udev rule; omarchy-migrate will retry it later." >&2
if [[ -n ${OMARCHY_MIGRATION_DEFER_FILE:-} && -n ${OMARCHY_MIGRATION_DEFER_TOKEN:-} ]]; then
printf '%s\n' "$OMARCHY_MIGRATION_DEFER_TOKEN" >"$OMARCHY_MIGRATION_DEFER_FILE"
fi
exit 75
}
# Omarchy 3 generated these two rules with an unquoted heredoc, so the installing
# user's $HOME was expanded and the file on disk names
# /home/<user>/.local/share/omarchy/bin/<command>. udev runs RUN+= as root, and
# user's $HOME was expanded and the file on disk names that absolute home path.
# udev runs RUN+= as root, and
# ~/.local/share/omarchy is a symlink that same unprivileged user owns: replacing
# it with a tree of their own and provoking a power_supply event runs their code
# as root. Quattro ships the rules as 99-omarchy-*.rules under /usr/bin, but the
@@ -32,7 +40,7 @@ as_root() {
# path in a comment, and so does a legacy file already repointed at /usr/bin.
rule_runs_from_home() {
local file="$1" binary="$2"
local pattern="^(/home/[^/]+|/root)/\\.local/share/omarchy/bin/$binary\$"
local pattern="^/.+/\\.local/share/omarchy/bin/$binary\$"
local line logical="" rest command word
local -a words
@@ -68,7 +76,7 @@ rule_runs_from_home() {
# argument. Compare whole words so no substring stands in for the path.
read -ra words <<<"$command"
for word in "${words[@]}"; do
if [[ $word =~ $pattern || $word == "$HOME/.local/share/omarchy/bin/$binary" ]]; then
if [[ $word =~ $pattern ]]; then
return 0
fi
done
@@ -84,7 +92,9 @@ for legacy_rule in "99-power-profile.rules:omarchy-powerprofiles-set" "99-wifi-p
rule_file="$rules_dir/${legacy_rule%%:*}"
if [[ -f $rule_file ]] && rule_runs_from_home "$rule_file" "${legacy_rule##*:}"; then
as_root rm -f "$rule_file"
if ! as_root rm -f "$rule_file"; then
defer_privileged_repair
fi
removed=1
fi
done
+16 -7
View File
@@ -115,10 +115,10 @@ sudoers_hash_is_active() {
# one line that is unmistakably this grant: its own self-cleanup. One
# hand-written line anywhere in the file and it is not ours to delete.
first_run_sudoers_is_generated() {
local spec_pattern='^[^[:space:]]+ ALL=\(ALL\) NOPASSWD: (.+)$'
local spec_pattern='^([^[:space:]]+) ALL=\(ALL\) NOPASSWD: (.+)$'
local marker_pattern='^/bin/rm -f /home/[^/]+/\.local/state/omarchy/first-run\.mode$'
local line command
local seen_any=0 seen_marker=0
local line user command generated_user=""
local seen_any=0 seen_marker=0 seen_spec=0
while IFS= read -r line; do
seen_any=1
@@ -140,7 +140,13 @@ first_run_sudoers_is_generated() {
if [[ ! $line =~ $spec_pattern ]]; then
return 1
fi
command=${BASH_REMATCH[1]}
user=${BASH_REMATCH[1]}
command=${BASH_REMATCH[2]}
if [[ -n $generated_user && $user != "$generated_user" ]]; then
return 1
fi
generated_user=$user
seen_spec=1
case "$command" in
"/usr/bin/systemctl" | "/usr/bin/ufw" | "/usr/bin/ufw-docker" | \
@@ -162,7 +168,7 @@ first_run_sudoers_is_generated() {
return 1
done < <(active_lines sudoers)
(( seen_any && seen_marker ))
(( seen_any && seen_marker && seen_spec ))
}
# bin/omarchy-install-tailscale (2025-08-22 to 2026-02-02) ran
@@ -202,7 +208,7 @@ tsui_sudoers_is_generated() {
plymouth_unit_runs_from_home() {
local binary="omarchy-plymouth-shutdown-sync"
local exec_stop_pattern='^ExecStop[[:space:]]*=[[:space:]]*(.*)$'
local home_pattern="^(/home/[^/]+|/root)/\\.local/share/omarchy/bin/$binary\$"
local home_pattern="^/.+/\\.local/share/omarchy/bin/$binary\$"
local line word
local -a words
local matched=1
@@ -229,7 +235,7 @@ plymouth_unit_runs_from_home() {
word=${word:1}
done
if [[ $word =~ $home_pattern || $word == "$HOME/.local/share/omarchy/bin/$binary" ]]; then
if [[ $word =~ $home_pattern ]]; then
# Non-empty ExecStop= assignments append to the command list. Once a
# vulnerable command is present it stays live until an empty assignment
# explicitly resets the list; a later packaged command does not replace it.
@@ -249,6 +255,9 @@ tsui_sudoers="$sudoers_dir/tsui"
defer_privileged_repair() {
echo "Cannot complete the privileged installer-artifact repair; omarchy-migrate will retry it later." >&2
if [[ -n ${OMARCHY_MIGRATION_DEFER_FILE:-} && -n ${OMARCHY_MIGRATION_DEFER_TOKEN:-} ]]; then
printf '%s\n' "$OMARCHY_MIGRATION_DEFER_TOKEN" >"$OMARCHY_MIGRATION_DEFER_FILE"
fi
exit 75
}
@@ -0,0 +1,5 @@
# omarchy:heredoc-expands paths=none -- the positional argument is a scalar
sudo tee /etc/omarchy/example.conf <<EOF
argument=$1
command=$HOME/.local/share/omarchy/bin/example
EOF
@@ -0,0 +1,4 @@
cat <<EOF |
command=$HOME/.local/share/omarchy/bin/example
EOF
sudo tee /etc/omarchy/example.conf
@@ -0,0 +1,6 @@
tmp=/tmp/omarchy-generated
copy=$tmp
cat >"$tmp" <<EOF
command=$HOME/.local/share/omarchy/bin/example
EOF
sudo install -m644 "$copy" /etc/omarchy/example.conf
@@ -0,0 +1,5 @@
tmp=/tmp/omarchy-generated
cat >"$tmp" <<EOF
command=$HOME/.local/share/omarchy/bin/example
EOF
sudo install -m644 "${tmp}" /etc/omarchy/example.conf
@@ -29,6 +29,15 @@ STUB
chmod +x "$test_dir/bin/"*
mkdir -p "$test_dir/failing-bin"
cat >"$test_dir/failing-bin/sudo" <<'STUB'
#!/bin/bash
echo "sudo: a terminal is required to read the password" >&2
exit 1
STUB
chmod +x "$test_dir/failing-bin/sudo"
export CALLS="$test_dir/calls"
rules_dir="$test_dir/rules.d"
@@ -189,17 +198,41 @@ run_migration
fail "migration keeps a legacy filename already repointed at /usr/bin"
pass "migration keeps a legacy filename already repointed at /usr/bin"
# Homes are not all under /home, so the running user's own home counts too, and
# the argument the later variants passed must not hide the path.
# Homes are not all under /home, and a different account may run this
# machine-wide repair after the installer account has gone away.
reset_machine
cat >"$wifi_rule" <<RULE
SUBSYSTEM=="power_supply", ATTR{type}=="Mains", ATTR{online}=="0", RUN+="/usr/bin/systemd-run --no-block --collect --unit=omarchy-wifi-powersave-on $home_dir/.local/share/omarchy/bin/omarchy-wifi-powersave on"
SUBSYSTEM=="power_supply", ATTR{type}=="Mains", ATTR{online}=="0", RUN+="/usr/bin/systemd-run --no-block --collect --unit=omarchy-wifi-powersave-on /srv/retired-installer/.local/share/omarchy/bin/omarchy-wifi-powersave on"
RULE
run_migration
[[ ! -e $wifi_rule ]] ||
fail "migration removes a rule that runs out of a home outside /home" "$(cat "$wifi_rule")"
pass "migration removes a rule that runs out of a home outside /home"
fail "migration removes another user's rule rooted outside /home" "$(cat "$wifi_rule")"
pass "migration removes another user's rule rooted outside /home"
# A user who cannot elevate must leave this repair pending without preventing
# later migrations from running. Once another account removes the machine-wide
# file, the next retry can complete without sudo.
reset_machine
write_vulnerable_wifi_rule
defer_file="$test_dir/defer-signal"
defer_token="legacy-udev-repair"
: >"$defer_file"
set +e
HOME="$home_dir" \
OMARCHY_UDEV_RULES_DIR="$rules_dir" \
OMARCHY_MIGRATION_DEFER_FILE="$defer_file" \
OMARCHY_MIGRATION_DEFER_TOKEN="$defer_token" \
PATH="$test_dir/failing-bin:$PATH" \
bash -euo pipefail "$migration" >"$test_dir/defer.out" 2>&1
defer_status=$?
set -e
(( defer_status == 75 )) || fail "migration defers when sudo cannot remove a vulnerable rule" "status=$defer_status"
[[ -e $wifi_rule ]] || fail "migration keeps the vulnerable rule when its elevated removal fails"
[[ $(<"$defer_file") == "$defer_token" ]] || fail "migration authenticates its deferral to the runner"
pass "migration defers instead of blocking the queue when removal cannot elevate"
# Nothing named the wrong binary is ours: the same path with a different command
# is a rule this migration cannot claim to know anything about.
+30
View File
@@ -83,6 +83,7 @@ mkdir -p "$deferred_root/migrations" "$deferred_home"
cat >"$deferred_root/migrations/100-deferred.sh" <<'SH'
echo deferred >>"$TEST_CALLS"
printf '%s\n' "$OMARCHY_MIGRATION_DEFER_TOKEN" >"$OMARCHY_MIGRATION_DEFER_FILE"
exit 75
SH
cat >"$deferred_root/migrations/200-after.sh" <<'SH'
@@ -111,3 +112,32 @@ grep -q '^100-deferred\.sh$' "$test_tmp/deferred-pending.out" ||
! grep -q '^200-after\.sh$' "$test_tmp/deferred-pending.out" ||
fail "migration runner does not report the completed later migration as pending"
pass "migration runner reports only the deferred migration as pending"
raw_75_root="$test_tmp/raw-75-omarchy"
raw_75_home="$test_tmp/raw-75-home"
raw_75_calls="$test_tmp/raw-75-calls"
mkdir -p "$raw_75_root/migrations" "$raw_75_home"
cat >"$raw_75_root/migrations/100-child-tempfail.sh" <<'SH'
echo child-tempfail >>"$TEST_CALLS"
bash -c 'exit 75'
SH
cat >"$raw_75_root/migrations/200-after.sh" <<'SH'
echo after-tempfail >>"$TEST_CALLS"
SH
set +e
HOME="$raw_75_home" \
OMARCHY_PATH="$raw_75_root" \
TEST_CALLS="$raw_75_calls" \
"$ROOT/bin/omarchy-migrate" >"$test_tmp/raw-75.out" 2>"$test_tmp/raw-75.err"
raw_75_status=$?
set -e
(( raw_75_status == 75 )) ||
fail "migration runner preserves an unmarked child exit 75" "status=$raw_75_status"
grep -q '^child-tempfail$' "$raw_75_calls" || fail "migration runner starts the exit-75 child"
! grep -q '^after-tempfail$' "$raw_75_calls" || fail "migration runner stops after an unmarked exit 75"
[[ ! -f $raw_75_home/.local/state/omarchy/migrations/100-child-tempfail.sh ]] ||
fail "migration runner leaves an unmarked exit-75 migration incomplete"
pass "migration runner does not mistake a child EX_TEMPFAIL for intentional deferral"
+66 -6
View File
@@ -50,14 +50,14 @@ USER_WRITABLE_VARS=(HOME PWD OLDPWD TMPDIR OMARCHY_PATH OMARCHY_INSTALL
WRITE_COMMANDS=(tee dd install cp mv)
ELEVATORS=(sudo as_root pkexec doas run0)
# A dollar the installing user's shell would act on: $name, ${name} or $(cmd).
# A dollar the installing user's shell would act on: $name, ${name}, $1, or $(cmd).
# Kept in a variable because an unquoted `(` inside a bracket expression is a
# syntax error in [[ =~ ]].
EXPANSION_RE='\$[A-Za-z_{(]'
EXPANSION_RE='\$[A-Za-z_{(0-9@*#?$!-]'
# One pattern for every expansion form, shared by masking and name extraction
# so the two stay in lockstep.
EXPANSION_SCAN_RE='^([^$]*)\$(\{[^}]*\}|\([^)]*\)|[A-Za-z_][A-Za-z0-9_]*(\[[^]]*\])?)(.*)$'
EXPANSION_SCAN_RE='^([^$]*)\$(\{[^}]*\}|\([^)]*\)|[A-Za-z_][A-Za-z0-9_]*(\[[^]]*\])?|[0-9@*#?$!-])(.*)$'
# Stand-in name for a command substitution, which has no variable to report.
COMMAND_SUBSTITUTION="command-substitution"
@@ -145,11 +145,14 @@ mask_and_names() {
body=${body#[\#!]}
if [[ $body =~ ^([A-Za-z_][A-Za-z0-9_]*) ]]; then
name=${BASH_REMATCH[1]}
elif [[ $body =~ ^[0-9@*#?$!-]$ ]]; then
name="shell-parameter"
else
name=$COMMAND_SUBSTITUTION
fi
else
name=${body%%\[*}
[[ $name =~ ^[A-Za-z_] ]] || name="shell-parameter"
fi
names+=("$name")
@@ -418,6 +421,28 @@ command_destinations() {
fi
}
# Does LINE carry the same resolved value as DEST? Compare resolved tokens rather
# than source spelling so $tmp, ${tmp}, and an alias assigned from either form
# all identify the same scratch file.
line_carries_destination() {
local line="$1" dest="$2" resolved token candidate
local -a tokens=()
resolved=$(resolve_value "$dest")
line=${line//\"/ }
line=${line//\'/ }
read -r -a tokens <<<"$line"
for token in "${tokens[@]}"; do
token=${token#[<>]}
token=${token%;}
candidate=$(resolve_value "$token")
[[ $candidate == "$resolved" ]] && return 0
done
return 1
}
# Does the heredoc on this line reach a root-owned file? Either directly, or in
# one hop: written to a scratch file that a later install/cp/mv carries into a
# privileged directory.
@@ -450,7 +475,7 @@ privileged_destination() {
while ((follow < ${#scan_lines[@]})); do
hop=${scan_lines[follow]}
follow=$((follow + 1))
[[ $hop == *"$dest"* ]] || continue
line_carries_destination "$hop" "$dest" || continue
[[ $hop =~ (^|[[:space:]])(install|cp|mv)([[:space:]]|$) ]] || continue
while IFS= read -r hop_dest; do
[[ $hop_dest == $'\002elevated' ]] && continue
@@ -475,6 +500,31 @@ privileged_destination() {
return 1
}
# A pipeline may put the command consuming a heredoc after its terminator:
#
# cat <<EOF |
# body
# EOF
# sudo tee /etc/file
#
# Join only while the command is syntactically continued, leaving unrelated
# commands below the heredoc to be scanned independently.
continued_heredoc_command() {
local command="$1" next="$2"
local -n source_lines="$3"
while [[ $command =~ (\|\||&&|\|)[[:space:]]*$ ]] && ((next < ${#source_lines[@]})); do
while ((next < ${#source_lines[@]})) && [[ ${source_lines[next]} =~ ^[[:space:]]*(#.*)?$ ]]; do
next=$((next + 1))
done
((next < ${#source_lines[@]})) || break
command+=" ${source_lines[next]}"
next=$((next + 1))
done
printf '%s' "$command"
}
# Count the \001 placeholders in a masked token.
count_placeholders() {
local text="$1" count=0
@@ -523,7 +573,7 @@ scan_file() {
local file="$1" display="${2:-$1}"
local -a lines=()
local index lineno line scan rest raw operator match prefix guard slot delim candidate candidate_delim body_start
local body_text unescaped destination body_line masked_line token name
local body_text unescaped destination destination_command body_line masked_line token name
local declared_paths annotation look shown_paths shown_plain count next slots terminated
local hd_re='(<<-?)[[:space:]]*("[A-Za-z_][A-Za-z0-9_]*"|'"'"'[A-Za-z_][A-Za-z0-9_]*'"'"'|[A-Za-z_][A-Za-z0-9_]*)'
@@ -611,7 +661,8 @@ scan_file() {
unescaped=$(strip_escapes "$body_text")
[[ $unescaped =~ $EXPANSION_RE || $unescaped == *'`'* ]] || continue
destination=$(privileged_destination "$line" "$index" lines) || continue
destination_command=$(continued_heredoc_command "$line" "$index" lines)
destination=$(privileged_destination "$destination_command" "$index" lines) || continue
# Sort the expansions into the ones that bake a path into the file and
# the ones that only interpolate a scalar.
@@ -802,6 +853,9 @@ fixture_flags shutdown-unit-home-execstop.sh \
fixture_flags annotated-paths-none-still-fails.sh \
"an annotation claiming paths=none cannot silence a baked \$HOME path" \
"declares paths=none but the path-shaped expansions are HOME"
fixture_flags annotated-special-parameter-before-home.sh \
"a shell special parameter cannot hide a later baked \$HOME path" \
"declares paths=none but the path-shaped expansions are HOME"
# A path can hide one or more hops away from the heredoc. In each of these the
# token in the body has no slash and the value never resolves to a literal path,
@@ -824,6 +878,12 @@ fixture_flags route-variable-path.sh \
"flags an elevated write whose destination is a variable resolving under /etc"
fixture_flags route-install-hop.sh \
"flags a scratch file that install(1) later copies into /usr"
fixture_flags route-install-hop-braced.sh \
"flags a scratch-file hop whose variable uses braces at the privileged copy"
fixture_flags route-install-hop-alias.sh \
"flags a scratch-file hop carried through an alias variable"
fixture_flags route-continued-pipeline.sh \
"flags a privileged pipeline command continued after the heredoc terminator"
fixture_flags route-dash-delimiter.sh "flags an indented <<- heredoc"
fixture_flags route-append-redirect.sh "flags an append redirect into /etc"
fixture_flags arithmetic-left-shift-before-heredoc.sh \
@@ -207,6 +207,19 @@ run_migration
assert_changed_nothing "migration changes nothing for a hand-written first-run file"
pass "migration keeps a first-run file carrying a hand-written rule"
# The installer wrote one account name consistently. A whitelisted command for
# another user is an administrator extension, not part of the generated body.
reset_machine
printf '%s\n' "${first_run_variants[-1]}" >"$first_run"
printf '%%wheel ALL=(ALL) NOPASSWD: /usr/bin/systemctl\n' >>"$first_run"
before=$(cat "$first_run")
run_migration
[[ -e $first_run ]] || fail "migration keeps a generated file extended for another sudoers user"
[[ $(cat "$first_run") == "$before" ]] ||
fail "migration leaves a generated file extended for another user byte for byte"
pass "migration does not delete an administrator grant that uses a generated command"
# Nothing in this file ties it to Omarchy's first run: no self-cleanup line.
reset_machine
cat >"$first_run" <<'EOF'
@@ -301,14 +314,16 @@ reload_at=$(grep -n '^systemctl daemon-reload$' "$CALLS" | cut -d: -f1)
fail "migration disables before removing and reloads last" "$(cat "$CALLS")"
pass "migration disables the unit, removes it, then reloads systemd in that order"
# Homes are not all under /home.
# Homes are not all under /home, and the account running this machine-wide
# repair may not be the account that installed the unit.
reset_machine
write_plymouth_unit "$home_dir/.local/share/omarchy/bin/omarchy-plymouth-shutdown-sync"
write_plymouth_unit "/srv/retired-installer/.local/share/omarchy/bin/omarchy-plymouth-shutdown-sync"
run_migration
[[ ! -e $plymouth_unit ]] ||
fail "migration removes a shutdown unit rooted in a home outside /home"
pass "migration removes a shutdown unit rooted in a home outside /home"
fail "migration removes another user's shutdown unit rooted outside /home"
[[ -e $machine_marker ]] || fail "migration marks the cross-user Plymouth repair complete"
pass "migration removes another user's shutdown unit rooted outside /home"
reset_machine
write_plymouth_unit "/usr/bin/omarchy-plymouth-shutdown-sync"