T09 · Insecure Skill Coding Practices
Warning
- Location
- bin/install.js:128
- Finding
- JavaScript Installer Follows Pre-Existing Destination Symlinks<![CDATA[ ## Vulnerability Details **File Location**: `bin/install.js:128-155` **Vulnerability Type**: Unsafe destination path and symlink handling **Risk Level**: Medium ### Vulnerable Code ```javascript function copyDir(src, dst, skip) { for (const entry of fs.readdirSync(src, { withFileTypes: true })) { if (skip.has(entry.name)) continue; const from = path.join(src, entry.name); const to = path.join(dst, entry.name); try { if (entry.isDirectory()) { fs.mkdirSync(to, { recursive: true }); copyDir(from, to, skip); } else if (entry.isFile()) { fs.copyFileSync(from, to); } } catch (err) { throw new InstallError('Failed to copy ' + from + ' -> ' + to + ': ' + err.message); } } } function installTo(dest) { if (!dest || typeof dest !== 'string') throw new UsageError('Destination directory is required'); const target = path.resolve(dest, SKILL_NAME); assertSafeTarget(target); try { fs.mkdirSync(target, { recursive: true }); copyDir(PKG_ROOT, target, COPY_SKIP); if (!fs.existsSync(path.join(target, 'SKILL.md'))) { throw new InstallError('Installed directory is missing SKILL.md'); } ``` ### Technical Analysis The installer creates and writes to the target using path-based filesystem operations without checking whether the target directory or any existing destination entry is a symbolic link. `fs.mkdirSync(..., { recursive: true })` accepts an existing directory symlink, while subsequent `fs.copyFileSync` calls can follow destination symlinks. The `assertSafeTarget` function only performs a lexical comparison against the package source directory. It does not canonicalize the destination with `fs.realpathSync`, inspect components with `fs.lstatSync`, or verify that the resolved target remains within the user-selected skills directory. Consequently, an attacker who can prepare entries in the destination directory can redirect installation writes outside the int ...[truncated 1331 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Resolve the destination parent using `fs.realpathSync` and verify that the final target remains beneath the intended skills directory. 2. Inspect every existing destination component with `fs.lstatSync` and reject symbolic links. 3. Reject a pre-existing target unless it is a verified directory owned or trusted by the current user. 4. Copy into a newly created temporary sibling directory using exclusive creation, then atomically rename it into place. 5. Avoid overwriting existing files without explicit user confirmation or a verified upgrade mode. 6. Add tests covering: - A symlink at the complete target path. - Symlinked files inside an existing target. - Symlinked intermediate path components. - Attempts to install into or through the package source directory. ]]>