T09 · Insecure Skill Coding Practices
Warning
- Location
- personal-finance.sh:250
- Finding
- Output Path Validation Can Be Bypassed to Overwrite Arbitrary Writable Files<![CDATA[ ## Vulnerability Details **File Location**: `personal-finance.sh`, lines 250-260 **Vulnerability Type**: Improper path validation and unsafe file overwrite **Risk Level**: Medium ### Vulnerable Code ```python out_path = Path(OUTPUT_PATH) if out_path.exists(): print(f"Warning: {out_path} already exists and will be overwritten.", file=sys.stderr) if out_path.is_absolute() and not str(out_path).startswith(str(Path.home())): raise FinanceError(f"Output path must be in user home directory for safety") out_path.parent.mkdir(parents=True, exist_ok=True) with out_path.open("w", newline="", encoding="utf-8") as ostream: writer = csv.DictWriter(ostream, fieldnames=headers) ``` ### Technical Analysis The output-path security check is applied only when the supplied path is absolute. A relative path containing parent-directory components, such as `../../target`, is accepted even when its resolved destination is outside the user's home directory. The string-prefix comparison also does not provide a reliable directory-containment check. For example, a path whose textual prefix resembles the home path is not necessarily inside that directory. Furthermore, the code does not resolve symlinks before validation. A path inside the allowed directory can therefore be a symbolic link to a file elsewhere. The file is opened in `"w"` mode, which truncates an existing file. The warning shown for an existing path does not require confirmation and does not prevent the overwrite. ### Attack Path 1. An attacker influences the `--output` argument supplied to the skill. 2. The attacker supplies a relative traversal path, for example: ```sh ./personal-finance.sh categorize \ --csv transactions.csv \ --output ../../target-file ``` 3. Because the path is relative, the absolute-path restriction is skipped. 4. `mkdir()` creates missing parent directories where permitted. 5. `open("w")` creates or truncates the resolved target file. Alternatively, ...[truncated 672 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Resolve both the home directory and requested output path before performing validation: ```python home = Path.home().resolve() out_path = Path(OUTPUT_PATH).expanduser().resolve(strict=False) try: out_path.relative_to(home) except ValueError as exc: raise FinanceError("Output path must be inside the user home directory") from exc ``` 2. Reject symbolic-link output files and validate existing parent directories for symlinks before writing. 3. Use exclusive creation mode (`"x"`) by default so existing files cannot be silently truncated. 4. If overwriting is required, introduce an explicit `--force` option and require it whenever the destination exists. 5. Create the file with restrictive permissions appropriate for financial data, such as mode `0600`, and ensure newly created directories are not broadly accessible. 6. Add tests covering relative traversal, similarly prefixed directories, symbolic links, nonexistent nested directories, and attempted overwrites. ]]>