Skip to content

Fix command injection via file path in convertToWavType - #259

Open
zachariah-mithani wants to merge 1 commit into
ChetanXpro:mainfrom
zachariah-mithani:fix/command-injection-in-convertToWavType
Open

Fix command injection via file path in convertToWavType#259
zachariah-mithani wants to merge 1 commit into
ChetanXpro:mainfrom
zachariah-mithani:fix/command-injection-in-convertToWavType

Conversation

@zachariah-mithani

Copy link
Copy Markdown

Summary

convertToWavType() (src/utils.ts) builds an ffmpeg command as a shell string and runs it through shelljs.exec(), interpolating the caller-supplied file path inside double quotes without escaping:

const command = `ffmpeg -nostats -loglevel error -y -i "${inputFilePath}" -ar 16000 -ac 1 -c:a pcm_s16le "${outputFilePath}"`
const result = shell.exec(command)

A path containing a " breaks out of the quoting. This is reached straight from the public nodewhisper(filePath, options) API — filePath is only existence-checked (checkIfFileExists), never sanitized — so an application that transcribes a user-influenced file name is exposed to arbitrary command execution (CWE-78).

Impact

A file named:

inp" ; touch PWNED ; ".mp3

turns the command into:

ffmpeg ... -i "inp" ; touch PWNED ; ".mp3" ...

and the shell runs touch PWNED. The injected command executes whether or not ffmpeg itself succeeds, during convertToWavType (before transcription). Apps that save user-uploaded audio under its original filename and pass that path to nodewhisper are the realistic exposure.

I confirmed this on the published nodejs-whisper@0.3.0: a file with that name caused the injected command to run.

Fix

Run ffmpeg with execFileSync and a discrete argv array (no shell), so the file path is always treated as data and can never be parsed as shell syntax:

execFileSync('ffmpeg', ['-nostats','-loglevel','error','-y','-i', inputFilePath, '-ar','16000','-ac','1','-c:a','pcm_s16le', outputFilePath], { stdio: 'pipe' })
  • Behaviour is unchanged for legitimate paths; the same error message is thrown on conversion failure.
  • Verified after the change: the PoC above no longer executes (ffmpeg receives the whole string as a single literal filename), and normal conversion still works. tsc build passes.

One file changed (src/utils.ts); the now-unused shelljs import in this file is removed.

convertToWavType() interpolated the caller-supplied file path into a shell
command string passed to shelljs.exec():

    const command = `ffmpeg ... -i "${inputFilePath}" ... "${outputFilePath}"`
    shell.exec(command)

The path is placed inside double quotes but never escaped, so a path that
contains a double quote breaks out of the quoting. Because this is reached
directly from the public nodewhisper(filePath, options) API (filePath is only
existence-checked, not sanitized), an application that transcribes a
user-influenced file name is exposed to arbitrary command execution. A file
named:  inp" ; touch PWNED ; ".mp3  runs:
    ffmpeg ... -i "inp" ; touch PWNED ; ".mp3" ...   ->  touch PWNED executes.

Fix: run ffmpeg with execFileSync and a discrete argv array (no shell), so the
file path is always treated as data and can never be parsed as shell syntax.
Behaviour is unchanged for legitimate paths; the same error message is thrown
on conversion failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant