T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/rebuild_verify.py:354
- Finding
- Predictable Output Files Follow Symbolic Links and Permit Unintended File Overwrites<![CDATA[ ## Vulnerability Details **File Location**: `scripts/rebuild_verify.py:354-370` and `scripts/rebuild_verify.py:521-536` **Vulnerability Type**: Symbolic-link file overwrite through unsafe output-file creation **Risk Level**: Medium ### Vulnerable Code The `extract-steps --write-steps` command creates predictable filenames inside a potentially pre-existing directory: ```python if args.write_steps: os.makedirs(args.write_steps, exist_ok=True) manifest = [] for s in steps: b = next(b for b in blocks if b["start"] == s["start_line"] and b["end"] == s["end_line"]) name = "step_%02d.%s" % (s["index"], {"bash": "sh", "sh": "sh", "shell": "sh", "python": "py"}.get(s["lang"], "txt")) p = os.path.join(args.write_steps, name) payload = "\n".join(b["content"]) + ("\n" if b["content"] else "") with open(p, "w", encoding="utf-8", newline="") as f: f.write(payload) manifest.append({"index": s["index"], "file": name, "bytes": s["bytes"], "sha256": s["sha256"], "status": s["status"]}) with open(os.path.join(args.write_steps, "steps.json"), "w", encoding="utf-8") as f: json.dump(manifest, f, ensure_ascii=False, indent=1) out["written"] = args.write_steps ``` The fixture generator has the same issue with predictable output names: ```python def cmd_genfixtures(args): root = os.path.abspath(args.dir) os.makedirs(root, exist_ok=True) canon = make_canonical(1116) assert len(canon) == 1116 and canon.endswith(b"\n") and not canon.endswith(b"\n\n") fixtures = { "canonical.txt": canon, "drift_no_nl.txt": canon[:-1], "drift_3nl.txt": canon + b"\n\n", "truncated.txt": canon[:500], "html404.txt": make_html404(), "same_size_diff.txt": canon[:100] + bytes([canon[100] + 1]) + canon[101:], "crlf.txt": canon.replace(b"\n", b"\r\n"), } asser ...[truncated 2560 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Require output directories to be newly created by the process rather than silently reusing existing directories. 2. If existing directories must be supported, inspect every destination with `os.lstat()` and reject symbolic links and non-regular files. 3. Open output files through `os.open()` with restrictive flags: - `O_WRONLY` - `O_CREAT` - `O_EXCL` - `O_NOFOLLOW`, where available 4. Apply a restrictive file mode such as `0o600` unless broader access is explicitly required. 5. Create output in securely generated temporary files and use an atomic rename only after validation. 6. Resolve and validate the parent directory, while recognizing that `realpath()` checks alone do not eliminate time-of-check/time-of-use races. 7. Avoid fixed shared temporary paths in documentation. Recommend `tempfile.mkdtemp()` or a private directory with mode `0o700`. 8. Add regression tests that pre-create each output filename as a symbolic link and verify that the command fails without modifying the linked target. ]]>