fix(cli): close shell injection via crafted staged filenames (security, #87) - #98
Merged
Merged
Conversation
stagedContent() built a shell command string via execSync(`git show
":${path}"`) with path taken directly from stagedFiles() -- i.e. the
literal name of a staged file, fully controlled by whoever authored
the commit. git's core.quotepath escapes an embedded `"` or `\`, but
not backticks or $(...), so a filename like `` x`touch pwned.txt`.ts ``
ran as shell syntax. Anyone running `harness check` as a pre-commit
hook (the README's own documented usage) against an untrusted staged
file list -- e.g. right after `git add .` on a checked-out
contributor branch -- got arbitrary command execution.
Both stagedContent() and stagedFiles() now use execFileSync with an
argv array; the filename reaches `git show` as a single literal
argument and is never parsed by a shell, regardless of its content.
stagedFiles() had no interpolated input (its args were already
literal) but is switched for consistency and defense in depth.
test/staged-content-shell-injection.test.ts exercises the real
stagedContent() against an isolated git repo (mkdtempSync, cleaned up
in a finally block) with a backtick-laden staged filename -- proven
discriminating: it fails against the vulnerable execSync form (the
shell eats the backticks before git ever sees them) and passes against
the fix.
All 9 execution sites in src/ were audited for the same shape; this
was the only exploitable one.
Reported in #87 by @VikramNehreTR, with reproduction and root-cause
analysis (including why core.quotepath doesn't help) and the exact fix
adopted here.
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
harness check, reported in Arbitrary command execution via crafted staged filenames inharness check#87 by @VikramNehreTR.stagedContent()/stagedFiles()(src/cli/run.ts) now useexecFileSyncwith an argv array instead of an interpolated shell string — a staged filename can no longer reach a shell, regardless of its content (backticks,$(...), etc.).test/staged-content-shell-injection.test.ts) proven discriminating: fails on the vulnerableexecSyncform, passes on the fix.src/audited for the same shape; this was the only exploitable one.Relates to #87 — issue will be closed manually once the npm publish for this version is confirmed, not automatically on merge.
Test plan
tsc --noEmitcleanbun test: 1095 pass / 1 skip / 0 fail (baseline 1094, +1 new test)