Skip to content

fix(interpreter): meter resource usage for $(<file) command substitutions - #2469

Merged
chaliy merged 2 commits into
mainfrom
2026-09-25-fix-file-command-execution-bypass
Sep 25, 2026
Merged

chaliy merged 2 commits into
mainfrom
2026-09-25-fix-file-command-execution-bypass

Conversation

@chaliy

@chaliy chaliy commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • An optimized $(<file) path read files directly and bypassed execute_command, skipping execution_budget.consume_work(1), counters.tick_command, session counters, and live-intermediate byte accounting and enabling repeated file reads to evade limits and inflate memory.
  • The change restores consistent resource accounting so file-backed command substitutions cannot circumvent per-exec, session, work-unit, or live-intermediate budgets.

Description

  • Add a shared gate charge_command_execution() that calls execution_budget.consume_work(1), counters.tick_command(...), and session limit checks, and use it at the normal command entry and in the optimized $(<file) fast path (interpreter: charge_command_execution, execute_cmd_subst).
  • Replace ad-hoc String growth in command-substitution with BudgetedString and acquire ExecutionBudget live-byte leases for file contents and substitution text so allocations are charged to lease_bytes while text is live (files: process_input_redirections usage; cmdsubst & expansion: BudgetedString, try_push_str, lease_bytes).
  • Hold sibling-substitution live-byte leases during word expansion to prevent sibling substitutions from each reusing the same live budget (changes in expand_word_inner).
  • Add focused security/regression tests execution_budget_tests::command_substitution_file_read_* to assert per-exec command limits, session-command limits, work-unit limits, and live-intermediate byte limits reject the crafted workloads, and update the threat model docs with TM-DOS-111 and knowledge/log entries.

Testing

  • Ran cargo test -p bashkit --test integration execution_budget_tests::command_substitution_file_read -- --nocapture and the full execution_budget_tests suite, and all new and affected tests passed.
  • Ran cargo test -p bashkit --test integration threat_model_doc_tests::public_threat_model_doc_covers_every_spec_threat_id, cargo clippy -p bashkit --tests -- -D warnings, cargo test -p bashkit --test integration, and just pre-pr; after installing PyYAML for the environment, these checks completed successfully.
  • Ran cargo bench -p bashkit --bench parallel_execution -- --noplot and just check-okf; benchmarks and OKF checks completed without regressions.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit 14f036e Commit Preview URL Sep 25 2026, 05:29 PM

The budget charge for a command substitution hand-duplicated the escape
set from quote_expansion_for_quoted_glob purely to predict how many bytes
append_expansion_for_word would write. The two agreed, but only by
coincidence of being edited together: adding a metacharacter to the
quoting function would silently under-charge the live-byte lease and stop
it bounding the substitution.

Extract needs_glob_escape as the one definition, derive both the quoting
and the byte count from it, and add a test asserting the charge equals
what append actually writes across quoted/glob combinations, metacharacter
runs, and multi-byte input.

Renumber the threat row to TM-DOS-115: TM-DOS-111 is now assigned to
silent scalar assignment rejection (#2466), and 112-114 are taken.

Claude-Session: https://claude.ai/code/session_01MJBT5na4uL5yZZXwwH1FMy
@chaliy
chaliy force-pushed the 2026-09-25-fix-file-command-execution-bypass branch from f1f33d3 to 14f036e Compare September 25, 2026 17:28

chaliy commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed, rebased onto latest main, and pushed two fixes. Charging the shared command-execution gate from the $(<file) shortcut is the right root cause — an optimisation should replace execution, not its accounting.

1. Duplicate threat ID (blocking)

The new row was numbered TM-DOS-111, which #2466 took while this PR was open (silent scalar assignment rejection). 112, 113 and 114 also landed since. Renumbered to TM-DOS-115 in both threat-model files and knowledge/log.md.

The threat-model conflicts from the rebase resolved to main's table plus this row; no other content was affected.

2. The byte charge duplicated the escape set (blocking)

appended_bytes inlined a copy of the 13-character escape set from quote_expansion_for_quoted_glob, purely to predict how many bytes append_expansion_for_word would write:

let appended_bytes = if word.quoted && word.has_unquoted_glob {
    trimmed.chars().map(|ch| if matches!(ch, '\\' | '*' | '?' | ... ) { ch.len_utf8() + 1 } ...

The two sets do agree today — I diffed them character by character. But they agree only by having been written together. Add a metacharacter to the quoting function and this charge silently under-counts, and a silent under-charge in live-byte accounting is exactly the bound this PR exists to enforce. Nothing would fail; the lease would just stop covering the bytes.

Fix: extracted needs_glob_escape as the one definition, derived both the quoting and the length from it, and replaced the inlined block with expansion_appended_len(word, value). This also drops a redundant pass — the old code iterated once to count and quote_expansion_for_quoted_glob iterated again to build.

Then pinned the invariant so it cannot regress silently:

assert_eq!(
    Interpreter::expansion_appended_len(&w, value),
    appended.len(),   // from the real append_expansion_for_word
);

across both quoted × has_unquoted_glob combinations, over plain text, a full metacharacter run, pre-escaped backslashes, a trailing backslash, and multi-byte input (héllo → 世界 *) — so the UTF-8 arithmetic is covered too, not just ASCII.

Validation

  • integration 1454 passed, 0 failed — including all five new command_substitution_file_read_* tests
  • lib 2944 passed, 0 failed, including the new invariant test
  • cargo fmt --check clean; cargo clippy -p bashkit --lib --tests --features http_client,ssh,sqlite -- -D warnings clean
  • just check-okf conformant; just check-doc-links OK
  • smoke: echo hi > /f; x=$(</f); echo "got:$x" → got:hi
  • rebased onto main @ a4fc5b3

Generated by Claude Code

@chaliy
chaliy merged commit eb5032f into main Sep 25, 2026
47 checks passed
@chaliy
chaliy deleted the 2026-09-25-fix-file-command-execution-bypass branch September 25, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant