From bca5667fc65517b0a328ae3f0746fee274c059bf Mon Sep 17 00:00:00 2001 From: Om Patel Date: Mon, 15 Jun 2026 11:39:46 -0400 Subject: [PATCH] =?UTF-8?q?fix(cli):=20prevent=20path=20traversal=20vulner?= =?UTF-8?q?abilities=20during=20skill=20install=E2=80=A6=20(#27767)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/cli/src/utils/skillUtils.test.ts | 131 ++++++++++++++++++++++ packages/cli/src/utils/skillUtils.ts | 61 +++++++--- 2 files changed, 178 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/utils/skillUtils.test.ts b/packages/cli/src/utils/skillUtils.test.ts index 449b61c5b2..db1bb6b2f6 100644 --- a/packages/cli/src/utils/skillUtils.test.ts +++ b/packages/cli/src/utils/skillUtils.test.ts @@ -267,5 +267,136 @@ describe('skillUtils', () => { const exists = await fs.stat(skillDir).catch(() => null); expect(exists).toBeNull(); }); + + it('should prevent path traversal in fallback uninstallation (e.g. sibling directories)', async () => { + const skillsDir = path.join(tempDir, '.gemini/skills'); + await fs.mkdir(skillsDir, { recursive: true }); + + const siblingDir = path.join(tempDir, '.gemini/skills-attacker'); + await fs.mkdir(siblingDir, { recursive: true }); + + // Attempt to uninstall the sibling directory using path traversal + const result = await uninstallSkill('../skills-attacker', 'user'); + expect(result).toBeNull(); + + // Verify sibling directory is NOT deleted + const exists = await fs.stat(siblingDir).catch(() => null); + expect(exists).not.toBeNull(); + }); + + it('should prevent path traversal in fallback uninstallation with dot or dot dot', async () => { + expect(await uninstallSkill('..', 'user')).toBeNull(); + expect(await uninstallSkill('.', 'user')).toBeNull(); + expect(await uninstallSkill('', 'user')).toBeNull(); + }); + }); + + describe('path traversal prevention', () => { + it('should throw error during installation if skill name is dot dot or dot', async () => { + const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source'); + const skillSubDir = path.join(mockSkillSourceDir, 'test-skill'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile( + path.join(skillSubDir, 'SKILL.md'), + '---\nname: ..\ndescription: exploit\n---\nbody', + ); + + await expect( + installSkill(mockSkillSourceDir, 'workspace', undefined, () => {}), + ).rejects.toThrow('Invalid skill name: Path traversal detected.'); + }); + + it('should throw error during linking if skill name is dot dot or dot', async () => { + const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source'); + const skillSubDir = path.join(mockSkillSourceDir, 'test-skill'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile( + path.join(skillSubDir, 'SKILL.md'), + '---\nname: ..\ndescription: exploit\n---\nbody', + ); + + await expect( + linkSkill(mockSkillSourceDir, 'workspace', () => {}), + ).rejects.toThrow('Invalid skill name: Path traversal detected.'); + }); + + it('should throw error during installation if subpath escapes temp directory', async () => { + const skillPath = path.join(projectRoot, 'weather-skill.skill'); + const exists = await fs.stat(skillPath).catch(() => null); + if (!exists) return; + + await expect( + installSkill(skillPath, 'workspace', '../escape', () => {}), + ).rejects.toThrow('Invalid path: Directory traversal not allowed.'); + }); + + it('should sanitize absolute path names and install them safely within the target directory', async () => { + const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source'); + const skillSubDir = path.join(mockSkillSourceDir, 'test-skill'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile( + path.join(skillSubDir, 'SKILL.md'), + '---\nname: /tmp/exploit\ndescription: exploit\n---\nbody', + ); + + const installed = await installSkill( + mockSkillSourceDir, + 'workspace', + undefined, + () => {}, + ); + expect(installed.length).toBe(1); + expect(installed[0].name).toBe('-tmp-exploit'); + + const destPath = installed[0].location; + const resolvedTarget = path.resolve(tempDir, '.gemini/skills'); + expect(destPath.startsWith(resolvedTarget + path.sep)).toBe(true); + }); + + it('should sanitize traversal names with spaces and install them safely within the target directory', async () => { + const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source'); + const skillSubDir = path.join(mockSkillSourceDir, 'test-skill'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile( + path.join(skillSubDir, 'SKILL.md'), + '---\nname: " ../../exploit "\ndescription: exploit\n---\nbody', + ); + + const installed = await installSkill( + mockSkillSourceDir, + 'workspace', + undefined, + () => {}, + ); + expect(installed.length).toBe(1); + expect(installed[0].name).toBe(' ..-..-exploit '); + + const destPath = installed[0].location; + const resolvedTarget = path.resolve(tempDir, '.gemini/skills'); + expect(destPath.startsWith(resolvedTarget + path.sep)).toBe(true); + }); + + it('should allow installation if skill name starts with double dots but is safe (e.g. ..-foo or ...)', async () => { + const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source'); + const skillSubDir = path.join(mockSkillSourceDir, 'test-skill'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile( + path.join(skillSubDir, 'SKILL.md'), + '---\nname: ..-foo\ndescription: safe skill name starting with double dots\n---\nbody', + ); + + const installed = await installSkill( + mockSkillSourceDir, + 'workspace', + undefined, + () => {}, + ); + expect(installed.length).toBe(1); + expect(installed[0].name).toBe('..-foo'); + + const destPath = installed[0].location; + const resolvedTarget = path.resolve(tempDir, '.gemini/skills'); + expect(destPath.startsWith(resolvedTarget + path.sep)).toBe(true); + }); }); }); diff --git a/packages/cli/src/utils/skillUtils.ts b/packages/cli/src/utils/skillUtils.ts index ca4ed08c82..1f0d783a50 100644 --- a/packages/cli/src/utils/skillUtils.ts +++ b/packages/cli/src/utils/skillUtils.ts @@ -75,6 +75,18 @@ export function renderSkillActionFeedback( return `Skill "${skillName}" ${actionVerb} ${preposition} ${s} settings.`; } +function isPathTraversal(relative: string): boolean { + return ( + relative === '..' || + relative.startsWith('..' + path.sep) || + path.isAbsolute(relative) + ); +} + +function isInvalidSubpath(relative: string): boolean { + return relative === '' || isPathTraversal(relative); +} + /** * Central logic for installing a skill from a remote URL or local path. */ @@ -132,11 +144,12 @@ export async function installSkill( sourcePath = path.resolve(sourcePath); // Quick security check to prevent directory traversal out of temp dir when cloning - if ( - tempDirToClean && - !sourcePath.startsWith(path.resolve(tempDirToClean)) - ) { - throw new Error('Invalid path: Directory traversal not allowed.'); + if (tempDirToClean) { + const resolvedTemp = path.resolve(tempDirToClean); + const relative = path.relative(resolvedTemp, sourcePath); + if (isPathTraversal(relative)) { + throw new Error('Invalid path: Directory traversal not allowed.'); + } } onLog(`Searching for skills in ${sourcePath}...`); @@ -159,16 +172,22 @@ export async function installSkill( throw new Error('Skill installation cancelled by user.'); } - await fs.mkdir(targetDir, { recursive: true }); + const resolvedTarget = path.resolve(targetDir); + await fs.mkdir(resolvedTarget, { recursive: true }); const installedSkills: Array<{ name: string; location: string }> = []; for (const skill of skills) { const skillName = skill.name; const skillDir = path.dirname(skill.location); - const destPath = path.join(targetDir, skillName); + const destPath = path.resolve(resolvedTarget, skillName); - const exists = await fs.stat(destPath).catch(() => null); + const relative = path.relative(resolvedTarget, destPath); + if (isInvalidSubpath(relative)) { + throw new Error('Invalid skill name: Path traversal detected.'); + } + + const exists = await fs.lstat(destPath).catch(() => null); if (exists) { onLog(`Skill "${skillName}" already exists. Overwriting...`); await fs.rm(destPath, { recursive: true, force: true }); @@ -231,14 +250,20 @@ export async function linkSkill( throw new Error('Skill linking cancelled by user.'); } - await fs.mkdir(targetDir, { recursive: true }); + const resolvedTarget = path.resolve(targetDir); + await fs.mkdir(resolvedTarget, { recursive: true }); const linkedSkills: Array<{ name: string; location: string }> = []; for (const skill of skills) { const skillName = skill.name; const skillSourceDir = path.dirname(skill.location); - const destPath = path.join(targetDir, skillName); + const destPath = path.resolve(resolvedTarget, skillName); + + const relative = path.relative(resolvedTarget, destPath); + if (isInvalidSubpath(relative)) { + throw new Error('Invalid skill name: Path traversal detected.'); + } const exists = await fs.lstat(destPath).catch(() => null); if (exists) { @@ -275,18 +300,21 @@ export async function uninstallSkill( ? storage.getProjectSkillsDir() : Storage.getUserSkillsDir(); + const resolvedTarget = path.resolve(targetDir); + // Load all skills in the target directory to find the one with the matching name - const discoveredSkills = await loadSkillsFromDir(targetDir); + const discoveredSkills = await loadSkillsFromDir(resolvedTarget); const skillToUninstall = discoveredSkills.find((s) => s.name === name); if (!skillToUninstall) { // Fallback: Check if a directory with the given name exists. // This maintains backward compatibility for cases where the metadata might be missing or corrupted // but the directory name matches the user's request. - const skillPath = path.resolve(targetDir, name); + const skillPath = path.resolve(resolvedTarget, name); // Security check: ensure the resolved path is within the target directory to prevent path traversal - if (!skillPath.startsWith(path.resolve(targetDir))) { + const relative = path.relative(resolvedTarget, skillPath); + if (isInvalidSubpath(relative)) { return null; } @@ -300,7 +328,12 @@ export async function uninstallSkill( return { location: skillPath }; } - const skillDir = path.dirname(skillToUninstall.location); + const skillDir = path.resolve(path.dirname(skillToUninstall.location)); + const relative = path.relative(resolvedTarget, skillDir); + if (isInvalidSubpath(relative)) { + return null; + } + await fs.rm(skillDir, { recursive: true, force: true }); return { location: skillDir }; }