T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/theme-manager.js:143
- Finding
- Output Directory Containment Check Can Be Bypassed Using Sibling Paths<![CDATA[ ## Vulnerability Details **File Location**: `scripts/theme-manager.js`, lines 143–177 **Vulnerability Type**: Improper path containment validation leading to an arbitrary file write outside the intended directory **Risk Level**: Medium ### Vulnerable Code ```js // Validate output path to prevent arbitrary file write function validateOutputPath(outputPath) { if (!outputPath) { throw new Error('Output path is required'); } // Resolve to absolute path const absolutePath = resolve(outputPath); // Get the directory part const outputDir = dirname(absolutePath); // Ensure output directory is within current working directory // This prevents writing to system directories like /etc, ~/.ssh, etc. const cwd = process.cwd(); const resolvedOutputDir = resolve(outputDir); if (!resolvedOutputDir.startsWith(cwd)) { throw new Error(`Invalid output path: must be within current directory (${cwd})`); } // Prevent path traversal in filename const filename = basename(absolutePath); if (filename.includes('..') || filename.includes('/') || filename.includes('\\')) { throw new Error('Invalid filename: path traversal not allowed'); } // Require .zip extension const ext = extname(filename).toLowerCase(); if (ext !== '.zip') { throw new Error('Invalid filename: must have .zip extension'); } // Additional safety: filename must not be empty after removing extension const nameWithoutExt = basename(filename, ext); if (!nameWithoutExt || nameWithoutExt.length === 0) { throw new Error('Invalid filename: name cannot be empty'); } return absolutePath; } ``` The accepted path is subsequently used for writing: ```js const fileStream = createWriteStream(validatedPath); res.pipe(fileStream); ``` ### Technical Analysis The function attempts to restrict theme downloads to the current working directory. However, it checks containment using a raw string-prefix comparison: ```js resolvedOutputDir ...[truncated 1968 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Use path-aware containment rather than string-prefix matching: ```js import { basename, resolve, dirname, extname, relative, isAbsolute } from 'path'; function validateOutputPath(outputPath) { if (!outputPath || typeof outputPath !== 'string') { throw new Error('Output path is required'); } const cwd = resolve(process.cwd()); const absolutePath = resolve(outputPath); const outputDir = dirname(absolutePath); const relativeDir = relative(cwd, outputDir); if ( relativeDir === '..' || relativeDir.startsWith(`..${path.sep}`) || isAbsolute(relativeDir) ) { throw new Error(`Output path must be within the current directory (${cwd})`); } const filename = basename(absolutePath); if (extname(filename).toLowerCase() !== '.zip') { throw new Error('Output filename must have a .zip extension'); } return absolutePath; } ``` Additional hardening should include: 1. Open downloads with exclusive creation, such as `createWriteStream(path, { flags: 'wx' })`, to prevent silent overwrites. 2. Require explicit confirmation before replacing an existing file when overwrite behavior is necessary. 3. Resolve and validate the real path of the destination directory to reduce symbolic-link bypass risk. 4. Create downloads in a dedicated application-owned directory with restrictive permissions. 5. Add regression tests for sibling-prefix paths, traversal paths, absolute paths, symbolic links, and existing-file overwrites. ]]>