From d678783af76c24224d7de468bb584fb4abbf9494 Mon Sep 17 00:00:00 2001 From: ltms Date: Thu, 10 Sep 2026 01:02:34 +0000 Subject: [PATCH] scrub: stop `export UID=` aborting the allow-list scrub mid-loop CB-633's generated scrub.zsh blanks every non-allow-listed exported name in one loop wrapped in `{ ... } 2>/dev/null`. In zsh, `export UID=` is not a failed command: it is a fatal parameter error ("failed to change user ID: operation not permitted") that aborts the whole sourced file. The loop stops at UID, every later name is left unscrubbed, and the report-writing block never runs -- silently, because of the 2>/dev/null. The inversion is the severity. `env` lists inherited names first and the names a startup file exports last, so the loop blanked the harmless inherited half and died immediately before the operator's own exports -- exactly the credentials this policy exists to remove. Measured on a fleet01 member pane: UID is name 42 of 57, and a decoy exported from ~/.zshrc sat at 58, non-empty, on 8 of 8 spawns. xtrace ends at `+scrub.zsh:28> export 'UID='` with rc=126. Reproduced independently on macOS/zsh 5.9 (fleetd #394). CONTAINMENT: `eval`, then VERIFY. Neither `export "$n=" 2>/dev/null || true` nor a `${(t)n}` type guard contains it -- both still abort, because it is an assignment error and not a command failure. Only `eval` does. `eval` is safe at that point because the loop above already rejected every name that is not [A-Za-z_][A-Za-z0-9_]*, so nothing but a bare identifier reaches it. Enumerating the specials (UID|EUID|GID|EGID|PPID|LINENO) also works, but only for the names enumerated: one that turns up exported on another host brings the abort straight back. `eval` covers all of them. `eval` alone, though, contains the error WITHOUT blanking the value, so the name would be reported as blanked having never been blanked -- measured: attempted=59 with UID still 1000. So each name is verified after the attempt and only counted when it actually blanked. A name that could not be blanked currently falls into the "allowed" count, which is imprecise in the safe direction; the honest third count ("tried and could not blank") needs a report-format change -- readReport treats every non-first non-blank line as a blanked name -- and belongs with #394. Why the suite stayed green: EnvAllowListScrubTest starts zsh from a CLEARED parent (`pb.environment().clear()`), where UID is not an exported name at all, so the abort cannot reproduce there. The new test supplies the production shape -- UID present and exported -- and asserts a report exists, which is written by the last statement in the file and so proves the script ran to completion. Verified by mutation in both shapes: reverting the fix fails exactly that test and no other. Orthogonal to #388, which adds a fifth call site to a script that still aborts at the same name. --- .../ltms/fleet/member/EnvAllowListScrub.java | 27 ++++++++++++- .../fleet/member/EnvAllowListScrubTest.java | 38 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java index d56105b..6b47d0b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java @@ -334,7 +334,30 @@ public final class EnvAllowListScrub { _cb633_blank+=("$_cb633_n") done - { for _cb633_n in "${_cb633_blank[@]}"; do export "$_cb633_n="; done; } 2>/dev/null + # `export UID=` is not a failed command: it is a FATAL zsh parameter error + # ("failed to change user ID") that aborts this whole sourced file mid-loop, + # leaving every later name unscrubbed and the report below unwritten — silently, + # because of the 2>/dev/null. Neither `|| true` nor a `${(t)n}` type guard + # contains it; only `eval` does. `eval` is safe here precisely because the loop + # above already rejected every name that is not [A-Za-z_][A-Za-z0-9_]*, so + # nothing but a bare identifier can reach it. + # + # Enumerating the special names instead (UID|EUID|GID|EGID|PPID|LINENO) also + # works, but only for the ones enumerated: a special that turns up exported on + # some other host brings the abort straight back. `eval` contains all of them. + # + # Then VERIFY. A contained failure is still a failure, so a name that did not + # actually blank must not be reported as blanked. It currently falls into the + # "allowed" count, which is imprecise in the safe direction; the honest third + # count ("tried and could not blank") needs a report-format change and belongs + # with fleetd #394, not here. + typeset -a _cb633_done + _cb633_done=() + for _cb633_n in "${_cb633_blank[@]}"; do + eval "export ${_cb633_n}=" 2>/dev/null + [[ -z "${(P)_cb633_n}" ]] && _cb633_done+=("$_cb633_n") + done + _cb633_blank=("${_cb633_done[@]}") integer _cb633_kept=$(( _cb633_total - ${#_cb633_blank} )) { @@ -342,7 +365,7 @@ public final class EnvAllowListScrub { for _cb633_n in "${_cb633_blank[@]}"; do print -r -- "$_cb633_n"; done } > "$ZDOTDIR/%s" 2>/dev/null - unset _cb633_allowed _cb633_names _cb633_blank _cb633_n _cb633_total _cb633_kept + unset _cb633_done _cb633_allowed _cb633_names _cb633_blank _cb633_n _cb633_total _cb633_kept """.formatted(names, MemberEnvAllowList.zshCasePattern(), REPORT_FILE); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java index 6d16bba..55a44b3 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java @@ -18,6 +18,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -118,6 +119,43 @@ class EnvAllowListScrubTest { "allowed N of M with N <= M — the denominator is always reported"); } + /** + * A pane inherits {@code UID}; a cleared test parent does not. The scrub must survive it. + * + *

Every other test here starts zsh from a CLEARED environment, so {@code UID} is never an + * exported name and never reaches the blanking loop. In a real member pane it is exported and + * it IS reached — and {@code export UID=} is a fatal zsh parameter error that aborts the whole + * sourced file, leaving every later name unscrubbed and writing no report at all. The abort is + * silent: the loop is wrapped in {@code 2>/dev/null}. + * + *

The assertion is deliberately "a report exists" rather than "the canary is blanked". The + * report is written by the last statement in the file, so its presence proves the script ran + * to completion; the canary alone would depend on where it happens to sit in {@code env} order. + * Both are checked, but only the first one fails deterministically without the fix. + */ + @Test + void scrubSurvivesAnInheritedUidTheWayARealPaneHasIt(@TempDir Path tmp) throws Exception { + assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here"); + Set allowed = MemberEnvAllowList.derive(List.of()); + Path zdotdir = EnvAllowListScrub.generate(tmp, allowed); + + // The production shape: UID present and exported, as every pane shell inherits it. + Map paneLikeParent = Map.of( + "HOME", System.getProperty("user.home"), + "PATH", "/usr/bin:/bin", + "SHELL", "/bin/zsh", + "UID", "1000", + "CB633_CANARY", "must-not-survive-the-scrub"); + Set survivors = exportedNamesFromCleanParent(paneLikeParent, zdotdir); + + EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(zdotdir); + assertNotNull(report, + "an inherited UID must not abort the scrub — no report means the file died mid-loop " + + "and every name after UID in `env` order was left unscrubbed"); + assertFalse(survivors.contains("CB633_CANARY"), + "a non-allow-listed name must still be blanked when UID is in the environment"); + } + /** A group-shared ZDOTDIR still lets the member truncate and write its pre-created receipt. */ @Test void groupSharedScrubWritesAndReadsItsReport(@TempDir Path tmp) throws Exception { -- 2.52.0