security: pass cwd to git via execFileSync, not interpolation through /bin/sh#1368
Open
garagon wants to merge 1 commit intogarrytan:mainfrom
Open
security: pass cwd to git via execFileSync, not interpolation through /bin/sh#1368garagon wants to merge 1 commit intogarrytan:mainfrom
garagon wants to merge 1 commit intogarrytan:mainfrom
Conversation
… /bin/sh
`bin/gstack-memory-ingest.ts:632-643` ran `execSync(\`git -C ${JSON.stringify(cwd)}
remote get-url origin 2>/dev/null\`, ...)`. JSON.stringify escapes `"` and `\`
but not `$` or backticks, so a `cwd` of `"$(touch /tmp/marker)"` survived JSON
quoting and detonated under /bin/sh's command-substitution-inside-double-quotes.
`cwd` originates from transcript JSONL records under
`~/.claude/projects/<encoded-cwd>/<uuid>.jsonl` and
`~/.codex/sessions/YYYY/MM/DD/rollout-*.jsonl`. The walker grabs the first
`.cwd` it sees per session. That's an untrusted surface in the gstack threat
model — the L1-L6 sidebar security stack exists exactly because agent
transcripts can carry attacker-influenced text. Two pivots above the local
same-uid bar: (a) prompt-injection appending `cwd="$(...)"` to the active
session log turns the next /sync-gbrain run into RCE under the user's uid;
(b) cross-machine transcript share (a colleague's `.claude/projects` snippet
untar'd into HOME, a documented gbrain dogfooding shape) → RCE on first sync.
Fix swaps the one execSync for `execFileSync("git", ["-C", cwd, "remote",
"get-url", "origin"], ...)`. No shell, argv passed directly to git. The same
module already uses execFileSync for `gbrainAvailable()` (line 762 pre-patch)
and `gbrainPutPage()` (line 816 pre-patch) — this single execSync was the
outlier.
Test: `gstack-memory-ingest security: untrusted cwd cannot trigger shell
substitution` plants a Claude-Code-shaped JSONL with cwd=`$(touch <marker>)`
and asserts the marker file is not created after `--incremental --quiet`.
Negative control: with the patch reverted, the test fails (marker created);
with the patch applied, it passes (18/18 in test/gstack-memory-ingest.test.ts).
Merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bin/gstack-memory-ingest.ts:632-643ran:JSON.stringifyescapes"and\but not$or backticks, so acwdof"$(touch /tmp/marker)"survives JSON quoting and detonates under/bin/sh's command-substitution-inside-double-quotes.cwdoriginates from transcript JSONL records under~/.claude/projects/<encoded-cwd>/<uuid>.jsonland~/.codex/sessions/YYYY/MM/DD/rollout-*.jsonl. The walker grabs the first.cwdit sees per session — agent transcripts are an explicit untrusted surface in the gstack threat model, which is what the L1-L6 sidebar security stack exists for.Two pivots above the local same-uid bar:
/sync-gbraininto RCE under the user's uid (HOME, ssh keys, ngrok creds, gbrain bearers, etc.)..claude/projectssnippet untar'd into a peer's HOME (a documented gbrain dogfooding pattern) → RCE on first sync.gstack-memory-ingest --incrementalis stage 2 ofgstack-gbrain-sync.ts:runMemoryIngest(lines 407-433), so any/sync-gbrainrun triggers it.Fix
Swap the one
execSyncforexecFileSync("git", ["-C", cwd, "remote", "get-url", "origin"], ...). No shell, argv passed directly to git. The same module already usesexecFileSyncforgbrainAvailable()(line 755) andgbrainPutPage()(line 816) — this singleexecSyncwas the outlier.Test
New regression test
gstack-memory-ingest security: untrusted cwd cannot trigger shell substitutionplants a Claude-Code-shaped JSONL withcwd="$(touch <marker>)"and asserts the marker file is not created after--incremental --quiet.Negative control: stash the patch, run the same test, it fails (marker file gets created on disk). Restore the patch, it passes.
Test plan
bun test test/gstack-memory-ingest.test.tsgreen (18/18)