T09 · Insecure Skill Coding Practices
Warning
- Location
- clips_machine.py:396
- Finding
- User-Controlled Output Path Can Escape the Intended Output Directory<![CDATA[ ## Vulnerability Details **File Location**: `clips_machine.py:396-398` **Vulnerability Type**: Arbitrary directory creation and file overwrite through path traversal **Risk Level**: Medium ### Vulnerable Code ```python output_dir = OUTPUT_DIR / output_name output_dir.mkdir(parents=True, exist_ok=True) results = {"directory": str(output_dir), "clips": []} ``` The value reaches this operation directly from the command-line argument: ```python parser.add_argument("--output", help="Custom output folder name") ``` ```python process_video( source=args.source, num_clips=args.clips, style=args.style, no_captions=args.no_captions, start_time=parse_timestamp(args.start) if args.start else None, end_time=parse_timestamp(args.end) if args.end else None, min_score=args.min_score, output_name=args.output, ) ``` The resulting directory is subsequently used for fixed-name output files, including: ```python transcript_path = output_dir / "transcript.json" with open(transcript_path, "w") as f: json.dump(transcript, f, indent=2) ``` ```python moments_path = output_dir / "viral_moments.json" with open(moments_path, "w") as f: json.dump(moments, f, indent=2) ``` ```python with open(output_dir / "summary.md", "w") as f: f.write(summary) ``` ### Technical Analysis The `--output` value is described as a folder name, but it is not restricted to a single path component. Python's `pathlib` permits both traversal components and absolute paths: - `OUTPUT_DIR / "../../target"` resolves outside `OUTPUT_DIR`. - If `output_name` is absolute, it replaces the preceding `OUTPUT_DIR` component entirely. The program creates the selected directory recursively and writes multiple predictable filenames into it. Video-processing commands also use FFmpeg's `-y` option, which authorizes replacement of existing output files. No canonicalization, containment check, or refusal to overwrite existing destinations is performed. This is not co ...[truncated 1435 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Treat `--output` strictly as a directory name rather than an arbitrary path: 1. Reject absolute paths. 2. Reject `.` and `..` components and all path separators. 3. Resolve the candidate destination and verify that it remains beneath the resolved `OUTPUT_DIR`. 4. Consider refusing existing output directories to prevent accidental replacement of prior artifacts. 5. Avoid unconditional overwrite behavior where it is unnecessary. Example hardening: ```python def safe_output_directory(output_name: str) -> Path: base = OUTPUT_DIR.resolve() candidate_name = Path(output_name) if ( candidate_name.is_absolute() or len(candidate_name.parts) != 1 or output_name in ("", ".", "..") ): raise ValueError("Output must be a single directory name") candidate = (base / candidate_name).resolve() if not candidate.is_relative_to(base): raise ValueError("Output directory escapes the configured output root") candidate.mkdir(parents=False, exist_ok=False) return candidate ``` Replace the vulnerable construction with: ```python output_dir = safe_output_directory(output_name) ``` If compatibility with older Python versions is required, perform containment validation using `os.path.commonpath()` instead of `Path.is_relative_to()`. ]]>