From 5c08054533d179e17834e890766d78574446f58d Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 08:24:46 +0700 Subject: [PATCH] #394 follow-up: re-assert the identifier guard at the eval call site EnvAllowListScrub's blanking loop splices each name into a string handed to eval ("export ${n}="). That is only safe because every name reaching _cb633_blank already passed an identifier check in the enumeration loop -- 20 lines away, in a different loop. Before eval was introduced a non-conforming name reaching plain `export "$n="` was inert either way (the quoting neutralized it); eval removed that safety net, so the enumeration loop's guard became the ONLY thing standing between a non-identifier string and code execution in the member's pane, with nothing at the eval site itself defending that property. Re-assert the same [A-Za-z_][A-Za-z0-9_]* check immediately before the eval call, independent of the enumeration loop's own guard (left untouched, not moved). A name that fails it is counted unblankable rather than silently dropped, so a bypass of the upstream guard would leave real evidence in the report. New test exploits the "junk from multi-line values" gap the enumeration loop's own comment already documents: a value with an embedded newline makes `command env`'s text output split into a spurious extra "name" line that was never a real variable. Runs the real generated scrubScript() end-to-end under zsh and asserts the non-conforming fragment is neither blanked nor counted unblankable. The fragment used is merely non-conforming (contains a dot) -- never command-shaped. Mutation-verified both guards. Weakening the enumeration guard alone DOES break the new test (the fragment then reaches the new eval-site guard and gets counted unblankable, failing the "not unblankable" assertion). Removing the new eval-site guard alone, with the enumeration guard intact, does NOT break it: _cb633_blank has exactly one producer (the enumeration loop), so nothing can reach the eval site without already having passed the identical check there. That is expected given the single-source architecture, and it is exactly why the eval-site guard is defense-in-depth against a future change that adds a second path into _cb633_blank or decouples the two loops -- not a currently independently-observable divergence. --- .../ltms/fleet/member/EnvAllowListScrub.java | 23 ++++++++ .../fleet/member/EnvAllowListScrubTest.java | 55 +++++++++++++++++++ 2 files changed, 78 insertions(+) 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 b2ba33e..57831c3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java @@ -88,6 +88,15 @@ import java.util.stream.Stream; * listed {@code !}-prefixed rather than silently disappearing. This is deliberately not a skip-list * of known-bad names — every enumerated name is still attempted, so a name nobody has thought of * yet still gets tried and, if it fails, still gets counted. + * + *

The blanking loop also re-asserts, on its own, the same {@code [A-Za-z_][A-Za-z0-9_]*} shape + * check the enumeration loop already applied. Before {@code eval} was introduced a non-conforming + * name reaching {@code export "$n="} was harmless either way — the quoting made it inert. With + * {@code eval}, the name is spliced into a string and interpreted as shell syntax, so the enumeration + * loop's check is no longer sufficient on its own to keep that call site safe — it is a guard on a + * different loop, and the two must not silently drift apart. Re-checking right before the + * {@code eval} keeps that call site safe by its own reading, independent of whatever the enumeration + * loop does or stops doing in a later change. */ public final class EnvAllowListScrub { @@ -357,10 +366,24 @@ public final class EnvAllowListScrub { # loop continues and we can tell allowed / blanked / unblankable apart afterwards. # This is not a skip-list of known-bad names (that would miss the next one nobody # thought of) — every name in _cb633_blank is still attempted, unconditionally. + # Every name reaching this loop already passed the identical identifier check in the + # enumeration loop above — but that guard is 20 lines away in a different loop, and + # this line is about to splice the name into a string handed to `eval`. Before this + # fix the name only ever reached `export` quoted ("$n="), which is inert on a + # non-identifier string either way; `eval` makes THIS line the only thing standing + # between such a string and code execution in the member's pane, so it re-asserts the + # same check on its own rather than trusting a guard it does not own. Under normal + # operation this can never fire (the enumeration guard already filtered everything + # reaching _cb633_blank), so a name caught here is counted as unblankable rather than + # silently dropped — it is real evidence that the upstream guard was bypassed. typeset -a _cb633_ok _cb633_unblankable _cb633_ok=() _cb633_unblankable=() for _cb633_n in "${_cb633_blank[@]}"; do + if [[ ! "$_cb633_n" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + _cb633_unblankable+=("$_cb633_n") + continue + fi if eval "export ${_cb633_n}=" 2>/dev/null; then _cb633_ok+=("$_cb633_n") else 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 e967b16..8ccdd77 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; @@ -189,6 +190,60 @@ class EnvAllowListScrubTest { "the unblankable name is reported by name, not silently dropped"); } + /** + * fleetd #394 follow-up: the blanking loop's {@code eval "export ${n}="} splices {@code n} into + * a string that zsh then interprets as shell syntax. That is only safe because every name + * reaching {@code _cb633_blank} already passed an identifier check in the ENUMERATION loop + * (20 lines away, in a different loop) — so the fix re-asserts the identical check immediately + * before the {@code eval} call, rather than trusting that distant guard to keep holding. + * + *

This test plants a value with an embedded newline, exploiting the exact "junk from + * multi-line values" gap the enumeration loop's own comment already documents: {@code command + * env}'s text output is read line-by-line, so a value's second line becomes a spurious extra + * "name" that was never a real exported variable. The fragment used here ({@code + * junk.fragment}) is merely non-conforming (it contains a dot) — never command-shaped; this + * test must never demonstrate command execution and plants no command-shaped payload. + * + *

Exercises the real artefact end-to-end: {@link EnvAllowListScrub#scrubScript} runs + * verbatim under a real {@code /bin/zsh}, exactly as {@code generate()} would produce it — this + * is not a synthetic call into just the blanking loop. + */ + @Test + void nonIdentifierJunkFromAMultilineValueIsSkippedNotBlankedOrUnblankable(@TempDir Path tmp) + throws Exception { + assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here"); + + String script = EnvAllowListScrub.scrubScript(Set.of("ZDOTDIR")); + + ProcessBuilder pb = new ProcessBuilder("/bin/zsh"); + pb.environment().clear(); + pb.environment().put("PATH", "/usr/bin:/bin"); + pb.environment().put("ZDOTDIR", tmp.toAbsolutePath().toString()); + // Embedded newline: `command env`'s own text output splits this into two lines, and the + // second ("junk.fragment") has no "=" at all, so `cut -d= -f1` returns it unchanged as a + // spurious candidate "name" — it was never an actual exported variable by that name. + pb.environment().put("FLEETD_TEST_MULTILINE", "keep\njunk.fragment"); + pb.redirectError(ProcessBuilder.Redirect.DISCARD); + Process zsh = pb.start(); + zsh.getOutputStream().write(script.getBytes(StandardCharsets.UTF_8)); + zsh.getOutputStream().flush(); + zsh.getOutputStream().close(); + assertTrue(zsh.waitFor(60, java.util.concurrent.TimeUnit.SECONDS), + "the scrub script did not exit within 60s"); + assertEquals(0, zsh.exitValue(), "the scrub script must reach its end"); + + EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(tmp); + assertNotNull(report, "the report must still be written"); + assertTrue(report.blanked().contains("FLEETD_TEST_MULTILINE"), + "sanity: the real, identifier-shaped variable must still be blanked normally"); + assertFalse(report.blanked().contains("junk.fragment"), + "a non-identifier fragment is not a real variable and must never be blanked"); + assertFalse(report.unblankable().contains("junk.fragment"), + "a non-identifier fragment must never even become a candidate the blanking loop " + + "attempts — it must be filtered before either guard has to catch it, so " + + "it is neither blanked nor counted as a failed attempt"); + } + /** A group-shared ZDOTDIR still lets the member truncate and write its pre-created receipt. */ @Test void groupSharedScrubWritesAndReadsItsReport(@TempDir Path tmp) throws Exception {