T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/backup.mjs:45
- Finding
- Unvalidated backup names allow writes outside the backup directory<![CDATA[ ## Vulnerability Details **File Location**: `scripts/backup.mjs:45-47, 99-108, 133-139, 169-175, 197-199` **Vulnerability Type**: Path traversal and unintended sensitive-file placement **Risk Level**: High ### Vulnerable Code ```js if (args[i] === '--name' && args[i + 1]) { options.name = args[i + 1]; i++; } ``` ```js const backupPath = options.name ? path.join(BACKUP_DIR, 'named', options.name) : path.join(BACKUP_DIR, timestamp); if (fs.existsSync(backupPath)) { console.error(`❌ Backup already exists: ${backupPath}`); process.exit(1); } fs.mkdirSync(backupPath, { recursive: true }); ``` ```js const sourcePath = path.join(WORKSPACE_ROOT, filename); const destPath = path.join(backupPath, filename); if (fs.existsSync(sourcePath)) { // Copy file fs.copyFileSync(sourcePath, destPath); ``` ```js const destPath = path.join(backupPath, 'openclaw.sanitized.json'); fs.writeFileSync(destPath, sanitizedContent); ``` ```js const manifestPath = path.join(backupPath, 'manifest.json'); fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); ``` ### Technical Analysis The `--name` value is incorporated directly into `backupPath` without an allowlist, normalization check, or verification that the resolved destination remains below `backups/named`. A value containing traversal components such as `../` can escape the intended backup hierarchy. The script subsequently creates that directory and writes copies of the workspace files, the sanitized configuration, and the manifest to the resulting location. The operation remains limited to locations writable by the account running the Skill; it does not independently escalate operating-system privileges. However, it violates the minimum path privileges required by the declared backup functionality, which only needs to write beneath the Skill's backup directory. ### Attack Path 1. An attacker influences a command, automation configuration, wrapper, or user instruction that invokes `bac ...[truncated 1070 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Apply a strict allowlist to backup names, for example: ```js const SAFE_NAME = /^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/; if (!SAFE_NAME.test(options.name)) { throw new Error('Invalid backup name'); } ``` 2. Explicitly reject path separators, `.` and `..` components, null bytes, and absolute paths. 3. Resolve and verify the final path before performing filesystem operations: ```js const namedRoot = path.resolve(BACKUP_DIR, 'named'); const backupPath = path.resolve(namedRoot, options.name); const relative = path.relative(namedRoot, backupPath); if (relative.startsWith('..') || path.isAbsolute(relative)) { throw new Error('Backup path escapes the named backup directory'); } ``` 4. Create backup directories with mode `0700`. 5. Refuse to traverse symlinks in the destination hierarchy. 6. Add regression tests using names such as `../escape`, `../../tmp/output`, absolute paths, and platform-specific separators. ]]>