fix(utils): fix sibling-directory escape in materializeFiles path check - #603
Conversation
materializeFiles guarded against escaping its target directory with
`fullPath.startsWith(resolvedBaseDir)`, a path-separator-unaware prefix match:
a relative name like `../<dir>-evil/x` resolves outside the target directory
but still starts with the same string, so it passed the check. Since the
resulting directories are created with `fs.mkdir(..., {recursive: true})`, this
let a caller-supplied file name write outside the intended directory as long as
its resolved sibling path happened to share the base directory's name as a
prefix.
materializeFiles is reachable from run_skill_script_tool /
run_skill_inline_script_tool with output files from a configured
CodeExecutor, including AgentEngineSandboxCodeExecutor, which decodes the
file name from the remote sandbox's own execution-result metadata; a caller
with no other privileges beyond choosing that file name could reach a
directory outside the one materializeFiles was scoped to.
Require a path-separator boundary (or exact equality) instead of a bare
prefix match, matching the containment check already used in
FileArtifactService's assertInsideRoot. Add a regression test for the specific
gap: an escape into a sibling directory that shares a name prefix with the
target directory, which the existing `../escape.txt`-style test does not
exercise (mkdtemp's random suffix means that case never collides with a real
sibling name).
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Fix is correct and the test is a real regression test — it fails on the old check, since /tmp/x-evil/f.txt genuinely does start with /tmp/x. I swept the repo: these two lines were the only separator-unaware containment checks left (skills/loader.ts:364 already normalizes its prefix with a trailing /), and nothing depends on the old lenient behavior — all three materializeFiles callers mock it in tests. One optional note below on duplication. Worth knowing, not asking you to fix it here: neither this check nor assertInsideRoot resolves symlinks, so a symlink planted inside the target dir still escapes.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Head is unchanged (3f3062bc) — nothing to re-verify, and the one comment I left was optional. Agreed on the follow-up: keep isInsideDir in utils/ and have file_artifact_service import it, in a separate PR. Don't fold it in here.
CI is green now that the workflow runs are released. LGTM.
2 participants
materializeFiles (core/src/utils/file_utils.ts) guards its target directory with
fullPath.startsWith(resolvedBaseDir)— a path-separator-unaware prefix match. A relative file name like../<dir>-evil/xresolves to a sibling of the target directory but still satisfies that check, sinceresolvedBaseDirand the sibling path share the same string prefix with no separator required between them. Because the resulting directory is created withfs.mkdir(path.dirname(finalPath), {recursive: true}), the sibling directory does not need to already exist.materializeFilesis called with nodirargument (defaulting toprocess.cwd()) fromrun_skill_script_tool.tsandrun_skill_inline_script_tool.tsonresult.outputFiles, which come back from whateverCodeExecutoris configured — includingAgentEngineSandboxCodeExecutor, which decodes each output file's name directly from the remote sandbox execution result's metadata (attributes['file_name'], base64-decoded, unvalidated). So a file name chosen by code running inside that isolated sandbox can, via this gap, land outside the directorymaterializeFileswas scoped to on the orchestrator host.Confirmed by executing the real function: with target dir
/tmp/x/sandboxdir, an output file named../sandboxdir-pwned/shell.jswas written to/tmp/x/sandboxdir-pwned/shell.js, outside the intended directory.This replaces the check with a helper requiring a path-separator boundary (or exact equality) — the same containment pattern already used by
FileArtifactService.assertInsideRoot(added in #210 for a related but distinct traversal in that file). Adds a regression test for exactly this gap: the existing../escape.txt-style test usesfs.mkdtemp's randomly-suffixed directory name, so it never happens to collide with a real sibling name and does not exercise this path.