From 68a4996f0b7f44472c0ad7a1bfed0b077b3dd5b5 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Sat, 18 Jul 2026 18:11:26 +0000 Subject: [PATCH] fix: version names die at one shared gate, and --purge-host hears --force MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 convergence from all three reviewers, both findings real: A version string used to be a path fragment: 'box uninstall ../../../.ssh' resolved below versions/ and rm -rf'd wherever it landed, 'box use' could point current outside the root, and a hostile flat-tree VERSION could steer the migration's mv the same way. One strict validator now gates every caller — only [A-Za-z0-9._+-], no leading '.' or '-' — byte-identical in install.sh and bin/box like existing_boxes, diff-guarded in the tests, with traversal regressions on use, uninstall and the migration (which now refuses BEFORE the tree moves anywhere). --force is uninstall's installer-family consent, and --purge-host now forwards it: teardown-host.sh gets --yes under --force/BOX_YES, so the combined non-interactive uninstall no longer dies at teardown's own prompt. CI's drill now runs the combined verb with --force alone (no BOX_YES, no TTY) — the exact invocation that used to abort. Also grok's polish, taken: current flips by rename (ln to a side name, mv -Tf over — no window with no current) in both install.sh and 'box use'; BOX_REINSTALL swaps by two renames and deletes LAST; and the single-version path refuses while current is dangling (readlink -f resolves a missing last component, so the guard checks the DIRECTORY, not just the string). Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 7 +++++-- bin/box | 40 ++++++++++++++++++++++++++++++++--- install.sh | 45 +++++++++++++++++++++++++++++++++------- test/cli.sh | 42 +++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ff847fc..e2846ec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -74,8 +74,11 @@ jobs: sudo test ! -e "/var/lib/incus/users/$uid" ! sudo incus project show "user-$uid" ! sudo incus config trust list --format csv | grep -q "incus-user-$uid" - sudo BOX_YES=1 /usr/local/bin/box teardown-host - sudo BOX_YES=1 /usr/local/bin/box uninstall --all + # The COMBINED verb, --force only, deliberately no BOX_YES and no + # TTY: this is the exact invocation that used to die at teardown's + # own prompt when consent was not forwarded (--purge-host now + # passes --yes through under --force/BOX_YES). + sudo /usr/local/bin/box uninstall --all --purge-host --force # zero residue: the daemon's state... ! sudo incus network show boxnet ! sudo incus profile show box-net diff --git a/bin/box b/bin/box index 0b69d7e..f764317 100755 --- a/bin/box +++ b/bin/box @@ -1112,6 +1112,21 @@ install_root() { dirname "$vdir" } +# A version is a DIRECTORY NAME under versions/ — nothing else. One strict +# gate for every caller that builds a path from one (the installer's new_ver, +# migration's flat_ver, and bin/box's 'use'/single-version uninstall): only +# [A-Za-z0-9._+-], no leading '.' or '-'. That forbids '/', '..'-escapes, +# spaces and option-lookalikes by construction — a crafted version dies HERE, +# never in an rm -rf or an ln. install.sh carries a byte-identical copy; +# test/cli.sh diffs the two so the gates cannot drift. +valid_version() { + case "$1" in + ''|.*|-*) return 1 ;; + *[!A-Za-z0-9._+-]*) return 1 ;; + esac + return 0 +} + # Which boxes exist on this host, at THIS caller's tier? Prints their names # (both tag generations) and succeeds when at least one exists; fails when # none are visible — including when incus is absent or not answering, because @@ -1188,9 +1203,12 @@ cmd_use() { local v="${args[0]:-}" ir eff expect out [ -n "$v" ] || usage_error "usage: $(synopsis_of use)" ir="$(install_root)" || die "this box runs from a working tree ($root), not a versioned install — nothing to switch" + valid_version "$v" || die "not a sane version name: '$v' (a version is a directory name under versions/ — see 'box versions')" [ -d "$ir/versions/$v" ] || die "no such version: $v (see 'box versions')" die_under_boxes "switch the default box version" "box use $v" - ln -sfn "versions/$v" "$ir/current" + # An atomic flip, not unlink+create: ln -sfn leaves a window where current + # is missing; a rename over it does not. + ln -sfn "versions/$v" "$ir/current.new.$$" && mv -Tf "$ir/current.new.$$" "$ir/current" converge_bin_links "$ir" # Assert the EFFECTIVE result, not the intent: current must resolve to the # version asked for, and the chain's own binary must answer that version — @@ -1246,8 +1264,15 @@ cmd_uninstall() { # -- one version ----------------------------------------------------------- if [ -n "$ver" ] && [ "$all" -eq 0 ]; then [ "$purge_host" -eq 0 ] || usage_error "--purge-host goes with the full uninstall, not a single version" + valid_version "$ver" || die "not a sane version name: '$ver' (a version is a directory name under versions/ — see 'box versions')" [ -d "$ir/versions/$ver" ] || die "no such version: $ver (see 'box versions')" cur="$(basename "$(readlink -f "$ir/current" 2>/dev/null || true)")" + # A broken current makes the CURRENT guard below unfireable (cur empty + # when the link is missing; cur naming a non-directory when it dangles — + # readlink -f resolves a link whose last component does not exist). Heal + # first, then decide; never delete around a broken default. + { [ -n "$cur" ] && [ -d "$ir/versions/$cur" ]; } \ + || die "current is dangling — 'box use ' to repoint the default first (refusing to remove versions while it is broken)" [ "$ver" != "$cur" ] || die "$ver is the CURRENT version — 'box use ' first, or 'box uninstall --all' for everything" uninstall_confirm "remove box version $ver from $ir" # rm's exit code is not the verdict — the absence re-check below is (a @@ -1268,8 +1293,17 @@ cmd_uninstall() { # 'box revoke --purge' is the clean path (and asserts its absence). granted="$(timeout 10 incus project list --format csv 2>/dev/null --purge' removes each world cleanly first" >&2 - bash "$root/host/teardown-host.sh" \ - || die "teardown-host did not complete — stopping BEFORE removing the install (the tree is untouched; fix the error and re-run)" + # Consent forwards: --force and BOX_YES are this verb's installer-family + # yes, and teardown-host must hear it too — otherwise a non-interactive + # 'uninstall --all --purge-host --force' dies at teardown's own prompt + # (EOF on read) with the tree untouched but the promise broken. + if [ "$force" -eq 1 ] || [ -n "${BOX_YES:-}" ]; then + bash "$root/host/teardown-host.sh" --yes \ + || die "teardown-host did not complete — stopping BEFORE removing the install (the tree is untouched; fix the error and re-run)" + else + bash "$root/host/teardown-host.sh" \ + || die "teardown-host did not complete — stopping BEFORE removing the install (the tree is untouched; fix the error and re-run)" + fi else die_under_boxes "uninstall box" "box uninstall (or 'box uninstall --purge-host' to tear the host stack down with them)" fi diff --git a/install.sh b/install.sh index 2d364f4..37fb3a8 100755 --- a/install.sh +++ b/install.sh @@ -62,6 +62,21 @@ confirm() { # $1 = question case "$reply" in y|Y|yes|YES) return 0 ;; *) return 1 ;; esac } +# A version is a DIRECTORY NAME under versions/ — nothing else. One strict +# gate for every caller that builds a path from one (the installer's new_ver, +# migration's flat_ver, and bin/box's 'use'/single-version uninstall): only +# [A-Za-z0-9._+-], no leading '.' or '-'. That forbids '/', '..'-escapes, +# spaces and option-lookalikes by construction — a crafted version dies HERE, +# never in an rm -rf or an ln. bin/box carries a byte-identical copy; +# test/cli.sh diffs the two so the gates cannot drift. +valid_version() { + case "$1" in + ''|.*|-*) return 1 ;; + *[!A-Za-z0-9._+-]*) return 1 ;; + esac + return 0 +} + # Which boxes exist on this host, at THIS caller's tier? Prints their names # (both tag generations) and succeeds when at least one exists; fails when # none are visible — including when incus is absent or not answering, because @@ -98,6 +113,15 @@ fi # under my boxes" class of failures (#66). confirm "Install box from $SRCDESC?" || die "cancelled — nothing was changed." +# Flip $DEST/current to versions/ atomically: build the new link beside it, +# rename over. Plain ln -sfn is unlink+create — a window where current names +# nothing and a concurrent 'box' invocation dies mid-chain. bin/box's cmd_use +# flips with the same pattern. +flip_current() { + ln -sfn "versions/$1" "$DEST/current.new.$$" + mv -Tf "$DEST/current.new.$$" "$DEST/current" +} + # --- migrate a pre-0.7.0 flat install -------------------------------------- # 0.6.0 and earlier installed the tree FLAT at $DEST (bin/box directly under # it). Move such a tree to versions/ BEFORE anything else, so an @@ -106,12 +130,16 @@ confirm "Install box from $SRCDESC?" || die "cancelled — nothing was changed." # window with no install — and the operator's tree is preserved bit for bit. if [ -e "$DEST/bin/box" ] && [ ! -d "$DEST/versions" ]; then flat_ver="$(cat "$DEST/VERSION" 2>/dev/null || echo 0.0.0-unknown)" + # The flat tree's VERSION is data from disk, not from this installer — the + # same trust boundary as the new_ver check, so the same gate: a corrupted + # (or hostile) VERSION must not steer the mv/ln below out of versions/. + valid_version "$flat_ver" || die "the flat install's VERSION is not a sane directory name: '$flat_ver' — fix $DEST/VERSION (one line, e.g. 0.6.0), then re-run" log "found a pre-0.7.0 flat install at $DEST (version $flat_ver) — migrating it into the versioned layout" staging="$DEST.migrating.$$" mv "$DEST" "$staging" mkdir -p "$DEST/versions" mv "$staging" "$DEST/versions/$flat_ver" - ln -sfn "versions/$flat_ver" "$DEST/current" + flip_current "$flat_ver" mkdir -p "$BINDIR" ln -sfn "$DEST/current/bin/box" "$BINDIR/box" log "migrated: it now lives at $DEST/versions/$flat_ver (still current; your boxes are untouched)" @@ -176,7 +204,7 @@ fi # the identity of what is being installed, and 'box versions' lists these names. new_ver="$(cat "$EXTRACTED/VERSION" 2>/dev/null || true)" [ -n "$new_ver" ] || die "source has no VERSION file — cannot install it as a version" -case "$new_ver" in */* | *' '* | .*) die "the source's VERSION is not a sane directory name: '$new_ver'" ;; esac +valid_version "$new_ver" || die "the source's VERSION is not a sane directory name: '$new_ver'" # --- install into $DEST/versions/ --------------------------------- VDIR="$DEST/versions/$new_ver" @@ -186,12 +214,15 @@ if [ -d "$VDIR" ]; then # Replace THIS version's tree, as atomically as two renames allow — never # a partial overlay of new files onto an old tree. log "BOX_REINSTALL=1 — replacing the installed $new_ver tree" - stage="$VDIR.new.$$" - rm -rf "$stage" + stage="$VDIR.new.$$"; old="$VDIR.old.$$" + rm -rf "$stage" "$old" chmod +x "$EXTRACTED/bin/box" mv "$EXTRACTED" "$stage" - rm -rf "$VDIR" + # Swap by renames, delete LAST: rm-then-move leaves a hole the whole + # length of the delete where current -> this version resolves to nothing. + mv "$VDIR" "$old" mv "$stage" "$VDIR" + rm -rf "$old" printf '%s\n' "$INSTALLED_FROM" > "$VDIR/INSTALLED_FROM" log "reinstalled $new_ver" else @@ -221,7 +252,7 @@ fi cur="$(readlink -f "$DEST/current" 2>/dev/null || true)" want="$(readlink -f "$VDIR")" if [ -z "$cur" ] || [ ! -d "$cur" ]; then - ln -sfn "versions/$new_ver" "$DEST/current" + flip_current "$new_ver" log "default version: $new_ver" elif [ "$cur" = "$want" ]; then : # already the default — nothing to flip @@ -241,7 +272,7 @@ else log " (a portable 'box export' is #70), then 'box rm ' when you are done" log " · then flip the default: box use $new_ver" else - ln -sfn "versions/$new_ver" "$DEST/current" + flip_current "$new_ver" log "default version switched: $old_ver -> $new_ver ('box use $old_ver' switches back)" fi fi diff --git a/test/cli.sh b/test/cli.sh index b1f35ee..52600d2 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -368,6 +368,10 @@ check "versions: marks the running one" 0 "(running)" ibox "$B1/box" versions # --- box use ---------------------------------------------------------------- check "use: no argument is a usage error" 2 "usage: box use" ibox "$B1/box" use check "use: an unknown version is refused by name" 1 "no such version" ibox "$B1/box" use 1.2.3 +# A version is a directory NAME — a crafted one must die at the gate, never +# reach the ln (current pointing outside the root) or an rm -rf. +check "use: a path-traversal version dies at the gate" 1 "not a sane version name" \ + ibox "$B1/box" use '../../tmp/evil' check "use: refuses under existing boxes, naming them (#66)" 1 "wedged" \ ibox FAKE_BOXES="wedged stuck" "$B1/box" use "$VER" check "use: the refusal points at the remedy (box rm, then re-run)" 1 "box rm" \ @@ -417,6 +421,26 @@ check "migrate+upgrade: both versions present" 0 "" \ check "migrate+upgrade: no boxes → the new version is the default" 0 "box 9.9.9-drill" \ ibox "$B4/box" --version +# A broken current must halt the single-version path BEFORE any decision: the +# CURRENT guard keys off what current resolves to, and a dangling link makes +# that answer a lie. Drive the version tree's own binary — the current chain +# is exactly what is broken. H4 has two versions; heal current afterwards. +ln -sfn "versions/gone" "$H4/current" +check "uninstall: refuses while current is dangling (heal before delete)" 1 "dangling" \ + ibox "$H4/versions/$VER/bin/box" uninstall 9.9.9-drill --force +check "uninstall: ...and both version trees survived the refusal" 0 "" \ + bash -c "[ -d '$H4/versions/$VER' ] && [ -d '$H4/versions/9.9.9-drill' ]" +ln -sfn "versions/9.9.9-drill" "$H4/current" + +# The migration reads VERSION off the old tree — disk data, not installer +# data. A hostile value must refuse BEFORE the tree moves anywhere. +H9="$WORK/h9"; B9="$WORK/b9"; mkdir -p "$H9/bin" "$B9" +cp "$ROOT/bin/box" "$H9/bin/box"; chmod +x "$H9/bin/box" +printf '%s\n' '../pwn' > "$H9/VERSION" +check "migrate: a hostile flat VERSION refuses to migrate" 1 "not a sane directory name" \ + inst "$H9" "$B9" +check "migrate: ...with the flat tree untouched where it was" 0 "" test -x "$H9/bin/box" + # --- healing: a wedged \$BINDIR/box must never block an install ------------- H5="$WORK/h5"; B5="$WORK/b5"; mkdir -p "$B5" ln -s "$WORK/nowhere/box" "$B5/box" # dangling @@ -433,6 +457,8 @@ check "uninstall: refuses to remove the CURRENT version" 1 "CURRENT" \ ibox "$B1/box" uninstall "$VER" --force check "uninstall: an unknown version is refused by name" 1 "no such version" \ ibox "$B1/box" uninstall 5.5.5 --force +check "uninstall: a path-traversal version dies at the gate (never an rm -rf)" 1 "not a sane version name" \ + ibox "$B1/box" uninstall '../../../../etc' --force check "uninstall: a version plus --all is ambiguous (usage error)" 2 "" \ ibox "$B1/box" uninstall 9.9.9-drill --all --force check "uninstall: removes a non-current version" 0 "removed version" \ @@ -485,6 +511,22 @@ check "existing_boxes: extracted from bin/box (guards the awk)" 0 "user.box=1" c check "existing_boxes: bin/box and install.sh copies are byte-identical" 0 "" diff "$EBBIN" "$EBINST" rm -f "$EBBIN" "$EBINST" +# Same discipline for the version-name gate: one policy, two copies, no drift +# — a version that install.sh would refuse must not be one 'box use' accepts. +VVBIN="$(mktemp)"; VVINST="$(mktemp)" +awk '/^valid_version\(\) \{/,/^\}/' "$ROOT/bin/box" > "$VVBIN" +awk '/^valid_version\(\) \{/,/^\}/' "$ROOT/install.sh" > "$VVINST" +check "valid_version: extracted from bin/box (guards the awk)" 0 "A-Za-z0-9" cat "$VVBIN" +check "valid_version: bin/box and install.sh copies are byte-identical" 0 "" diff "$VVBIN" "$VVINST" +rm -f "$VVBIN" "$VVINST" + +# --purge-host must FORWARD installer-family consent: under --force/BOX_YES +# the teardown call carries --yes, or a non-interactive combined uninstall +# dies at teardown's own prompt with the flag's promise broken. +# shellcheck disable=SC2016 # the $-string is a literal in the target file +check "uninstall: --purge-host forwards consent to teardown-host (--yes)" 0 "" \ + grep -qF -- 'bash "$root/host/teardown-host.sh" --yes' "$ROOT/bin/box" + # --- the help keeps its promises -------------------------------------------- check "help: the table lists 'versions'" 0 "versions" "$BOX" help check "help use: names the #66 stance" 0 "boxes" "$BOX" help use