Skip to content

config_digest pins the launch target's spelling, not its bytes: a project-local binary/launch_args is rewritable in place #500

Description

@pbean

Split out of #461 point 4 / PR #495, where a reviewer found that config_digest pins the launch target's spelling and nothing verifies its bytes. Sibling of #499 — same fix surface (validate-level confinement on binary), different mechanism: #499 is an unpinned hard-coded token (serve) under the hookless transport, this is a pinned pointer to mutable content under the ordinary hooked transport.

The hole

config_digest hashes binary and launch_args as strings. For a project-local launch target — binary a path into the tree, or binary = "python" with the real program in launch_args — a driven session rewrites the referenced file in place. No config field moves, the digest matches, and the unattended child sweep launches the replacement in the host process.

Nothing forbids or flags the shape today:

  • adapters/profile.py:146-148 requires only a non-empty string for binary; launch_args gets str_list and no path check.
  • The three sibling path fields in the same parser are all containment-guardedhooks.config_path (profile.py:165-170), skill_tree (profile.py:195-196), seed_files (profile.py:203-205) each reject absolute paths and parent refs. The two fields naming the program actually executed are the ones left out.
  • validate's check is shutil.which(p.binary) (cli.py:414-418), which accepts a path happily. probe-adapter already does Path(profile.binary).name (cli.py:2891), i.e. the code tolerates a path-shaped binary by design.

Why the sweep child is genuinely exposed

The obvious dismissal — "the parent loop re-execs the same target every session anyway, so this buys the attacker nothing" — is false for the triage role:

  • ROLES = ("dev", "review", "triage") (runsetup.py:81) and all three adapters are built for every run.
  • Base Engine.__init__ wires only dev+review (engine.py:272-275). sweep.py:391 holds the only adapters["triage"] assignment, and sweep.py:739/855 the only two role="triage" dispatches.
  • [adapter.triage] may name a different profile (policy.py:373, documented in data/settings/core.toml).

So a [adapter.triage] override's launch target is exec'd by a sweep and by nothing else. Two further shapes where no parent session follows the child: sweep.auto = "run-end" fires when _pick_next() returns None (engine.py:773-775), and under isolation = "worktree" parent sessions run at the worktree root while triage runs at the main checkout.

Why a wider hash is the wrong instrument

Content-hashing the launch target would catch the in-place edit — this is not the skill_tree "cheaper route" argument. It fails for different reasons:

  1. The target is not identifiable. Which launch_args token names a file is undecidable (-i vs tools/agent.py); you would be stat-ing argv tokens heuristically.
  2. Digest-time resolution is not launch-time resolution. The digest runs in the orchestrator's Python process; the exec happens in a tmux shell with its own PATH.
  3. One indirection defeats it. This repo's own tests/conftest.py:60-76 write_script_launcher writes a stub that execs sys.executable on a sidecar .py. Hashing the stub misses the payload — exactly the under-catch a lexical rule has.
  4. The target is usually not ours to pin. It is normally a third-party CLI on PATH that self-updates. A mid-run update would move a content hash and burn the auto-sweep trigger for the life of the run: _maybe_auto_sweep appends to sweeps_triggered and _save()s at engine.py:5097, before calling the factory at 5110, with an early return at 5086.

Why the reviewer's second remedy does not decide it either

"Reject launch targets that resolve inside the writable project" is refuted as worded — a bare command name can resolve inside the project. Measured in this repo under an active venv:

PATH[0]: /home/pinkyd/dev/bmad-automator2/.venv/bin
python  -> /home/pinkyd/dev/bmad-automator2/.venv/bin/python

So a lexical rule (reject path-shaped binary) under-catches, and a resolve-and-contain rule is environment-dependent, must run at launch rather than parse, and would reject a legitimate binary = "python" on any machine with a project venv or node_modules/.bin on PATH. Under isolation = "worktree" "the writable project" is ambiguous (main checkout and the linked worktree).

It would also break this repo's own zero-token E2E gates: tests/test_stories_e2e.py:324 sets binary to <sandbox>/.bmad-loop/fake-cli.sh, a project-local path beside the very policy.toml/profiles/ the digest polices (14 tests; plus path-binary adapter constructions in test_generic_tmux.py and test_opencode_http.py).

Scope

  • Unreachable on stock config — all six shipped profiles use bare PATH names with no script path in launch_args.
  • Requires a user-authored project-local profile. SECURITY.md puts "Command or path injection via configuration, profiles, or spawned coding-CLI / tmux sessions" in scope, but "Security issues in user-authored skills, plugins, profiles, or test/lint commands" out of scope, and best-practice docs(roadmap): TEA plugin shipped (experimental) #2 asks users to trust their profiles. This sits on that seam.

Fix space (not decided)

  1. validate warning when the resolved binary (or a launch_args token that exists as a file) lands inside the project tree — cheap, non-breaking, honest, and useful to the user regardless of security. Same instrument opencode-http _serve_argv puts a cwd-relative "serve" at argv[1], so an interpreter binary executes a workspace file #499 settled on as its option 3, so the two should land together.
  2. Containment guard at parse time, matching the three sibling path fields. Stricter, and would break the E2E scaffolds above as written (repairable by moving the fake CLI out of the project root).
  3. Content-pin at enable time — the shape Enabled plugins are trusted by NAME, so a session can swap the module behind one #496 fix-option-2 proposes for plugins. Same objections as above (items 1-4); noted so nobody re-derives it.

Option 1 looks right, for #499's reason: the shape is unsanctioned rather than supported, and telling the human is more useful than a security-framed refusal.

Refs #461, #495, #499.

Metadata

Metadata

Assignees

No one assigned

    Labels

    P3Robustness, enhancement, tests, or docs worth schedulingarea:adaptersCoding-CLI adapters and profilesarea:configBMAD config resolution and the setup skillbugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions