fix(subtitle): prevent ffmpeg protocol/argument injection in /api/subtitle - #144
Open
andesyteoss wants to merge 1 commit into
Open
andesyteoss wants to merge 1 commit into
andesyteoss wants to merge 1 commit into
Conversation
m-kudahl
added a commit
to m-kudahl/nodecast-tv
that referenced
this pull request
Sep 16, 2026
…T login Upstream issues technomancer702#151, technomancer702#133, and the half of technomancer702#144 that was never finished. technomancer702#151 - JWT and session cookies were signed with constants committed to the repository ('nodecast-tv-secret-key-change-in-production' and 'keyboard cat') whenever JWT_SECRET was unset, which is the default and was the case here. A token signed with the published value authenticated as admin; verified before and after, it now gets a 401. JWT_SECRET still wins if set, otherwise a key is generated on first run into data/secret.key (0600, already gitignored). Existing logins are invalidated once - passwords are unaffected. PR technomancer702#144 gave /api/subtitle a scheme check and a -protocol_whitelist, but /api/probe, /api/remux and /api/transcode take the same url parameter, hand it to the same binaries, and had the same hole - ffmpeg speaks file:, concat: and subfile:, so an unauthenticated request could read files off the server. One validator in server/safeUrl.js now covers all four, and the whitelist is on every ffmpeg invocation including the session builder. Confirmed: file:// reached ffprobe before, returns 400 on all four now, and ordinary http URLs are unaffected. technomancer702#133 - the login form had no method, so any native submit (JS not attached yet, or an error before preventDefault) sent the password as a GET query string into history and logs. It is method="post" now. Also deletes public/js/login.js, which no page loaded and which referenced element ids and a storage key that do not exist.
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
GET /api/subtitleendpoint (server/routes/subtitle.js) passes the user-suppliedurlquery parameter directly toffmpegas-i <url>, and interpolatesindexinto-map 0:${index}without validation. Because ffmpeg supports a wide range of input protocols (file:,concat:,subfile:,data:,gopher:, etc.), an attacker who can reach this endpoint can coerce the server-side ffmpeg into reading arbitrary local files and streaming their contents back as WebVTT. The route is mounted inserver/index.js(app.use('/api/subtitle', require('./routes/subtitle'))) with no authentication middleware in front of it, so exploitation is unauthenticated.server/routes/subtitle.js(router.get('/')).Proof of concept
With a default local run of nodecast-tv (e.g.
npm startor the Docker image) listening on port 3000, no login required:Before the fix, the first two return the file contents wrapped in a WebVTT envelope (or trigger ffmpeg errors that leak the content in stderr-derived responses). After the fix, all three return HTTP 400 with a clear validation error and ffmpeg is never invoked.
Fix
server/routes/subtitle.js:urlwith the WHATWGURLconstructor and reject anything whose protocol is nothttp:orhttps:.indexto a number and requireNumber.isInteger(indexNum) && indexNum >= 0with an exact string round-trip so values like0 -map 0:v,1e2,0x1, or1.5are rejected.-protocol_whitelist http,https,tcp,tls,cryptoto the ffmpeg argv as defense-in-depth, so even if a future code path forwards an attacker-influenced URL, ffmpeg itself refuses non-network protocols (and refuses to follow a redirect into e.g.file:).Total diff: +26 / -3 in a single file, no behavior change for legitimate
http(s)subtitle sources.Testing
HEAD~1(got/etc/passwdback as WebVTT cues), applied the patch, re-ran — all three PoC requests now return400with{"error":"Only http(s) URLs are allowed"}or{"error":"Index must be a non-negative integer"}.curl 'http://127.0.0.1:3000/api/subtitle?url=https://example.com/sample.mkv&index=2'still reaches ffmpeg with the expected-i https://example.com/sample.mkv -map 0:2argv.-protocol_whitelist(standard since ffmpeg 3.0).Security analysis / adversarial review
Before submitting we tried to disprove this. Specifically:
server/index.jsmounts the router withapp.use('/api/subtitle', require('./routes/subtitle')). There is noapp.usemiddleware that enforces authentication before/api/subtitle— the auth-protected routes rely on per-routeensureAuthenticatedchecks, andsubtitle.jshas none. So the unauthenticated-remote threat model holds.file:by default? No. Upstream ffmpeg builds enable thefileprotocol by default; it must be explicitly excluded via-protocol_whitelist, which is exactly what part (3) of the fix does.indexreally injectable? Because Node'schild_process.spawnis used (argv array, not a shell), classic shell metacharacters don't apply — but-map 0:${index}string-concatenates into a single argv token, and ffmpeg parses-mapvalues loosely; combined with the surrounding argv it's enough to smuggle option-like tokens into subsequent parsing. Even setting that aside, the URL-side file read alone is high severity and independently sufficient to justify the fix.http(s)-only matches the documented usage.