Fix #25: eval injection in deny.cli wrapper generation

.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

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;
@@ -344,56 +345,34 @@ 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);
     }
   }
 
   const denyByCmd = new Map<string, string[]>();
   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();
     } catch { continue; }
     if (!realPath) continue;
 
     const denyPats = denyByCmd.get(cmd) || [];
-    const logLine = `[ -n "\$BAND_OPS_FILE" ] && echo "${cmd} $*" >> "\$BAND_OPS_FILE"`;
-    const trackLine = trackOps ? buildInsistTracker(cmd) : "";
-
-    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}
-${trackLine}
-exec ${realPath} "$@"
-`;
-    } else {
-      wrapper = `#!/bin/bash
-${logLine}
-${trackLine}
-exec ${realPath} "$@"
-`;
+      writeFile(`${wrapperDir}/.deny-${cmd}`, buildDenyPatternsFile(denyPats));
     }
-
-    writeFile(`${wrapperDir}/${cmd}`, wrapper);
+    const trackLine = trackOps ? buildInsistTracker(cmd) : "";
+    writeFile(`${wrapperDir}/${cmd}`, buildCliWrapperScript(cmd, realPath, denyPats.length > 0, trackLine));
     shell(`chmod +x ${wrapperDir}/${cmd}`);
   }
 }

packages/runtime/src/cli-wrapper.ts

@@ -0,0 +1,54 @@
+/**
+ * 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,
+  trackLine = ""
+): 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"`;
+  const track = trackLine ? `${trackLine}\n` : "";
+  if (!hasDeny) {
+    return `#!/bin/bash
+${logLine}
+${track}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}
+${track}exec ${realPath} "$@"
+`;
+}
+
+export function buildDenyPatternsFile(patterns: string[]): string {
+  return patterns.join("\n") + "\n";
+}

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);
+    }
+  });
+});