Skip to content

fix(sed): bound output growth during execution - #2455

Merged
chaliy merged 3 commits into
mainfrom
2026-09-25-propose-fix-for-sed-command-vulnerability
Sep 25, 2026
Merged

chaliy merged 3 commits into
mainfrom
2026-09-25-propose-fix-for-sed-command-vulnerability

Conversation

@chaliy

@chaliy chaliy commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • The new r/R implementation could append a preloaded file/stdin after every selected input line and materialize quadratic output in an unbounded String, allowing attacker-controlled stdin and script text to exhaust host memory synchronously.
  • Existing SED_MAX_CYCLE_STEPS and post-return max_stdout_bytes checks did not prevent large transient allocations inside the sed engine.
  • The goal is to make sed destination-aware and enforce configured output and live-intermediate budgets while preserving normal, below-limit behavior.

Description

  • Replace sed's unbounded String sinks with a budget-aware Sink that uses BudgetedString, tracks an optional max_bytes destination limit, and records a LimitExceeded error on overrun so further accumulation stops early.
  • Make Machine::run_segment accept an optional max_bytes destination budget and return Result<String, LimitExceeded> so callers can stop execution on allocation errors instead of materializing huge results.
  • Wire caller-side limits: read ExecutionLimits::max_stdout_bytes and the shared ExecutionBudget from Context, compute per-destination remaining bytes for the current stdout accumulator, and pass that to run_segment; file/in-place sinks lease from the live-intermediate budget instead.
  • Propagate sink allocation failures into controlled sed errors before any large writes or in-place rewrites, and check write_files sinks for errors before writing to the VFS.
  • Add integration tests sed_resource_tests that verify repeated r /dev/stdin is stopped under a small max_stdout_bytes while below-limit output remains unchanged, and update the threat model with TM-DOS-109 describing the mitigation.

Testing

  • Ran unit tests for the sed crate: cargo test -p bashkit sed::tests --lib (passed).
  • Ran focused integration tests: cargo test -p bashkit --test integration sed_resource_tests (2 tests passed).
  • Ran format and lint checks: cargo fmt --all -- --check and cargo clippy -p bashkit --lib --tests -- -D warnings (both passed).
  • Ran repository checks: just check-okf and git diff --check (passed).

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 06de33a Commit Preview URL Sep 25 2026, 11:35 AM

The destination cap derived from max_stdout_bytes applied the stdout
*capture* limit to data that never reaches captured stdout. sed output
piped to another command or redirected to a file was refused once it
passed 1 MB, so ordinary transformations returned exit 1 with no output:

  seq 1 100000 | sed 's/$/<20 chars>/' > /out.txt   # 0 bytes, exit 1

Drop the destination cap and keep the BudgetedString lease against
max_live_intermediate_bytes, which is the limit that actually bounds
transient growth inside the builtin (the same layer awk, jq and zip use).
The r-amplification attack is still refused at the budget; 2.6 MB of
ordinary sed output through a pipe or into a file works again.

Renumber the threat row to TM-DOS-112: TM-DOS-109 is already assigned to
silent truncation at builtin caps.

Claude-Session: https://claude.ai/code/session_01MJBT5na4uL5yZZXwwH1FMy
@chaliy
chaliy force-pushed the 2026-09-25-propose-fix-for-sed-command-vulnerability branch from 2f7124e to 2c77530 Compare September 25, 2026 10:26

chaliy commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed, rebased onto latest main, and pushed two fixes. The budgeted-sink approach is right; the limit it was wired to was not.

1. The destination cap was the wrong limit (blocking)

max_stdout_bytes bounds what the interpreter captures as stdout. sed's output frequently never becomes captured stdout — it gets piped to the next stage, or redirected to a file. Deriving the sink cap from it meant any sed output over 1 MB (the default) failed with exit 1 and zero output.

Measured on this branch as pushed, versus main, with 2.6 MB of output (under the 10 MB VFS cap, over the 1 MB stdout cap):

2.6 MB sed output main this branch (before fix)
seq 1 100000 | sed 's/$/…/' > /out.txt rc=0, 2,588,895 bytes rc=1, 0 bytes
seq 1 100000 | sed 's/$/…/' | wc -c 2588895 0 + error

Both are ordinary transformations, and the file-redirect case silently left an empty file while the script carried on — data loss, not a bounded refusal.

Fix: dropped the destination cap and kept the BudgetedString lease against max_live_intermediate_bytes. That is the limit that actually bounds transient growth inside a builtin, and it is the same layer awk, jq and zip already use. The stdout capture cap keeps doing its own job at the interpreter boundary.

The amplification this PR set out to stop is still refused:

$ sed 'r /in.txt' /in.txt | wc -c      # 64 KB input, quadratic output
resource limit exceeded: execution budget exhausted:
  live intermediate bytes (32783888 requested, max 32000000)

…and the 2.6 MB cases above now match main byte-for-byte.

2. Duplicate threat ID (blocking)

The new row was numbered TM-DOS-109, which is already assigned to "Silent truncation at builtin caps". The highest ID in use is TM-DOS-110, and #2466 takes TM-DOS-111, so this is now TM-DOS-112. Its wording also records why the stdout cap is deliberately not a sink limit, so this does not get re-introduced.

3. Tests

sed_resource_tests asserted the behaviour that caused the regression, so it was rewritten to pin both halves of the contract:

  • the r amplification is refused with a bounded diagnostic and nonzero status (either layer may trip first — that is an implementation detail, so the test accepts both);
  • output over max_stdout_bytes still flows intact through a pipe and into a file — the two regressions above, now locked down.

Validation

  • 36 sed tests pass, including sed_spec_tests (GNU differential) and the unicode boundary suite
  • cargo fmt --check clean; cargo clippy -p bashkit --lib --tests --features http_client,ssh,sqlite -- -D warnings clean
  • rebased onto main @ 1308956; end-to-end re-verified against a main build

Generated by Claude Code

The spec-vs-public parity test requires every knowledge/ TM ID to appear
in crates/bashkit/docs/threat-model.md.

Claude-Session: https://claude.ai/code/session_01MJBT5na4uL5yZZXwwH1FMy
@chaliy
chaliy merged commit 0eb7fae into main Sep 25, 2026
47 checks passed
@chaliy
chaliy deleted the 2026-09-25-propose-fix-for-sed-command-vulnerability branch September 25, 2026 16:30
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