From e7ddda74b58bdc249cd435337baa03e9af7a73c8 Mon Sep 17 00:00:00 2001 From: mrjf Date: Sat, 16 May 2026 07:18:37 -0700 Subject: [PATCH 1/3] Fix #25: eval injection in deny.cli wrapper generation The CLI deny-wrapper generator embedded user-authored deny.cli patterns into a bash array literal and matched them via `eval`. A pattern like `foo$(id)*` would execute at wrapper-generation time (because bash array assignment performs command substitution) or at match time (because eval re-parses the value of $P). Fix: - Patterns are written to a side file (one per line) and read at run time with `read -r`, which does not perform expansion. - The match uses `[[ "$FULL_CMD" == $P ]]` directly. Parameter expansion of $P substitutes the value but does not recurse into command substitution, so $(...) inside a pattern is matched literally. - Command names (first word of allow/deny patterns) must match a conservative character class; values containing shell metacharacters are dropped at parse time. Extracted the wrapper-script builder to packages/runtime/src/cli-wrapper.ts so it can be unit-tested without the band-server's startup side effects. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/runtime/src/band-server.ts | 31 ++-------- packages/runtime/src/cli-wrapper.ts | 52 +++++++++++++++++ .../unit/band-server-cli-wrappers.test.ts | 58 +++++++++++++++++++ 3 files changed, 116 insertions(+), 25 deletions(-) create mode 100644 packages/runtime/src/cli-wrapper.ts create mode 100644 packages/runtime/test/unit/band-server-cli-wrappers.test.ts diff --git a/packages/runtime/src/band-server.ts b/packages/runtime/src/band-server.ts index 211c568..79402ee 100644 --- a/packages/runtime/src/band-server.ts +++ b/packages/runtime/src/band-server.ts @@ -21,6 +21,7 @@ import { Hono } from "hono"; import { createHash } from "crypto"; +import { buildCliWrapperScript, buildDenyPatternsFile, SAFE_CMD_NAME } from "./cli-wrapper"; const BAND_RUNNER_USER = "band-runner"; let executing = false; @@ -333,7 +334,7 @@ function setupCliWrappers( const allowedCommands = new Set(ESSENTIAL_COMMANDS); for (const pattern of allowPatterns) { const cmd = pattern.split(/\s+/)[0]; - if (cmd && !cmd.includes("*") && !cmd.includes("/")) { + if (cmd && SAFE_CMD_NAME.test(cmd)) { allowedCommands.add(cmd); } } @@ -341,13 +342,14 @@ function setupCliWrappers( const denyByCmd = new Map(); for (const pattern of denyPatterns) { const cmd = pattern.split(/\s+/)[0]; - if (!cmd) continue; + if (!cmd || !SAFE_CMD_NAME.test(cmd)) continue; const existing = denyByCmd.get(cmd) || []; existing.push(pattern); denyByCmd.set(cmd, existing); } for (const cmd of allowedCommands) { + if (!SAFE_CMD_NAME.test(cmd)) continue; let realPath: string; try { realPath = shell(`readlink -f $(which ${cmd}) 2>/dev/null`).trim(); @@ -355,31 +357,10 @@ function setupCliWrappers( if (!realPath) continue; const denyPats = denyByCmd.get(cmd) || []; - const logLine = `[ -n "\$BAND_OPS_FILE" ] && echo "${cmd} $*" >> "\$BAND_OPS_FILE"`; - - let wrapper: string; if (denyPats.length > 0) { - const patternArray = denyPats.map(p => `"${p.replace(/"/g, '\\"')}"`).join(" "); - wrapper = `#!/bin/bash -FULL_CMD="${cmd} $*" -DENY_PATTERNS=(${patternArray}) -for P in "\${DENY_PATTERNS[@]}"; do - if eval "[[ \\"\\$FULL_CMD\\" == \\$P ]]" 2>/dev/null; then - echo "DENIED: $FULL_CMD" >&2 - exit 126 - fi -done -${logLine} -exec ${realPath} "$@" -`; - } else { - wrapper = `#!/bin/bash -${logLine} -exec ${realPath} "$@" -`; + writeFile(`${wrapperDir}/.deny-${cmd}`, buildDenyPatternsFile(denyPats)); } - - writeFile(`${wrapperDir}/${cmd}`, wrapper); + writeFile(`${wrapperDir}/${cmd}`, buildCliWrapperScript(cmd, realPath, denyPats.length > 0)); shell(`chmod +x ${wrapperDir}/${cmd}`); } } diff --git a/packages/runtime/src/cli-wrapper.ts b/packages/runtime/src/cli-wrapper.ts new file mode 100644 index 0000000..59b7aef --- /dev/null +++ b/packages/runtime/src/cli-wrapper.ts @@ -0,0 +1,52 @@ +/** + * CLI wrapper-script generation. Pure functions, no side effects. + * + * The band server creates a per-execution directory of small bash wrapper + * scripts — one per allowed command. PATH is set to that directory inside the + * sandbox, so only declared commands resolve. Each wrapper logs its + * invocation, checks deny patterns, and execs the real binary. + * + * Security note: deny patterns originate from user-authored BAND.md files. + * They must never be embedded into the wrapper script source. Bash array + * literals and unquoted heredocs perform command substitution at parse time, + * so a pattern like `foo$(id)*` would execute when the wrapper is generated. + * Instead, patterns are written to a side file and read at run time with + * `read -r`, which does no expansion. The match expression `[[ "$x" == $P ]]` + * does glob matching on the value of P without re-parsing. + */ + +/** Conventional command names: letters, digits, underscore, dash, dot. */ +export const SAFE_CMD_NAME = /^[a-zA-Z0-9_.-]+$/; + +export function buildCliWrapperScript( + cmd: string, + realPath: string, + hasDeny: boolean +): string { + if (!SAFE_CMD_NAME.test(cmd)) { + throw new Error(`unsafe cmd name: ${cmd}`); + } + const logLine = `[ -n "\$BAND_OPS_FILE" ] && echo "${cmd} $*" >> "\$BAND_OPS_FILE"`; + if (!hasDeny) { + return `#!/bin/bash +${logLine} +exec ${realPath} "$@" +`; + } + return `#!/bin/bash +FULL_CMD="${cmd} $*" +while IFS= read -r P; do + [ -z "\$P" ] && continue + if [[ "\$FULL_CMD" == \$P ]]; then + echo "DENIED: \$FULL_CMD" >&2 + exit 126 + fi +done < "\$(dirname "\$0")/.deny-${cmd}" +${logLine} +exec ${realPath} "$@" +`; +} + +export function buildDenyPatternsFile(patterns: string[]): string { + return patterns.join("\n") + "\n"; +} diff --git a/packages/runtime/test/unit/band-server-cli-wrappers.test.ts b/packages/runtime/test/unit/band-server-cli-wrappers.test.ts new file mode 100644 index 0000000..410a673 --- /dev/null +++ b/packages/runtime/test/unit/band-server-cli-wrappers.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, test } from "bun:test"; +import { buildCliWrapperScript, buildDenyPatternsFile, SAFE_CMD_NAME } from "../../src/cli-wrapper"; + +describe("buildCliWrapperScript", () => { + test("no-deny wrapper just logs and execs realPath", () => { + const script = buildCliWrapperScript("ls", "/usr/bin/ls", false); + expect(script).toContain("exec /usr/bin/ls"); + expect(script).not.toContain("DENY_PATTERNS"); + expect(script).not.toContain("eval"); + expect(script).not.toContain(".deny-"); + }); + + test("deny wrapper reads patterns from side file, never embeds them", () => { + const script = buildCliWrapperScript("rm", "/usr/bin/rm", true); + expect(script).toContain("read -r P"); + expect(script).toContain('"$(dirname "$0")/.deny-rm"'); + expect(script).toContain("[[ \"$FULL_CMD\" == $P ]]"); + expect(script).not.toContain("eval"); + expect(script).not.toContain("DENY_PATTERNS=("); + }); + + test("rejects cmd names containing shell metacharacters", () => { + expect(() => buildCliWrapperScript("rm$(id)", "/usr/bin/rm", false)).toThrow(); + expect(() => buildCliWrapperScript("rm;ls", "/usr/bin/rm", false)).toThrow(); + expect(() => buildCliWrapperScript("rm`id`", "/usr/bin/rm", false)).toThrow(); + expect(() => buildCliWrapperScript("rm ls", "/usr/bin/rm", false)).toThrow(); + expect(() => buildCliWrapperScript("../bin/rm", "/usr/bin/rm", false)).toThrow(); + }); + + test("accepts conventional cmd names", () => { + expect(() => buildCliWrapperScript("ls", "/usr/bin/ls", false)).not.toThrow(); + expect(() => buildCliWrapperScript("aws-cli", "/usr/bin/aws-cli", false)).not.toThrow(); + expect(() => buildCliWrapperScript("git_lfs", "/usr/bin/git_lfs", false)).not.toThrow(); + expect(() => buildCliWrapperScript("node18", "/usr/bin/node18", false)).not.toThrow(); + expect(() => buildCliWrapperScript("a.out", "/tmp/a.out", false)).not.toThrow(); + }); +}); + +describe("buildDenyPatternsFile", () => { + test("emits patterns one per line with trailing newline, no escaping", () => { + const file = buildDenyPatternsFile(["rm -rf *", 'foo$(id)*', "echo `whoami`"]); + expect(file).toBe("rm -rf *\nfoo$(id)*\necho `whoami`\n"); + }); +}); + +describe("SAFE_CMD_NAME", () => { + test("rejects shell metacharacters", () => { + for (const bad of ["$(id)", "`id`", "rm;ls", "rm|ls", "rm&", "rm>x", "a b", "*", "../rm", "rm$X"]) { + expect(SAFE_CMD_NAME.test(bad)).toBe(false); + } + }); + + test("accepts conventional identifiers", () => { + for (const ok of ["ls", "git", "gh", "git-lfs", "git_lfs", "a.out", "node18"]) { + expect(SAFE_CMD_NAME.test(ok)).toBe(true); + } + }); +}); From 2162c3f4a6bec391aa3aa28da5b77719e004ec69 Mon Sep 17 00:00:00 2001 From: mrjf Date: Sat, 16 May 2026 08:03:29 -0700 Subject: [PATCH 2/3] CI: copy cli-wrapper.ts to Lima VM alongside band-server.ts band-server.ts now imports from ./cli-wrapper, which was introduced in the eval-injection fix. CI only copied server.ts to the VM, causing "Cannot find module './cli-wrapper'" on startup. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7766a8f..257f3e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,6 +80,7 @@ jobs: # Deploy band server using limactl copy (avoids pipe issues with --plain) limactl shell bands-executor -- mkdir -p $VM_HOME/bands-server limactl copy packages/runtime/src/band-server.ts bands-executor:$VM_HOME/bands-server/server.ts + limactl copy packages/runtime/src/cli-wrapper.ts bands-executor:$VM_HOME/bands-server/cli-wrapper.ts limactl shell bands-executor -- bash -c "cd $VM_HOME/bands-server && $VM_HOME/.bun/bin/bun add hono" # Create systemd user service for the band server limactl shell bands-executor -- mkdir -p $VM_HOME/.config/systemd/user @@ -198,6 +199,7 @@ jobs: # Deploy band server using limactl copy (avoids pipe issues with --plain) limactl shell bands-executor -- mkdir -p $VM_HOME/bands-server limactl copy packages/runtime/src/band-server.ts bands-executor:$VM_HOME/bands-server/server.ts + limactl copy packages/runtime/src/cli-wrapper.ts bands-executor:$VM_HOME/bands-server/cli-wrapper.ts limactl shell bands-executor -- bash -c "cd $VM_HOME/bands-server && $VM_HOME/.bun/bin/bun add hono" # Create systemd user service for the band server limactl shell bands-executor -- mkdir -p $VM_HOME/.config/systemd/user From 9f36459e6152c1cbbbe2f769b56f91317c444e5e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 16:46:28 +0000 Subject: [PATCH 3/3] Restructure cli-wrapper template to avoid regex newline cleanup --- packages/runtime/src/cli-wrapper.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/runtime/src/cli-wrapper.ts b/packages/runtime/src/cli-wrapper.ts index 2191d97..5a4eaaf 100644 --- a/packages/runtime/src/cli-wrapper.ts +++ b/packages/runtime/src/cli-wrapper.ts @@ -28,12 +28,12 @@ export function buildCliWrapperScript( throw new Error(`unsafe cmd name: ${cmd}`); } const logLine = `[ -n "\$BAND_OPS_FILE" ] && echo "${cmd} $*" >> "\$BAND_OPS_FILE"`; + const track = trackLine ? `${trackLine}\n` : ""; if (!hasDeny) { return `#!/bin/bash ${logLine} -${trackLine} -exec ${realPath} "$@" -`.replace(/\n{3,}/g, "\n\n"); +${track}exec ${realPath} "$@" +`; } return `#!/bin/bash FULL_CMD="${cmd} $*" @@ -45,9 +45,8 @@ while IFS= read -r P; do fi done < "\$(dirname "\$0")/.deny-${cmd}" ${logLine} -${trackLine} -exec ${realPath} "$@" -`.replace(/\n{3,}/g, "\n\n"); +${track}exec ${realPath} "$@" +`; } export function buildDenyPatternsFile(patterns: string[]): string {