T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/backup-restore.js:260
- Finding
- Shell Command Injection Through User-Controlled Archive and Backup Paths<![CDATA[ ## Vulnerability Details **File Location**: `scripts/backup-restore.js:260-307` **Related Locations**: `scripts/server.js:198-200`, `scripts/server.js:255-268` **Vulnerability Type**: OS command injection **Risk Level**: Critical ### Vulnerable Code ```js function createArchive(sourceDir, archivePath) { const dirName = path.basename(sourceDir); const parentDir = path.dirname(sourceDir); if (hasTar()) { execSync(`tar -czf "${archivePath}" -C "${parentDir}" "${dirName}"`, { stdio: 'ignore' }); } else if (IS_WIN) { // Fallback: PowerShell Compress-Archive (creates zip) const zipPath = archivePath.replace(/\.tar\.gz$/, '.zip'); execSync( `powershell -NoProfile -Command "Compress-Archive -Path '${sourceDir}' -DestinationPath '${zipPath}' -CompressionLevel Optimal -Force"`, { stdio: 'ignore' } ); fs.renameSync(zipPath, archivePath); warn('Used ZIP format (tar not available). Archive is still cross-platform compatible.'); } else { error('tar is required but not found. Install it with your package manager.'); } } function extractArchive(archivePath, destDir) { fs.mkdirSync(destDir, { recursive: true }); if (hasTar()) { try { execSync(`tar -xzf "${archivePath}" -C "${destDir}"`, { stdio: 'ignore' }); return; } catch {} } if (IS_WIN) { try { execSync( `powershell -NoProfile -Command "Expand-Archive -Path '${archivePath}' -DestinationPath '${destDir}' -Force"`, { stdio: 'ignore' } ); return; } catch {} } else { try { execSync(`unzip -o "${archivePath}" -d "${destDir}"`, { stdio: 'ignore' }); return; } catch {} } error('Could not extract archive. Ensure tar or zip tools are available.'); } ``` The HTTP server also constructs shell commands from configured paths and uploaded filenames: ```js const out = execSync( `node "${BACKUP_JS}" backup --output-dir "${BACKUP_DIR}"`, { encoding: 'utf8', timeout: ...[truncated 2316 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace every shell-string invocation with an executable and an argument array: ```js const { execFileSync } = require('child_process'); execFileSync('tar', [ '-czf', archivePath, '-C', parentDir, dirName, ], { stdio: 'ignore' }); ``` 2. Invoke Node directly without a shell: ```js execFileSync(process.execPath, [ BACKUP_JS, 'restore', filePath, '--dry-run', ], { encoding: 'utf8', timeout: 180000, }); ``` 3. Do not use `echo yes | ...`. Add a non-interactive restore flag intended for the server, and require the server to perform its own authorization and confirmation checks before passing that flag. 4. Apply a conservative filename policy, for example: ```js const SAFE_ARCHIVE = /^[A-Za-z0-9][A-Za-z0-9._-]{0,199}\.(?:tar\.gz|zip)$/; ``` 5. Resolve all configured and requested paths with `path.resolve()` and verify that they remain under the intended backup directory using `path.relative()`. 6. On Windows, invoke PowerShell through `execFileSync()` with separately supplied arguments, or use a Node archive library that does not require command construction. 7. Add regression tests using filenames containing quotes, semicolons, command substitutions, spaces, Unicode characters, and path separators. ]]>
