Skip to content

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

Merged
mrjf merged 4 commits into
mainfrom
fix/cli-deny-eval-injection
May 16, 2026
Merged

Fix #25: eval injection in deny.cli wrapper generation#39
mrjf merged 4 commits into
mainfrom
fix/cli-deny-eval-injection

Conversation

@mrjf

@mrjf mrjf commented May 16, 2026

Copy link
Copy Markdown
Owner

Fixes #25.

What broke

The CLI deny-wrapper generator embedded user-authored deny.cli patterns into a bash array literal and matched them with eval. A pattern like foo$(id)* would execute at wrapper-generation time (bash array assignment performs command substitution) or at match time (eval re-parses the value of $P).

The attacker is anyone who can author a BAND.md the system loads. Code runs as the host VM user, before sandboxing — privilege escalation inside the VM.

Fix

  • Patterns are written to a side file, one per line, and read at run time with read -r, which does not perform parameter or command substitution.
  • 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 rather than executed.
  • Command names (first word of allow/deny patterns) must match a conservative character class. Values containing shell metacharacters are dropped at parse time, which also closes a separate injection point in the which/readlink shell call.
  • 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.

Tests

New unit tests in packages/runtime/test/unit/band-server-cli-wrappers.test.ts verify:

  • The generated wrapper contains neither eval nor an embedded DENY_PATTERNS=(...) literal
  • Patterns are read from .deny-<cmd> at run time
  • Unsafe command names ($(id), backticks, semicolons, spaces, ../) throw
  • Conventional command names (git-lfs, node18, a.out) are accepted
  • The deny patterns file emits patterns verbatim, one per line — no escaping needed

All 278 existing unit tests still pass; typecheck clean.

🤖 Generated with Claude Code

mrjf and others added 2 commits May 16, 2026 07:18
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@mrjf

mrjf commented May 16, 2026

Copy link
Copy Markdown
Owner Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented May 16, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved. The only conflict was in packages/runtime/src/band-server.ts — origin/main had added insist tracking (trackOps/buildInsistTracker) using the old inline wrapper approach, while our branch had replaced that with the secure buildCliWrapperScript. I updated buildCliWrapperScript to accept an optional trackLine parameter so both features work together, then merged. Typecheck clean, 282 unit tests pass.

Copilot finished work on behalf of mrjf May 16, 2026 16:46
@mrjf
mrjf merged commit 88ba4f6 into main May 16, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Critical: eval injection in deny.cli wrapper generation

2 participants