Skip to content

fix(uploads): prevent path traversal in reference audio upload endpoints - #37

Open
sebastionoss wants to merge 1 commit into
BazedFrog:mainfrom
sebastionoss:fix/cwe22-main-reference-1aeb
Open

fix(uploads): prevent path traversal in reference audio upload endpoints#37
sebastionoss wants to merge 1 commit into
BazedFrog:mainfrom
sebastionoss:fix/cwe22-main-reference-1aeb

Conversation

@sebastionoss

Copy link
Copy Markdown

Summary

The /api/upload-reference and /api/upload-and-trim-reference endpoints in main.py write uploaded files to disk using the client-supplied file.filename value without stripping path components. A crafted filename such as ../../../../tmp/pwned.wav causes the server to write outside UPLOADS_DIR, which is a path traversal vulnerability (CWE-22).

Both endpoints do prefix the filename with a UUID (f"{file_id}_{file.filename}"), but ../ sequences inside file.filename are still honoured by Path / open(), so the UUID prefix does not neutralize the traversal — the resulting path just becomes UPLOADS_DIR/<uuid>_../../../tmp/pwned.wav, which resolves outside the uploads directory.

The file-extension check (endswith(allowed_ext)) only restricts the suffix, so an attacker can simply append .wav (e.g. ../../../../home/user/.ssh/authorized_keys.wav — or more realistically, overwrite arbitrary .wav/config files under the app user's write scope).

Affected code

main.py lines ~416–470, functions upload_reference and upload_and_trim_reference.

Fix

Sanitize the client-supplied filename with Path(file.filename).name, which returns only the final path component and discards any directory traversal segments. The sanitized value is then used for both the on-disk path and the JSON response. This is a minimal, targeted change — 11 additions / 4 deletions — and leaves all other upload/trim logic intact.

safe_filename = Path(file.filename).name
file_path = UPLOADS_DIR / f"{file_id}_{safe_filename}"

Reproduction

With the server running (default uvicorn main:app, listening on 127.0.0.1:8000 unless --host is overridden):

# Create a payload
echo "pwn" > /tmp/payload.wav

# Craft an upload with a traversal filename
curl -v -F 'file=@/tmp/payload.wav;filename=../../../../tmp/pwned.wav' \
     http://127.0.0.1:8000/api/upload-reference

# On a vulnerable (pre-fix) build, a file matching
#   /tmp/<uuid>_../../../../tmp/pwned.wav  →  /tmp/pwned.wav (or nearby)
# is written outside UPLOADS_DIR. With the fix applied, the file is written
# as UPLOADS_DIR/<uuid>_pwned.wav and no traversal occurs.

I verified locally that Path("../../../../tmp/pwned.wav").name == "pwned.wav", and that the post-fix code path produces only basenames under UPLOADS_DIR.

Threat model / preconditions

  • Network reachability to the FastAPI server. The app defaults to binding on localhost, which limits exposure to a local user on single-user Pinokio installs. However, --host is user-configurable and it is common for users to run with --host 0.0.0.0 for LAN access (e.g. accessing the UI from another machine on the same network).
  • No authentication is required on either upload endpoint — I confirmed there is no router-level dependencies=[Depends(...)] gate and no app-level auth middleware in main.py.

Impact is therefore bounded on a strict localhost deployment but becomes a real arbitrary-write primitive (limited to the app user's permissions, with any extension the attacker chooses via suffix) whenever the server is exposed on a LAN or reachable through a reverse proxy.

Adversarial review

Before submitting I tried to disprove this: I checked whether the UUID prefix or the extension whitelist neutralizes the traversal, and neither does — the UUID becomes part of a filename fragment that Path happily walks out of with ../, and the extension check operates on the untrusted string so appending .wav bypasses it. I also verified there is no framework-level auth gate on the affected router that would prevent an unauthenticated attacker from reaching the endpoints.

Diff stat

 main.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Discovered by the Sebastion AI GitHub App.

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