fix: eliminate command injection in elevated install/uninstall paths#79
Merged
Conversation
file.ts previously built shell command strings (via template literals) for every installer format and passed them to execSync/sudo-prompt, which both invoke a shell. Since installer file paths and the elevated CLI's slug/version/type arguments ultimately originate from community-submitted registry metadata or local project files, this was a command injection vector leading to arbitrary code execution as root/admin - a malicious `file.url` or project dependency slug could break out of the quoted string via `$(...)`, backticks, `;`, `|`, etc. - fileInstall/fileOpen/dirOpen: replaced execSync + string interpolation with execFileSync + argument arrays for every branch (.dmg/.pkg/.deb/.rpm/.exe/.msi and open/xdg-open/start). execFileSync never invokes a shell, so argument content can't be interpreted as shell syntax regardless of what characters it contains. - .dmg install now mounts to an explicit, freshly created mountpoint and searches only inside it for a .pkg, rather than scanning all of /Volumes for whatever shows up (also closes an ambient-authority/ TOCTOU gap where a concurrently mounted, unrelated volume could be picked up instead). - runCliAsAdmin: sudo-prompt's exec() only accepts a single command string, so there's no argv-array option at that layer. Switched to base64url-encoding a JSON payload of the dynamic fields (appDir/type/id/version/log) before interpolating - base64url's alphabet is only [A-Za-z0-9_-], so the shell never sees characters that could be interpreted as syntax, regardless of payload content. admin.ts decodes --payload; the old individual --flag form is kept only for manual/developer invocation, which never carries untrusted data. - ManagerLocal: added isValidSlug()/isValidVersion() guards at the top of install()/uninstall(), the single choke point every other path (installAll, installDependency(ies), uninstallDependency(ies)) routes through - including installAll() -> sync(), where slug comes directly from an unvalidated remote registry JSON key with no user action required beyond running "install all". Verified: build, lint, and full test suite (165/165) all pass unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… slip) archiveExtract() had two paths with no containment check on where an archive entry actually lands relative to the intended extraction directory: - The zip Windows-fallback loop (used when adm-zip's own extractAllTo throws ENOENT due to special characters in filenames) built destination paths by hand from entry.entryName, stripping `<>:"|?*`/newlines but never `..` - a crafted entry name like `../../../etc/passwd` would write outside the target directory. - The .7z path shells out to the 7za binary via 7zip-min with no per-entry containment logic at all, and there's no way to sanitize a destination mid-extraction. Both are now guarded by a new isSafeArchiveEntryPath() helper (built on dirContains(), which existed but was unused in any production code path): the zip fallback now throws and aborts the whole extraction if any entry would escape, and the .7z path lists the archive's contents via 7zip-min's list() API and refuses to extract at all if any entry would escape. Verified rather than assumed that the other two extraction paths don't need the same treatment: adm-zip 0.5.18's extractAllTo already sanitizes entries internally (falls back to the entry's basename if it would otherwise escape), and node-tar 7.5.20 rejects '..' segments and relativizes absolute paths by default. Both left untouched, with comments documenting why. Also tightened dirContains() itself, since it's now load-bearing: it previously used a plain startsWith() prefix check, which would incorrectly treat a sibling directory sharing a prefix (e.g. "/foo/bar" vs "/foo/barbaz") as contained. Added direct unit coverage for isSafeArchiveEntryPath and a regression test for the dirContains fix, rather than constructing malicious zip/7z fixtures - both adm-zip and node-tar sanitize entry names on write, making a genuinely malicious fixture impractical to build. Verified: build, lint, and full test suite (167/167, up from 165) all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
fileInstall/fileOpen/dirOpen/runCliAsAdminbuilt shell command strings via template literals and ran them throughexecSync/sudo-prompt(both of which invoke a shell). Installer file paths and the elevated CLI'sslug/version/typearguments ultimately come from community-submitted registry metadata or local project files, so a craftedfile.urlor dependency slug containing$(...), backticks,;,|etc. could break out of the quoted string and execute arbitrary commands as root/admin during install/uninstall.Fix
fileInstall/fileOpen/dirOpen: replacedexecSync+ string interpolation withexecFileSync+ argument arrays for every branch (.dmg/.pkg/.deb/.rpm/.exe/.msi,open/xdg-open/start).execFileSyncnever invokes a shell, so argument content can't be interpreted as shell syntax regardless of what it contains..dmginstall now mounts to an explicit, freshly created mountpoint and searches only inside it for a.pkg, instead of scanning all of/Volumesfor whatever shows up — also closes an ambient-authority/TOCTOU gap where a concurrently mounted, unrelated volume could be picked up instead.runCliAsAdmin:sudo-prompt'sexec()only accepts a single command string (no argv-array option at that layer), so the dynamic fields (appDir/type/id/version/log) are now base64url-encoded into a JSON payload before being interpolated. Base64url's alphabet is only[A-Za-z0-9_-], so the shell never sees characters that could be syntax, regardless of payload content.admin.tsdecodes--payload; the old individual--flagform is kept only for manual/developer invocation, which never carries untrusted data.ManagerLocal: addedisValidSlug()/isValidVersion()guards at the top ofinstall()/uninstall()— the single choke point every other path (installAll,installDependency(ies),uninstallDependency(ies)) routes through, includinginstallAll() → sync(), whereslugcomes directly from an unvalidated remote registry JSON key with no user action required beyond running "install all."Scope note
Deliberately did not add character restrictions on
file.urlat the Zod/registry-ingestion layer (a defense-in-depth idea considered alongside this). Checked it against the real registry data first and found it would break 9 currently-published packages (URLs containing+,%2B, and?id=-style query strings). Since the fix above already makes URL content irrelevant to exec-safety, that would be a separate hygiene improvement with real regression risk, not required here.Test plan
npm run build— type-checks cleannpm test— 165/165 passing, unchangednpm run lint— cleannpx prettier --check— clean🤖 Generated with Claude Code