T09 · Insecure Skill Coding Practices
Warning
- Location
- src/video_merger.py:97
- Finding
- FFmpeg Concat Manifest Injection Through Unescaped Filenames<![CDATA[ ## Vulnerability Details **File Location**: `src/video_merger.py:97-115` and `src/video_merger.py:239-257` **Vulnerability Type**: Injection into an FFmpeg concat-demuxer manifest **Risk Level**: Medium ### Vulnerable Code The full-video merge path constructs a concat manifest directly from filenames and then processes it with unsafe path checks disabled: ```python # 生成concat列表 with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f: for v in video_list: f.write(f"file '{os.path.abspath(v)}'\n") concat_file = f.name try: # 先无损拼接所有片段 with tempfile.NamedTemporaryFile(suffix='.mp4', delete=False) as f: temp_raw = f.name cmd_concat = [ self.ffmpeg_path, "-y", "-f", "concat", "-safe", "0", "-i", concat_file, "-c", "copy", temp_raw ] print("正在拼接视频片段...") subprocess.run(cmd_concat, capture_output=True, check=True) ``` The chunked merge path repeats the same vulnerable pattern: ```python # 生成concat列表 with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f: for v in video_list: f.write(f"file '{os.path.abspath(v)}'\n") concat_file = f.name try: # 先无损拼接所有片段 with tempfile.NamedTemporaryFile(suffix='.mp4', delete=False) as f: temp_raw = f.name cmd_concat = [ self.ffmpeg_path, "-y", "-f", "concat", "-safe", "0", "-i", concat_file, "-c", "copy", temp_raw ] print(f"正在拼接分块 {os.path.basename(output_path)}，包含 {len(video_list)} 个片段...") subprocess.run(cmd_concat, capture_output=True, check=True) ``` ### Technical Analysis `get_sorted_videos()` only requires a filename to begin with a numeric prefix followed by an underscore and end in `.mp4`. It does not reject apostrophes, carriage returns, newline characters, or other concat-manifest syntax. The selected filename is embedded between single quotes without FFmpeg concat-demuxer escaping: ```python f.write(f" ...[truncated 3044 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Reject manifest control characters** - Reject filenames containing `\r`, `\n`, NUL, or other control characters. - Reject or correctly encode apostrophes according to FFmpeg concat-demuxer quoting rules. - Perform validation before creating the manifest. 2. **Use a dedicated FFmpeg concat-path encoder** - Do not rely on generic shell escaping because concat-manifest syntax is not shell syntax. - Generate each `file` directive using escaping explicitly documented for the FFmpeg concat demuxer. - Prefer a tested library routine rather than manually combining quotes and paths. 3. **Restore safe-path enforcement** - Remove `-safe 0` unless absolute paths are strictly necessary. - If absolute paths are required, independently enforce path containment before disabling FFmpeg's safety check. 4. **Enforce input-directory containment** - Resolve the input directory with `Path.resolve()`. - Resolve every candidate path and verify it remains beneath the resolved input directory. - Define an explicit policy for symbolic links; reject them if following links outside the input directory is not required. 5. **Restrict FFmpeg protocols** - Supply a minimal `-protocol_whitelist` appropriate for local file processing. - Do not enable HTTP, pipe, concat, or other unnecessary protocols for manifest inputs. 6. **Add security regression tests** Test filenames containing: ```text ' \r \n file ' http: pipe: # ``` Tests should confirm that malicious names are rejected before FFmpeg is invoked and that generated manifests cannot contain attacker-created directives. 7. **Apply the fix to both code paths** - Update the manifest generation in `merge()`. - Update the duplicate manifest generation in `_merge_single_chunk()`. - Prefer a single hardened helper function so the two implementations cannot diverge. ]]>