fix(cli): prevent directory traversal in skill file installation#2235
Conversation
installSkillFiles() used path.join() with unsanitized file paths from GitHub API responses. A malicious skill repository could include files with "../" sequences in their paths, allowing writes outside the intended skill directory. Changes: - installer.ts: use path.resolve() and verify the resolved path stays within the skill directory boundary before writing - github.ts: reject file paths containing ".." at download time as an additional defense-in-depth check
|
Hi @fahreddinozcan, Now that the fix has been merged and released for a few weeks, I'd
I attempted to file a draft advisory at Two options: Option 1 (easier for you): You (as maintainer) file the advisory Option 2: Enable Private Vulnerability Reporting in repo Settings For this specific issue, Option 1 is simpler. I can share the full Details I already have ready:
Thanks! |
Summary
Fixes #2234
path.join()withpath.resolve()and add a boundary check that ensures the resolved file path stays within the skill directory. Throws an error if a path would escape the boundary...at download time, before they reach the installer.Vulnerability Details
installSkillFiles()usedpath.join(skillDir, file.path)wherefile.pathcomes from the GitHub API tree response. A malicious skill repository could include files with../path sequences, causingpath.join()to resolve to locations outside the intended skill directory. This allowed arbitrary file writes as the current user.Changes
packages/cli/src/utils/installer.tsresolve()+ boundary check beforewriteFile()packages/cli/src/utils/github.ts..at download timeTest Plan
npx tsc --noEmit -p packages/cli/tsconfig.jsonpasses with zero errorspath.resolve("/base/skill", "../../../etc/passwd")→/etc/passwdwhich is caught by thestartsWithguardsrc/index.tsstill install correctlyFound by SpiderShield security scanner