T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/export-conversations.ts:106
- Finding
- Symlink Following Allows Sensitive Export Disclosure and Arbitrary File Overwrite<![CDATA[ ## Vulnerability Details **File Location**: `scripts/export-conversations.ts:106-109, 394-409, 428-431, 443-475, 509` **Vulnerability Type**: Symlink following / improper filesystem object validation **Risk Level**: High ### Vulnerable Code ```ts function writePrivate(path: string, contents: string): void { writeFileSync(path, contents, { mode: 0o600 }); chmodSync(path, 0o600); } ``` ```ts mkdirSync(outputDir, { recursive: true, mode: 0o700 }); chmodSync(outputDir, 0o700); if (lstatSync(outputDir).isSymbolicLink()) { throw new Error( `Refusing to export to ${outputDir} — the destination itself is a symbolic link.`, ); } const settledDir = realpathSync(outputDir); const settledObjection = destinationObjection(settledDir); if (settledObjection) { throw new Error( `Refusing to export to ${outputDir} — after creation it resolves to ${settledDir}, ` + `which is unacceptable: ${settledObjection}.`, ); } mkdirSync(join(outputDir, "conversations"), { recursive: true, mode: 0o700 }); chmodSync(join(outputDir, "conversations"), 0o700); ``` The following calls consequently write through any existing symbolic links: ```ts writePrivate( join(outputDir, MANIFEST_NAME), JSON.stringify(/* manifest data */), ); writePrivate( join(outputDir, "index.json"), redact(JSON.stringify(items, null, 2), redactSecrets), ); writePrivate( join(outputDir, "conversations", `${item.id}.json`), redact(JSON.stringify(conversation, null, 2), redactSecrets), ); writePrivate(join(outputDir, "conversations", `${item.id}_${slug}.md`), md); writePrivate(join(outputDir, "summary.md"), summary); ``` ### Technical Analysis The exporter validates only the top-level `outputDir` against symbolic links. It does not validate the fixed output files or the `conversations` child directory before opening them. Node.js `writeFileSync()` follows a symbolic link at the destination. Its default behavior also truncates the linked target. Likewise, `chmodSync ...[truncated 3602 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Reject every pre-existing symbolic-link component and output object.** - Validate the `conversations` child with `lstatSync()` and reject it if it is a symbolic link or is not a real directory. - Validate every existing fixed-name output file with `lstatSync()` and reject symbolic links and non-regular files. - Revalidate the canonical path of every child against the canonical export directory. 2. **Use no-follow, exclusive filesystem operations.** - Open output files with `fs.openSync()` using `O_NOFOLLOW` where supported. - Prefer `O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW` for newly generated files. - For replacement of existing files, write to a securely created temporary file in the same validated directory, apply mode `0600`, and atomically rename it after validating the destination. - Do not rely on `writeFileSync(path, ...)` followed by `chmodSync(path, ...)`, because both path operations can follow links and are subject to races. 3. **Use directory-descriptor-relative operations where the platform permits.** - Open the validated export directory once. - Create and open children relative to that trusted directory descriptor. - Apply no-follow semantics so an attacker cannot replace ancestors between validation and use. 4. **Secure reused directories.** - Reject reused output directories unless ownership, type, and permissions are verified. - Ensure the output directory and all ancestors relevant to the export are owned by the current user and are not writable by group or others. - If a `conversations` directory already exists, verify it is a real directory, has the expected owner, and resolves strictly below the validated export root. 5. **Treat canonical containment checks as defense in depth.** - After opening or creating an object, verify its resolved location remains inside the canonical export root. - Do not use path-string canonicalization alone as the primary pr ...[truncated 468 chars]