fix(server): prevent path traversal in voice_prompt query parameter - #101
Open
sebastionoss wants to merge 1 commit into
Open
fix(server): prevent path traversal in voice_prompt query parameter#101sebastionoss 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
/api/chatWebSocket handler inmoshi/moshi/server.pyaccepts avoice_promptquery parameter and joins it directly ontoself.voice_prompt_dirbefore passing it toload_voice_prompt/load_voice_prompt_embeddings. Both loaders calltorch.load()on the resulting path. Because the value is unvalidated, an attacker who can reach the endpoint can:..-sequences or absolute paths (voice_prompt=../../etc/passwd), and.ptfile exists anywhere on the filesystem —torch.load()unpickles by default, and pickle deserialization is a well-known RCE sink.torch.load.moshi/moshi/server.py,ServerState.handle_chat(around line 152).GET /api/chat(registered atapp.router.add_get("/api/chat", state.handle_chat)).localhostby default but supports--host 0.0.0.0and ships a first-class--gradio-tunnelflag that publishes it to the public internet, so remote reachability is a documented deployment mode, not a hypothetical.Data flow
Fix
Restrict
voice_promptto a bare filename insidevoice_prompt_dir:os.path.basename(name) != name, containsos.sep/os.altsep, is absolute, or is./...os.path.realpathand verify containment withos.path.commonpath, so symlinks pointing outside the directory are also rejected.HTTPBadRequestbefore touching the filesystem.The check is defence-in-depth: the syntactic rejection catches the common attack strings early with a clear error, and the
realpath+commonpathcheck is the authoritative containment guarantee.Diff is 19 additions / 1 deletion in a single file. No behavioural change for legitimate callers that pass a plain filename such as
default.pt.Proof of concept
Start the server against a directory containing at least one
.ptvoice prompt (the normal deployment):cd moshi python -m moshi.server --host 0.0.0.0 --port 8998Before the fix — the server joins the path unchecked and calls
torch.loadon it:After the fix, both requests fail fast with
400 Bad Request: Invalid voice_prompt name/Invalid voice_prompt pathand never reachtorch.load. A legitimate request such as?voice_prompt=default.ptcontinues to work unchanged.Testing
../x,/etc/passwd,.,..,foo/bar, and Windows-style..\x.default.pt) still resolves and loads.grepthat/api/chatis the only route consumingvoice_prompt, so no parallel unpatched sink exists.git diff main..HEAD --stat: change is confined tomoshi/moshi/server.py(+19 / -1).Adversarial review
Before submitting we tried to disprove this. Candidate reasons it might not matter:
--host 0.0.0.0is a supported CLI flag and--gradio-tunnelexplicitly exposes the server to the public internet, both documented inargparsehelp text inserver.py. Multi-tenant/remote is an intended deployment./api/chat." — There isn't. The route is registered directly on the aiohttp router with no middleware and no per-handler token check;greping the file confirms no@authdecorator orweb.middlewaregate.torch.loadis safe now." — It still defaults to pickle-based loading in the torch versions this project targets, and even the file-existence probe alone is a CWE-22 information disclosure. The fix is worthwhile even ignoring the RCE path.We could not find a mitigation that makes the unpatched code safe in the shipped deployment modes.