From 19e652dc30ceb32b194698e18e23fc435028bd20 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sun, 16 Aug 2026 23:38:38 +0200 Subject: [PATCH] Close three fail-open paths in the prefix trace harness All three were found by review and reproduced before fixing. Each let a run report success while proving nothing -- the shape this harness exists to prevent. A reused prefix defeated dropped-write detection entirely. Run 1's files were still there when run 2's manifest was taken, so the manifest reported writes that never happened and the fixture pair diffed CLEAN. The doc's own examples demonstrated that workflow. A non-empty prefix is now refused -- not deleted, since the path is user-supplied -- and the examples use distinct prefixes. A missing bwrap was swallowed. trace_prefix_bwrap_argv returned 1, but the caller reads it through `mapfile < <(...)` and a process substitution's status is discarded, so runner ended up empty and the run continued with no sandbox and no uid 0. Same swallow as the grep-status one fixed earlier, one layer up. bwrap is now probed in validate_requirements, before anything runs. The absolute-path scan only fired when no --bind-abs was given at all, so binding one path and missing three produced no warning. It now runs whenever --prefix is given and reports only what is not already covered. Fixing that surfaced a second defect: --bind-abs on a FILE aborted with "Can't mkdir /etc/profile: Not a directory", and binding its parent reintroduced the wholesale /etc shadowing this change had already measured as harmful ($SUDO_USER collapsing to /home//, the real /etc/profile unreadable). Files are now bound file-over-file, seeded with a copy so a script that greps before appending sees real content. Also measured rather than argued: the XDG redirect. A new fixture run with a real XDG_DATA_HOME pointing at a decoy shows all four writes landing in the prefix and the decoy empty -- the rsync --delete data-loss path, proven closed without running install_leechblock.sh. And no-prefix output was diffed against cdf2a55 from a detached worktree: byte-identical, so existing baselines hold. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011w2Pj4Ek3ZNMQVYcFHgXDY --- docs/shell-split-verification.md | 17 +++++-- .../trace-prefix-writes-2026-08-16.json | 27 +++++++++++- meta/scripts/fixtures/xdg_destinations.sh | 30 +++++++++++++ meta/scripts/lib/trace_prefix.sh | 34 ++++++++++++-- meta/scripts/trace_shell_split.sh | 44 +++++++++++++++++-- 5 files changed, 140 insertions(+), 12 deletions(-) create mode 100755 meta/scripts/fixtures/xdg_destinations.sh diff --git a/docs/shell-split-verification.md b/docs/shell-split-verification.md index 8310db16..0ea01cd5 100644 --- a/docs/shell-split-verification.md +++ b/docs/shell-split-verification.md @@ -102,13 +102,19 @@ resolves that in two layers — see `meta/scripts/lib/trace_prefix.sh`: ```bash # $HOME-rooted destinations: no bwrap, no root needed -meta/scripts/trace_shell_split.sh install_usage_monitoring.sh --prefix /tmp/pfx +meta/scripts/trace_shell_split.sh install_usage_monitoring.sh --prefix /tmp/pfx-before # hardcoded absolutes: bind each LEAF directory -meta/scripts/trace_shell_split.sh nvidia_troubleshoot.sh --prefix /tmp/pfx \ +meta/scripts/trace_shell_split.sh nvidia_troubleshoot.sh --prefix /tmp/pfx-nv \ --bind-abs /etc/modprobe.d --bind-abs /etc/X11 --stub git,lsmod,nvidia-smi ``` +**Every run needs its own empty prefix**, hence the `-before`/`-after` naming. +Reusing one leaves the first run's files in place, so the second run's manifest +reports writes it never made and a dropped write stops showing up entirely — +measured: the fixture pair below diffs _clean_ on a reused prefix. The harness +refuses a non-empty prefix rather than deleting a path you named. + `HOME` **and every `XDG_*` base** are redirected. Exporting the XDG vars explicitly is not belt-and-braces: `install_leechblock.sh` reads `XDG_DATA_HOME` directly and then runs `rsync -a --delete` at it, so inheriting @@ -129,7 +135,11 @@ Four traps, each of which cost a debugging round: to `/home//pyroveil`, `/etc/profile` became "No such file or directory", and a nested `/etc/X11` bind was masked so its write vanished from the manifest. All three look like a broken split. The scanner therefore never emits a bare - `/etc`. + `/etc`. A `--bind-abs` naming a **file** (`/etc/profile`) is bound + file-over-file, seeded with a copy of the original — bwrap cannot mount a + directory over a file, and binding the file's parent would be the wholesale + `/etc` bind again. The copy matters because a script that greps a config + before appending to it would otherwise read an empty file and branch wrong. - **`require_root` truncates a stubbed run to nothing.** `lib/common.sh` does `exec sudo "$0" "$@"`; under the `sudo` stub that records one line and exits 0 — a three-line trace that diffs clean against _any_ split. `--bind-abs` @@ -170,6 +180,7 @@ harness change can be checked instead of trusted: | `circular_nameref.sh` | exit 0, correct stdout, warning only in stderr | | `valued_stub_branch.sh` | the silent-stub trap (`--stub du` vs `du=4096`) | | `dropped_write_{before,after}.sh` | a split that silently loses one write | +| `xdg_destinations.sh` | writes addressed via `XDG_*`, not `$HOME` | A lint fix once broke one of these invisibly: adding a trailing `echo` to satisfy SC2034 made _that_ echo the function's last statement, so the fixture diff --git a/docs/superpowers/evidence/trace-prefix-writes-2026-08-16.json b/docs/superpowers/evidence/trace-prefix-writes-2026-08-16.json index 1fd7daf2..675aad24 100644 --- a/docs/superpowers/evidence/trace-prefix-writes-2026-08-16.json +++ b/docs/superpowers/evidence/trace-prefix-writes-2026-08-16.json @@ -56,7 +56,32 @@ { "command": "wc -l on changed files", "result": "pass", - "evidence": "trace_shell_split.sh 197, lib/trace_prefix.sh 166, lib/trace_stubs.sh 71, docs/shell-split-verification.md 230 - all under the 250-line cap." + "evidence": "All changed files under the 250-line cap; stub generation was moved to lib/ specifically because --prefix pushed the harness to 252." + }, + { + "command": "REVIEW FOLLOW-UP: reused prefix across a before/after pair", + "result": "pass", + "evidence": "Found by review and reproduced: with one prefix reused, run 1's files remain and the dropped-write fixture pair diffed CLEAN - the manifest reported a write that did not happen. The harness now refuses a non-empty prefix (it does not rm -rf a user-supplied path) and the doc's examples use distinct -before/-after prefixes. Re-verified: reuse exits 1; separate prefixes still detect the drop." + }, + { + "command": "REVIEW FOLLOW-UP: bwrap missing swallowed by mapfile", + "result": "pass", + "evidence": "trace_prefix_bwrap_argv returned 1, but a process substitution's status is discarded, so mapfile saw 0, runner was empty, and the run proceeded UNSANDBOXED as uid 1000 - the same fail-open shape as the grep-status swallow fixed earlier. bwrap is now probed in validate_requirements before any run. Verified with a minimal PATH lacking bwrap: exit 1, clear message, prefix never created." + }, + { + "command": "REVIEW FOLLOW-UP: no-prefix output vs the pre-change commit cdf2a55", + "result": "pass", + "evidence": "Traced disk_cleanup_check.sh with the old and new harness from a detached worktree at cdf2a55: byte-identical. Confirms the cat->sed substitution on the stdout/stderr path did not alter default output, so existing baselines remain valid." + }, + { + "command": "REVIEW FOLLOW-UP: XDG redirect measured, not just argued", + "result": "pass", + "evidence": "New fixture xdg_destinations.sh run with a REAL XDG_DATA_HOME pointing at a decoy dir: all four XDG-addressed writes landed in the prefix and the decoy stayed empty. This is the rsync --delete data-loss path closed, proven without running install_leechblock.sh." + }, + { + "command": "REVIEW FOLLOW-UP: partial --bind-abs list, and file bind targets", + "result": "pass", + "evidence": "The absolute scan now runs whenever --prefix is given and reports only paths not covered by an existing bind (a bind of /etc/X11 covers /etc/X11/xorg.conf.d). Fixing that surfaced a second defect: --bind-abs /etc/profile aborted the run with 'Can't mkdir /etc/profile: Not a directory', and binding its parent reintroduced the wholesale /etc shadowing (/home//, unreadable /etc/profile). File targets are now bound file-over-file, seeded with a copy so a script that greps before appending sees real content. Verified: all three nvidia writes captured, /etc/passwd intact, live /etc untouched." } ], "risks": [ diff --git a/meta/scripts/fixtures/xdg_destinations.sh b/meta/scripts/fixtures/xdg_destinations.sh new file mode 100755 index 00000000..afa9f4df --- /dev/null +++ b/meta/scripts/fixtures/xdg_destinations.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash + +# Fixture: writes addressed through XDG_* rather than through $HOME. +# +# --prefix exports XDG_DATA_HOME, XDG_CONFIG_HOME, XDG_CACHE_HOME and +# XDG_STATE_HOME explicitly instead of letting them default off HOME. This +# fixture is what makes that claim measurable: all four writes must appear in +# the manifest under the prefix. +# +# Why it matters beyond tidiness: install_leechblock.sh reads XDG_DATA_HOME +# directly and then runs `rsync -a --delete` at the result. If the harness +# redirected only HOME, a real XDG_DATA_HOME inherited from the caller's +# environment would still point at live data -- and --delete would empty it. +# Proving the redirect here costs nothing; proving it by running the installer +# would risk exactly the loss being guarded against. + +set -euo pipefail + +data_dir="${XDG_DATA_HOME:-$HOME/.local/share}/xdg-fixture" +config_dir="${XDG_CONFIG_HOME:-$HOME/.config}/xdg-fixture" +cache_dir="${XDG_CACHE_HOME:-$HOME/.cache}/xdg-fixture" +state_dir="${XDG_STATE_HOME:-$HOME/.local/state}/xdg-fixture" +mkdir -p "$data_dir" "$config_dir" "$cache_dir" "$state_dir" + +printf 'data\n' >"$data_dir/payload" +printf 'config\n' >"$config_dir/settings.conf" +printf 'cache\n' >"$cache_dir/entry" +printf 'state\n' >"$state_dir/last-run" + +echo "wrote four XDG-addressed files" diff --git a/meta/scripts/lib/trace_prefix.sh b/meta/scripts/lib/trace_prefix.sh index 224c4de6..04da6ba3 100755 --- a/meta/scripts/lib/trace_prefix.sh +++ b/meta/scripts/lib/trace_prefix.sh @@ -119,16 +119,42 @@ trace_prefix_bwrap_argv() { return 0 fi + # Kill the whole process group rather than `return 1`. The caller reads this + # function through `mapfile < <(...)`, and a process substitution's exit + # status is discarded -- returning 1 left the caller with an empty argv, + # which is indistinguishable from "no bind needed", so the run proceeded + # unsandboxed as uid 1000. validate_requirements probes bwrap up front so + # this path should be unreachable; it is a backstop, and it must not be a + # silent one. if ! command -v bwrap >/dev/null 2>&1; then echo "Error: --bind-abs needs bubblewrap (pacman -S bubblewrap)" >&2 - return 1 + kill -TERM $$ + exit 1 fi local -a argv=(bwrap --dev-bind / / --unshare-user --uid 0 --gid 0) - local dir + local dir target for dir in "${abs_dirs[@]}"; do - mkdir -p "$prefix/abs$dir" - argv+=(--bind "$prefix/abs$dir" "$dir") + # A bind target that exists as a FILE is bound as a file: bwrap refuses + # to mount a directory over one ("Can't mkdir /etc/profile: Not a + # directory") and aborts before the run writes anything. Binding its + # PARENT instead is worse than the crash -- that is a wholesale /etc + # bind, which was measured to shadow /etc/passwd (collapsing $SUDO_USER + # to /home//) and hide the real /etc/profile from the script's own + # grep. So seed a file in the prefix and bind file-over-file. + # + # The seed is a COPY of the original, because a script that reads a + # config before rewriting it (nvidia_troubleshoot.sh greps /etc/profile, + # then appends) would otherwise see an empty file and take a branch it + # would not take live. + target="$prefix/abs$dir" + if [[ -f $dir && ! -d $dir ]]; then + mkdir -p "$(dirname "$target")" + cp -p "$dir" "$target" + else + mkdir -p "$target" + fi + argv+=(--bind "$target" "$dir") done printf '%s\n' "${argv[@]}" } diff --git a/meta/scripts/trace_shell_split.sh b/meta/scripts/trace_shell_split.sh index f7f6e6a5..46ebae65 100755 --- a/meta/scripts/trace_shell_split.sh +++ b/meta/scripts/trace_shell_split.sh @@ -81,16 +81,52 @@ validate_requirements() { echo "Error: no such script: $TARGET" >&2 exit 1 fi + # A prefix must start empty. Reusing one across a before/after pair leaves + # run 1's files in place, so run 2's manifest reports writes it never made + # and a DROPPED WRITE STOPS SHOWING UP -- measured: the fixture pair diffs + # clean on a reused prefix. Refuse rather than deleting a user-supplied + # path. + if [[ -n "$PREFIX" && -d "$PREFIX" ]] && [[ -n "$(ls -A "$PREFIX" 2>/dev/null)" ]]; then + echo "Error: --prefix $PREFIX is not empty; use a fresh directory" >&2 + echo " (a stale file makes the manifest report a write that did not happen)" >&2 + exit 1 + fi + # bwrap is probed HERE, not where the argv is built: there the failure is + # returned through a process substitution, whose status mapfile discards, + # so a missing bwrap left runner empty and the run continued with no + # sandbox and no uid 0 -- the exact fail-open this flag exists to prevent. + if [[ ${#BIND_ABS[@]} -gt 0 ]] && ! command -v bwrap >/dev/null 2>&1; then + echo "Error: --bind-abs needs bubblewrap (pacman -S bubblewrap)" >&2 + exit 1 + fi # Refuse rather than quietly writing to the real path: a script with # hardcoded absolute destinations and no --bind-abs would either mutate the # live system or take a different branch on EPERM, and both look like a # clean trace. - if [[ -n "$PREFIX" && ${#BIND_ABS[@]} -eq 0 ]]; then - local -a found=() + # + # The scan runs whenever --prefix is given, not only when BIND_ABS is empty. + # Gating it on "no binds at all" meant a user who bound one path and missed + # three got no warning: those writes fail closed, but the failure surfaces + # only as a stderr line inside a long trace, which is easy to read as the + # script's own behaviour. + if [[ -n "$PREFIX" ]]; then + local -a found=() unbound=() + local path bound covered mapfile -t found < <(trace_prefix_scan_absolute "$TARGET") - if [[ ${#found[@]} -gt 0 ]]; then + for path in ${found[@]+"${found[@]}"}; do + covered=0 + for bound in ${BIND_ABS[@]+"${BIND_ABS[@]}"}; do + # A bind of /etc/X11 covers /etc/X11/xorg.conf.d beneath it. + if [[ $path == "$bound" || $path == "$bound"/* ]]; then + covered=1 + break + fi + done + ((covered)) || unbound+=("$path") + done + if [[ ${#unbound[@]} -gt 0 ]]; then echo "Error: $TARGET writes to absolute paths; pass --bind-abs for each:" >&2 - printf ' --bind-abs %s\n' "${found[@]}" >&2 + printf ' --bind-abs %s\n' "${unbound[@]}" >&2 exit 1 fi fi