From 8e41961c7de081c3a097fc61c68f19ad4397cc5e Mon Sep 17 00:00:00 2001 From: acrogenesis Date: Sat, 29 Aug 2026 23:55:42 -0600 Subject: [PATCH] Close privileged cleanup review gaps --- migrations/1787946619.sh | 34 ++++++- migrations/1788025225.sh | 6 +- .../nested-parameter-default.sh | 6 ++ .../route-install-hop-literal.sh | 7 ++ .../route-prebody-escaped-pipeline.sh | 6 ++ .../legacy-power-udev-rules-migration-test.sh | 55 ++++++++++- test/shell.d/privileged-heredoc-test.sh | 93 +++++++++++++------ ...ired-installer-artifacts-migration-test.sh | 15 +++ 8 files changed, 186 insertions(+), 36 deletions(-) create mode 100644 test/shell.d/fixtures/privileged-heredoc/nested-parameter-default.sh create mode 100644 test/shell.d/fixtures/privileged-heredoc/route-install-hop-literal.sh create mode 100644 test/shell.d/fixtures/privileged-heredoc/route-prebody-escaped-pipeline.sh diff --git a/migrations/1787946619.sh b/migrations/1787946619.sh index d6a0705a..204712d9 100644 --- a/migrations/1787946619.sh +++ b/migrations/1787946619.sh @@ -1,6 +1,8 @@ echo "Remove Omarchy 3 power udev rules that run a command out of a user home" rules_dir="${OMARCHY_UDEV_RULES_DIR:-/etc/udev/rules.d}" +reload_needed_marker="${OMARCHY_UDEV_RELOAD_NEEDED_MARKER:-/var/lib/omarchy/migrations/1787946619-udev-reload-needed}" +udev_control="${OMARCHY_UDEV_CONTROL:-/run/udev/control}" as_root() { if (( EUID == 0 )); then @@ -78,19 +80,41 @@ rule_runs_from_home() { return 1 } +finish_pending_reload() { + # With no control socket there is no running udevd holding the deleted rule; + # the next daemon start reads the directory from disk. If a daemon is running, + # a failed reload must keep this migration pending so the in-memory root rule + # cannot outlive the per-user completion marker. + if [[ -e $udev_control ]] && ! as_root udevadm control --reload 2>/dev/null; then + echo "Could not reload udev after removing a vulnerable legacy rule. Ask an administrator to run omarchy-migrate." >&2 + exit 1 + fi + + if ! as_root rm -f "$reload_needed_marker"; then + echo "Could not finish the legacy udev-rule repair. Ask an administrator to run omarchy-migrate." >&2 + exit 1 + fi +} + +# Deleting the file and reloading the daemon are one repair. A prior run may +# have removed the file and then failed before udevd accepted the new ruleset. +if [[ -e $reload_needed_marker ]]; then + finish_pending_reload +fi + for legacy_rule in "99-power-profile.rules:omarchy-powerprofiles-set" "99-wifi-powersave.rules:omarchy-wifi-powersave"; do rule_file="$rules_dir/${legacy_rule%%:*}" if [[ -f $rule_file ]] && rule_runs_from_home "$rule_file" "${legacy_rule##*:}"; then + if ! as_root install -Dm644 /dev/null "$reload_needed_marker"; then + echo "Administrator privileges are required to remove the vulnerable legacy udev rule. Ask an administrator to run omarchy-migrate." >&2 + exit 1 + fi if ! as_root rm -f "$rule_file"; then echo "Administrator privileges are required to remove the vulnerable legacy udev rule. Ask an administrator to run omarchy-migrate." >&2 exit 1 fi - # Reload after each removal, not after the whole loop. If removing a later - # rule fails, udevd must not keep running one this migration already deleted. - # Best effort the way install/post-install/udev.sh is: a machine with no - # udevd to talk to has had the file removed, and the next boot reads fresh. - as_root udevadm control --reload 2>/dev/null || true + finish_pending_reload fi done diff --git a/migrations/1788025225.sh b/migrations/1788025225.sh index 31656ddc..96ba2f6c 100644 --- a/migrations/1788025225.sh +++ b/migrations/1788025225.sh @@ -116,8 +116,8 @@ sudoers_hash_is_active() { # 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 marker_pattern='^/bin/rm -f /home/[^/]+/\.local/state/omarchy/first-run\.mode$' - local line user command generated_user="" + local marker_pattern='^/bin/rm -f /home/([^/]+)/\.local/state/omarchy/first-run\.mode$' + local line user command marker_user generated_user="" local seen_any=0 seen_marker=0 seen_spec=0 while IFS= read -r line; do @@ -161,6 +161,8 @@ first_run_sudoers_is_generated() { esac if [[ $command =~ $marker_pattern ]]; then + marker_user=${BASH_REMATCH[1]} + [[ $marker_user == "$generated_user" ]] || return 1 seen_marker=1 continue fi diff --git a/test/shell.d/fixtures/privileged-heredoc/nested-parameter-default.sh b/test/shell.d/fixtures/privileged-heredoc/nested-parameter-default.sh new file mode 100644 index 00000000..7cb74bc0 --- /dev/null +++ b/test/shell.d/fixtures/privileged-heredoc/nested-parameter-default.sh @@ -0,0 +1,6 @@ +#!/bin/bash + +# omarchy:heredoc-expands paths=none -- review regression fixture +sudo tee /etc/omarchy/review.conf >/dev/null </tmp/omarchy-review-unit +[Service] +ExecStart=$HOME/.local/bin/payload +EOF +sudo install -m 644 /tmp/omarchy-review-unit /etc/systemd/system/review.service diff --git a/test/shell.d/fixtures/privileged-heredoc/route-prebody-escaped-pipeline.sh b/test/shell.d/fixtures/privileged-heredoc/route-prebody-escaped-pipeline.sh new file mode 100644 index 00000000..c03d436f --- /dev/null +++ b/test/shell.d/fixtures/privileged-heredoc/route-prebody-escaped-pipeline.sh @@ -0,0 +1,6 @@ +#!/bin/bash + +cat <"$test_dir/bin/udevadm" <<'STUB' #!/bin/bash printf 'udevadm %s\n' "$*" >>"$CALLS" +if [[ -n ${FAIL_UDEV_RELOAD_ONCE_MARKER:-} && ! -e $FAIL_UDEV_RELOAD_ONCE_MARKER ]]; then + touch "$FAIL_UDEV_RELOAD_ONCE_MARKER" + exit 1 +fi STUB chmod +x "$test_dir/bin/"* @@ -44,10 +48,13 @@ rules_dir="$test_dir/rules.d" home_dir="$test_dir/home" power_rule="$rules_dir/99-power-profile.rules" wifi_rule="$rules_dir/99-wifi-powersave.rules" +reload_needed_marker="$test_dir/reload-needed" +udev_control="$test_dir/udev-control" reset_machine() { - rm -rf "$rules_dir" "$home_dir" + rm -rf "$rules_dir" "$home_dir" "$reload_needed_marker" "$udev_control" mkdir -p "$rules_dir" "$home_dir" + touch "$udev_control" } run_migration() { @@ -55,6 +62,8 @@ run_migration() { HOME="$home_dir" \ OMARCHY_UDEV_RULES_DIR="$rules_dir" \ + OMARCHY_UDEV_RELOAD_NEEDED_MARKER="$reload_needed_marker" \ + OMARCHY_UDEV_CONTROL="$udev_control" \ PATH="$test_dir/bin:$PATH" \ bash -euo pipefail "$migration" >/dev/null } @@ -118,6 +127,45 @@ run_migration fail "migration reloads udev after each removal" "$(cat "$CALLS")" pass "migration removes both legacy rules and reloads after each one" +# Removing the file and reloading the running daemon are one repair. If reload +# fails, the durable marker must keep the migration pending even though the rule +# has already disappeared from disk; a retry finishes that half before exiting. +reset_machine +write_vulnerable_wifi_rule +reload_failure_seen="$test_dir/reload-failure-seen" +rm -f "$reload_failure_seen" + +set +e +FAIL_UDEV_RELOAD_ONCE_MARKER="$reload_failure_seen" run_migration +reload_status=$? +set -e + +(( reload_status != 0 )) || fail "migration fails when a running udevd cannot reload" +[[ ! -e $wifi_rule && -e $reload_needed_marker ]] || + fail "migration records a deleted rule whose daemon reload is still pending" +pass "migration keeps a failed udev reload pending" + +run_migration + +[[ ! -e $reload_needed_marker ]] || fail "migration clears the reload marker after a successful retry" +(( $(reload_count) == 1 )) || + fail "migration retries the pending udev reload" "$(cat "$CALLS")" +pass "migration retries and completes a previously failed udev reload" + +# A chroot or stopped daemon has no in-memory ruleset to update. An absent udev +# control socket is therefore a completed removal, not a permanent migration +# failure waiting for a daemon that is not running. +reset_machine +rm -f "$udev_control" +write_vulnerable_wifi_rule +run_migration + +[[ ! -e $wifi_rule && ! -e $reload_needed_marker ]] || + fail "migration completes the disk-only repair when udevd is not running" +(( $(reload_count) == 0 )) || + fail "migration does not contact an absent udevd" "$(cat "$CALLS")" +pass "migration permits environments with no running udev daemon" + # The second run is what every other account on the machine does, and what a # user gets from running omarchy-migrate again. run_migration @@ -219,6 +267,8 @@ write_vulnerable_wifi_rule set +e HOME="$home_dir" \ OMARCHY_UDEV_RULES_DIR="$rules_dir" \ + OMARCHY_UDEV_RELOAD_NEEDED_MARKER="$reload_needed_marker" \ + OMARCHY_UDEV_CONTROL="$udev_control" \ PATH="$test_dir/failing-bin:$PATH" \ bash -euo pipefail "$migration" >"$test_dir/elevation-failure.out" 2>&1 failure_status=$? @@ -250,6 +300,8 @@ chmod +x "$test_dir/failing-bin/sudo" set +e HOME="$home_dir" \ OMARCHY_UDEV_RULES_DIR="$rules_dir" \ + OMARCHY_UDEV_RELOAD_NEEDED_MARKER="$reload_needed_marker" \ + OMARCHY_UDEV_CONTROL="$udev_control" \ PATH="$test_dir/failing-bin:$test_dir/bin:$PATH" \ bash -euo pipefail "$migration" >"$test_dir/partial-failure.out" 2>&1 partial_status=$? @@ -257,6 +309,7 @@ set -e (( partial_status != 0 )) || fail "migration fails when the second rule cannot be removed" [[ ! -e $power_rule && -e $wifi_rule ]] || fail "migration preserves the expected partial-removal state" +[[ -e $reload_needed_marker ]] || fail "migration records the second rule removal as still pending" (( $(reload_count) == 1 )) || fail "migration reloads udev before a later removal failure" "$(cat "$CALLS")" pass "a later removal failure cannot leave an already-deleted rule loaded" diff --git a/test/shell.d/privileged-heredoc-test.sh b/test/shell.d/privileged-heredoc-test.sh index 9501113b..0caa4215 100755 --- a/test/shell.d/privileged-heredoc-test.sh +++ b/test/shell.d/privileged-heredoc-test.sh @@ -121,8 +121,8 @@ strip_escapes() { # path. Because both halves come from one pass over one pattern, the Nth \001 # is the Nth name, so a token can be judged against the right variable. mask_and_names() { - local text="$1" masked="" body name guard=0 - local -a names=() + local text="$1" masked="" body inner tail name guard=0 nested_masked + local -a names=() nested_scan=() nested_names=() # Normalize backtick substitution into $( ) so one pattern covers both. while ((guard++ < 64)) && [[ $text =~ ^([^\`]*)\`([^\`]*)\`(.*)$ ]]; do @@ -135,20 +135,35 @@ mask_and_names() { masked+="${BASH_REMATCH[1]}"$'\001' body=${BASH_REMATCH[2]} text=${BASH_REMATCH[4]} + nested_masked="" + nested_names=() if [[ $body == \(* ]]; then name=$COMMAND_SUBSTITUTION elif [[ $body == \{* ]]; then - body=${body:1:${#body}-2} + inner=${body:1:${#body}-2} # ${name}, ${name:-default}, ${name//a/b}, ${#name}, ${!name} all start # with the name once the decorations are stripped. - body=${body#[\#!]} - if [[ $body =~ ^([A-Za-z_][A-Za-z0-9_]*) ]]; then + inner=${inner#[\#!]} + if [[ $inner =~ ^([A-Za-z_][A-Za-z0-9_]*) ]]; then name=${BASH_REMATCH[1]} - elif [[ $body =~ ^[0-9@*#?$!-]$ ]]; then + tail=${inner#"$name"} + elif [[ $inner =~ ^[0-9@*#?$!-] ]]; then name="shell-parameter" + tail=${inner:1} else name=$COMMAND_SUBSTITUTION + tail=$inner + fi + + # The shell expands the operator payload too. Keep it as a synthetic + # adjacent token so its placeholders stay aligned with their names while + # the outer expansion remains independently classifiable. Without this, + # ${target:-$HOME/path} is consumed as only `target` and hides HOME. + if [[ $tail =~ $EXPANSION_RE || $tail == *'`'* ]]; then + mapfile -t nested_scan < <(mask_and_names "$tail") + nested_masked=${nested_scan[0]} + nested_names=("${nested_scan[@]:1}") fi else name=${body%%\[*} @@ -156,6 +171,10 @@ mask_and_names() { fi names+=("$name") + if ((${#nested_names[@]} > 0)); then + masked+=" $nested_masked" + names+=("${nested_names[@]}") + fi done printf '%s\n' "$masked$text" @@ -469,25 +488,24 @@ privileged_destination() { unresolved+=("$dest") fi - # One hop: a later copy of this same expression into a root-owned path. - if [[ $dest == *'$'* ]]; then - follow=$start_index - while ((follow < ${#scan_lines[@]})); do - hop=${scan_lines[follow]} - follow=$((follow + 1)) - [[ $hop =~ (^|[[:space:]])(install|cp|mv)([[:space:]]|$) ]] || continue - line_carries_destination "$hop" "$dest" || continue - while IFS= read -r hop_dest; do - [[ $hop_dest == $'\002elevated' ]] && continue - [[ $hop_dest == "$dest" ]] && continue - hop_dest=$(resolve_value "$hop_dest") - if starts_with_privileged_prefix "$hop_dest"; then - printf '%s' "$hop_dest" - return 0 - fi - done < <(command_destinations "$hop") - done - fi + # One hop: a later copy of this same destination into a root-owned path. + # Literal scratch files need tracing just as much as variable destinations. + follow=$start_index + while ((follow < ${#scan_lines[@]})); do + hop=${scan_lines[follow]} + follow=$((follow + 1)) + [[ $hop =~ (^|[[:space:]])(install|cp|mv)([[:space:]]|$) ]] || continue + line_carries_destination "$hop" "$dest" || continue + while IFS= read -r hop_dest; do + [[ $hop_dest == $'\002elevated' ]] && continue + [[ $hop_dest == "$dest" ]] && continue + hop_dest=$(resolve_value "$hop_dest") + if starts_with_privileged_prefix "$hop_dest"; then + printf '%s' "$hop_dest" + return 0 + fi + done < <(command_destinations "$hop") + done done < <(command_destinations "$line") # An elevated write whose destination cannot be resolved counts as privileged: @@ -572,7 +590,7 @@ inside_same_line_arithmetic() { 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 index lineno line command scan rest raw operator match prefix guard slot delim candidate candidate_delim body_start 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_]*)' @@ -588,9 +606,21 @@ scan_file() { [[ $line =~ ^[[:space:]]*# ]] && continue + # A backslash-escaped newline is removed before Bash parses the command, so + # a pipeline consumer can appear on the next physical line before heredoc + # body collection begins: `cat </ cleanup path. A different account in the path proves the +# line was edited or hand-written and makes the whole file administrator-owned. +reset_machine +cat >"$first_run" <<'EOF' +alice ALL=(ALL) NOPASSWD: /bin/rm -f /home/bob/.local/state/omarchy/first-run.mode +EOF +before=$(cat "$first_run") +run_migration + +[[ -e $first_run ]] || fail "migration keeps a first-run cleanup path for another account" +[[ $(cat "$first_run") == "$before" ]] || + fail "migration leaves the cross-account first-run file byte for byte" +pass "migration requires the cleanup path account to match the granted account" + # Nothing in this file ties it to Omarchy's first run: no self-cleanup line. reset_machine cat >"$first_run" <<'EOF'