T09 · Insecure Skill Coding Practices
Warning
- Location
- skill.js:136
- Finding
- Unrestricted FFmpeg Paths and Protocols Permit Network Access and Arbitrary File Overwrite<![CDATA[ ## Vulnerability Details **File Location**: `skill.js:136-170` **Vulnerability Type**: Unvalidated FFmpeg input/output paths and protocols **Risk Level**: Medium ### Vulnerable Code ```js function crop({ inFile, out, top }) { if (!inFile) die('--in required'); if (!out) die('--out required'); let t; if (String(top || '') === 'auto') { const { w, h } = getVideoWH(inFile); t = recommendTop({ w, h }); console.error(`[aoi-clip] recommend cropTop=${t} (auto, from ${w}x${h})`); } else { t = Number(top || 150); } if (!Number.isFinite(t) || t < 0 || t > 600) die('--top invalid (0..600 or auto)'); runAllowed('ffmpeg', [ '-y', '-i', inFile, '-vf', `crop=in_w:in_h-${t}:0:${t}`, '-pix_fmt', 'yuv420p', out, ]); } function trim({ inFile, out, from, to }) { if (!inFile) die('--in required'); if (!out) die('--out required'); if (from == null || to == null) die('--from and --to required'); runAllowed('ffmpeg', [ '-y', '-ss', String(from), '-to', String(to), '-i', inFile, '-pix_fmt', 'yuv420p', out, ]); } ``` The related probing operation also passes the input directly to FFprobe: ```js function getVideoWH(inFile) { const res = spawnSync('ffprobe', [ '-v', 'error', '-select_streams', 'v:0', '-show_entries', 'stream=width,height', '-of', 'csv=p=0', inFile, ], { encoding: 'utf8' }); if (res.status !== 0) throw new Error('ffprobe failed'); const [w, h] = (res.stdout || '').trim().split(',').map(Number); if (!Number.isFinite(w) || !Number.isFinite(h)) throw new Error('invalid ffprobe output'); return { w, h }; } ``` ### Technical Analysis The `crop` and `trim` commands accept user-controlled `--in` and `--out` values and pass them directly to FFmpeg or FFprobe. The code does not restrict these values to local regular files, an approved working directory, or an allowed set of extensions and protocols. FFmpeg and FFprobe support multiple URL an ...[truncated 2851 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Resolve every input and output through `path.resolve` and require it to remain inside a dedicated, explicitly approved working directory. 2. Reject absolute paths, `..` traversal, URI schemes, protocol prefixes, null bytes, and option-like filenames beginning with `-`. 3. Use `fs.lstatSync` or equivalent checks to require inputs to be regular files and reject symbolic links unless they are explicitly supported and safely resolved. 4. Restrict output extensions to intended media formats and verify that the resolved parent directory is approved. 5. Disable FFmpeg network protocols where practical. Use protocol allowlisting options supported by FFmpeg, such as restricting protocols to local file access. 6. Remove unconditional `-y` behavior. Prefer `-n`, fail when the destination exists, or require an explicit overwrite flag and user confirmation. 7. Apply the same validation to `getVideoWH`, `crop`, `trim`, and every preset-generated path. 8. Implement the strict argument allowlist promised by the documentation and add tests covering remote URLs, absolute paths, traversal, symbolic links, option-like names, and existing destinations. ]]>