T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/lib/local-run.mjs:22
- Finding
- Shell Command Injection in the Exported Local Model Runner<![CDATA[ ## Vulnerability Details **File Location**: `scripts/lib/local-run.mjs:22-32, 65` **Vulnerability Type**: Shell command injection **Risk Level**: High ### Vulnerable Code ```js function defaultWhich(bin) { execFileSync("command", ["-v", bin], { stdio: "ignore", shell: true }); } function defaultExec(cmd) { execFileSync(cmd, { stdio: ["ignore", "pipe", "pipe"], shell: true, timeout: 600000 }); } const fill = (tpl, vars) => tpl.replace(/\{(\w+)\}/g, (_, k) => (vars[k] != null ? String(vars[k]) : "")); ``` The generated command is subsequently executed as follows: ```js try { exec(fill(model.invoke, vars)); } catch (e) { lastFailure = { recommend: "install", model: model.id, sizeMB: model.sizeMB, command: model.install, reason: e.message || String(e), }; continue; } ``` Relevant model templates in `scripts/lib/local-models.mjs` contain directly substituted values: ```js invoke: "python -m kokoro --text {text} --voice {voice} --out {out}", ``` ```js invoke: "whisperx {audio} --output_format json --out {out}", ``` ```js invoke: "realesrgan-ncnn-vulkan -i {in} -o {out} -s 4", ``` ### Technical Analysis `fill()` directly inserts values such as speech text, input paths, output paths, and voice names into a command string. `defaultExec()` then executes that string through a system shell by setting `shell: true`. No shell escaping or quoting is applied. Consequently, values containing shell metacharacters such as `;`, `&&`, `|`, backticks, `$()`, redirection operators, or embedded quotes can alter the command structure instead of remaining ordinary arguments. The helper is exported and designed to receive caller-provided `vars`. No active production caller of `runLocalModel()` was identified in the audited repository; the currently active image and video providers use safer argument-array construction. The vulnerability is therefore dormant in the current observed call graph, but it becomes directly exploitable i ...[truncated 1336 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Remove `shell: true` from all model execution. 2. Represent every invocation as a binary and argument array: ```js const [binary, ...argv] = buildArgv(model.invoke, vars); execFileSync(binary, argv, { stdio: ["ignore", "pipe", "pipe"], timeout: 600000, }); ``` 3. Use the existing `buildArgv()` approach from `scripts/lib/local-models.mjs`, which preserves substituted values as individual arguments. 4. Do not attempt to repair this solely through shell escaping; avoiding a shell is safer and less platform-dependent. 5. Validate path variables and constrain enumerated fields such as voice IDs. 6. Add regression tests using values containing: - `; touch /tmp/test` - `$(touch /tmp/test)` - backticks - `&&` and `|` - spaces and quotes 7. Assert that injected metacharacters are passed literally to the model process and never interpreted by a shell. ]]>