T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/topology.js:20
- Finding
- Symlink-Based Filesystem Containment Bypass<![CDATA[ ## Vulnerability Details **File Location**: `scripts/topology.js:20-31`, with unsafe filesystem operations at `scripts/topology.js:180-199` **Vulnerability Type**: Improper link resolution before file access **Risk Level**: Medium ### Vulnerable Code ```js // Canonicalize TREES_DIR once at startup for path containment checks. const TREES_DIR_REAL = fs.realpathSync(TREES_DIR); const CONCEPT_INDEX_PATH = path.join(TREES_DIR_REAL, 'concepts.json'); // Resolve a user-supplied file path and enforce that it stays inside TREES_DIR. // Rejects absolute paths, ".." traversal, and symlinks that escape the directory. function resolveSafePath(file) { const resolved = path.resolve(TREES_DIR_REAL, path.basename(file)); if (!resolved.startsWith(TREES_DIR_REAL + path.sep) && resolved !== TREES_DIR_REAL) { console.error(`Error: Path escapes trees directory: ${file}`); process.exit(1); } return resolved; } ``` The resulting path is subsequently used for reads and writes without validating the final target: ```js function loadTree(file) { // Strip to basename and resolve inside TREES_DIR to prevent path traversal. const filePath = resolveSafePath(file); if (!fs.existsSync(filePath)) { console.error(`Error: Tree file not found: ${filePath}`); process.exit(1); } return { tree: JSON.parse(fs.readFileSync(filePath, 'utf8')), filePath }; } function saveTree(filePath, tree) { tree.updated = new Date().toISOString(); // Update concept index, then use it for weights and companion links const index = updateConceptIndexForTree(filePath, tree); // Update node weights from cross-tree spread updateWeights(tree, index); fs.writeFileSync(filePath, JSON.stringify(tree, null, 2)); writeMdCompanion(filePath, tree, index); } ``` ### Technical Analysis The containment check validates only the lexical path produced by joining the canonical trees directory with `path.basename(file)`. It does not canonicalize the resulting existing fil ...[truncated 2686 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Reject symbolic-link inputs before reading: ```js const stat = fs.lstatSync(candidate); if (stat.isSymbolicLink()) { throw new Error('Symbolic-link tree files are not allowed'); } ``` 2. For an existing input file, canonicalize the final target and verify containment: ```js const candidate = path.resolve(TREES_DIR_REAL, path.basename(file)); const realTarget = fs.realpathSync(candidate); if (!realTarget.startsWith(TREES_DIR_REAL + path.sep)) { throw new Error('Tree target escapes the configured directory'); } ``` 3. Require tree files to be regular files rather than directories, devices, FIFOs, or sockets: ```js if (!fs.statSync(realTarget).isFile()) { throw new Error('Tree target is not a regular file'); } ``` 4. Reduce check-to-use race exposure by opening verified files through file descriptors and using no-follow semantics where supported. On platforms exposing the relevant constants, use `O_NOFOLLOW` when opening files. 5. Avoid writing directly through a user-selectable existing pathname. Serialize to a newly created temporary regular file inside `TREES_DIR_REAL`, flush and close it, verify the destination policy, and atomically rename it into place. Ensure temporary files are created with exclusive creation and restrictive permissions. 6. Apply equivalent link and file-type validation to every file enumerated by `getAllTrees()`, because directory scans can also encounter attacker-created symbolic links. 7. Add regression tests covering: - A symlink inside the trees directory targeting a file outside it. - A symlink targeting another file inside the directory. - Replacement of a validated file with a symlink between validation and use. - Non-regular filesystem objects with a `.json` suffix. - Read and mutating commands against each prohibited target type. ]]>