rbrooks/Circa

Watch

Fork 0

Arbitrary file write via unsanitized upload filename (RCE) #55

New issue

Closed

opened 2026-07-28 05:57:09 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-28 05:57:09 +00:00

Severity: CRITICAL

Found by the security audit of 2026-07-28, independently confirmed three ways.

The bug

backend/app/api/routes/ingest.py:58 and :66 build a destination path by joining a
temp directory with the client-supplied filename, then write client-supplied bytes:

front_path = tmp / (front.filename or "front")
data = await front.read()
front_path.write_bytes(data)

Two facts make this arbitrary file write:

Starlette does not sanitize UploadFile.filename — verified by reading the installed
python_multipart/multipart.py and starlette/formparsers.py. No basename, no separator
stripping. The value comes straight from the Content-Disposition header.
In Python, joining an absolute path discards the left operand entirely:
Path("/tmp/xyz") / "/etc/passwd" is /etc/passwd. Traversal (../) also works and
resolves through write_bytes.

_validate_upload checks only content_type. The filename is never inspected.

Attack

The route requires require_role(reviewer, admin) — which, per the companion issue on
auto-provisioning, any account the IdP accepts already satisfies. The payload needs no
knowledge of the install path:

Content-Disposition: form-data; name="front"; filename="/proc/self/cwd/app/api/routes/health.py"

/proc/self/cwd resolves to the uvicorn working directory. With --reload (the command in
the README) it executes immediately; otherwise on next restart. Remote code execution as
the app user.

Equally damaging variants using the same primitive:

filename="/proc/self/cwd/circa.db" — overwrite the database. Total loss of every dating
decision, evidence row, comment, and the audit log.
filename="/proc/self/cwd/storage/<collection_id>/ab/<sha>_front.jpg" — overwrite an
original scan. Both components of the storage key are returned to any authenticated user
by GET /api/photos. This bypasses LocalStorage.put's no-overwrite guard entirely,
because the write never goes through LocalStorage.

Why this matters

docs/circa-spec.md §6.1 states originals are never modified and §6.4 states nothing is ever
hard-deleted. Both are false while this exists. Backups (§6.2/§6.3) are unbuilt, so overwrite
is permanent. The originals are being kept but are degrading, so a rescan is not a clean
recovery path.

Fix

Never let the client filename become a path. Derive the temp name server-side and keep the
original string as data only:

def _safe_temp_path(tmp: Path, upload: UploadFile, fallback: str) -> Path:
    raw = os.path.basename(upload.filename or fallback).replace("\x00", "")
    ext = Path(raw).suffix.lower()
    if ext not in {".jpg", ".jpeg", ".png", ".tif", ".tiff"}:
        raise HTTPException(415, "Unsupported file extension")
    return tmp / f"{uuid.uuid4()}{ext}"

Pass the sanitized raw as original_filename for display and filename parsing. As
defence in depth, assert dest.resolve().is_relative_to(tmp.resolve()) before writing, and
run the app as a user with no write permission to its own source tree.

Done when

No client-controlled string can influence a filesystem path anywhere in ingest
Absolute-path and traversal filenames are rejected or neutralized, covered by tests
original_filename still records what the user uploaded, for parsing and display
A test asserts a write cannot escape the temp directory
Deployment docs state the app user must not own its source tree

References

backend/app/api/routes/ingest.py:58,66
backend/app/services/storage.py (LocalStorage.put no-overwrite guard, bypassed here)
docs/circa-spec.md §6.1, §6.4

## Severity: CRITICAL Found by the security audit of 2026-07-28, independently confirmed three ways. ## The bug `backend/app/api/routes/ingest.py:58` and `:66` build a destination path by joining a temp directory with the **client-supplied** filename, then write client-supplied bytes: ```python front_path = tmp / (front.filename or "front") data = await front.read() front_path.write_bytes(data) ``` Two facts make this arbitrary file write: - Starlette does **not** sanitize `UploadFile.filename` — verified by reading the installed `python_multipart/multipart.py` and `starlette/formparsers.py`. No `basename`, no separator stripping. The value comes straight from the `Content-Disposition` header. - In Python, joining an **absolute** path discards the left operand entirely: `Path("/tmp/xyz") / "/etc/passwd"` is `/etc/passwd`. Traversal (`../`) also works and resolves through `write_bytes`. `_validate_upload` checks only `content_type`. The filename is never inspected. ## Attack The route requires `require_role(reviewer, admin)` — which, per the companion issue on auto-provisioning, any account the IdP accepts already satisfies. The payload needs no knowledge of the install path: ``` Content-Disposition: form-data; name="front"; filename="/proc/self/cwd/app/api/routes/health.py" ``` `/proc/self/cwd` resolves to the uvicorn working directory. With `--reload` (the command in the README) it executes immediately; otherwise on next restart. **Remote code execution as the app user.** Equally damaging variants using the same primitive: - `filename="/proc/self/cwd/circa.db"` — overwrite the database. Total loss of every dating decision, evidence row, comment, and the audit log. - `filename="/proc/self/cwd/storage/<collection_id>/ab/<sha>_front.jpg"` — **overwrite an original scan.** Both components of the storage key are returned to any authenticated user by `GET /api/photos`. This bypasses `LocalStorage.put`'s no-overwrite guard entirely, because the write never goes through `LocalStorage`. ## Why this matters `docs/circa-spec.md` §6.1 states originals are never modified and §6.4 states nothing is ever hard-deleted. Both are false while this exists. Backups (§6.2/§6.3) are unbuilt, so overwrite is permanent. The originals are being kept but are degrading, so a rescan is not a clean recovery path. ## Fix Never let the client filename become a path. Derive the temp name server-side and keep the original string as data only: ```python def _safe_temp_path(tmp: Path, upload: UploadFile, fallback: str) -> Path: raw = os.path.basename(upload.filename or fallback).replace("\x00", "") ext = Path(raw).suffix.lower() if ext not in {".jpg", ".jpeg", ".png", ".tif", ".tiff"}: raise HTTPException(415, "Unsupported file extension") return tmp / f"{uuid.uuid4()}{ext}" ``` Pass the sanitized `raw` as `original_filename` for display and filename parsing. As defence in depth, assert `dest.resolve().is_relative_to(tmp.resolve())` before writing, and run the app as a user with no write permission to its own source tree. ## Done when - [ ] No client-controlled string can influence a filesystem path anywhere in ingest - [ ] Absolute-path and traversal filenames are rejected or neutralized, covered by tests - [ ] `original_filename` still records what the user uploaded, for parsing and display - [ ] A test asserts a write cannot escape the temp directory - [ ] Deployment docs state the app user must not own its source tree ## References - `backend/app/api/routes/ingest.py:58,66` - `backend/app/services/storage.py` (`LocalStorage.put` no-overwrite guard, bypassed here) - `docs/circa-spec.md` §6.1, §6.4

claude-bot added this to the v0.1.1 milestone

2026-07-28 05:57:09 +00:00

claude-bot added the

bug

backend

security

labels

2026-07-28 05:57:09 +00:00

claude-bot referenced this issue

2026-07-28 06:07:51 +00:00

Implement multi-user roles and review attribution #22

rbrooks referenced this issue from a commit

2026-07-28 17:56:50 +00:00

Close the ingest RCE chain: sanitize filenames, deny by default

rbrooks closed this issue

2026-07-28 17:56:50 +00:00

claude-bot commented

2026-07-28 17:59:08 +00:00

Author

Fixed in 6268f9e. CI green.

What changed

safe_temp_path() builds the destination from a server-generated UUID plus an allowlisted
extension, so no client-supplied string reaches the filesystem path:

name = sanitize_filename(raw_filename, fallback)
ext = Path(name).suffix.lower()
if ext not in ALLOWED_EXTENSIONS:
    raise HTTPException(415, ...)
dest = tmp / f"{uuid.uuid4()}{ext}"
if not dest.resolve().is_relative_to(tmp.resolve()):
    raise HTTPException(400, "Invalid filename")

The containment assertion is redundant given the construction above it — it is there so a later
refactor that reintroduces client input into the path fails loudly instead of silently restoring
the vulnerability.

sanitize_filename() strips null bytes, normalises Windows separators to / before taking the
basename (so a client on either platform cannot smuggle a directory component), and falls back for
"", ".", and "..". The sanitized value is still passed as original_filename, because it is
displayed to reviewers and parsed for album and sequence hints — the fix must not break #105.

Tests

backend/tests/test_upload_filename_safety.py, parameterised over the real payloads from the audit:

/etc/passwd, /proc/self/cwd/app/api/routes/health.py, /proc/self/cwd/circa.db
../../../../etc/cron.d/evil, ../../circa.db, ..\\..\\windows\\system32\\...
subdir/nested/payload.jpg, /absolute/path/photo.jpg

Each is checked twice: that the returned path is inside the temp directory, and that actually
writing through it leaves nothing outside — rglob asserts the destination is the only file
produced. Plus null bytes, extension allowlisting, collision safety between two uploads of the same
name, and that album/sequence filename hints survive sanitizing.

Not addressed here, deliberately

The route still allows reviewer, though its docstring says "Admin-only in Phase 1". Tightening
that belongs to #73 with the rest of the permission matrix.
Streaming and the pre-check RAM buffer are #86.
Magic-byte validation and Pillow limits are #63 and #65.
The audit also recommends running the app as a user without write access to its own source tree.
That belongs with the deployment work in #50 and is not enforceable from application code.

**Fixed** in 6268f9e. CI green. ## What changed `safe_temp_path()` builds the destination from a server-generated UUID plus an allowlisted extension, so no client-supplied string reaches the filesystem path: ```python name = sanitize_filename(raw_filename, fallback) ext = Path(name).suffix.lower() if ext not in ALLOWED_EXTENSIONS: raise HTTPException(415, ...) dest = tmp / f"{uuid.uuid4()}{ext}" if not dest.resolve().is_relative_to(tmp.resolve()): raise HTTPException(400, "Invalid filename") ``` The containment assertion is redundant given the construction above it — it is there so a later refactor that reintroduces client input into the path fails loudly instead of silently restoring the vulnerability. `sanitize_filename()` strips null bytes, normalises Windows separators to `/` before taking the basename (so a client on either platform cannot smuggle a directory component), and falls back for `""`, `"."`, and `".."`. The sanitized value is still passed as `original_filename`, because it is displayed to reviewers and parsed for album and sequence hints — the fix must not break #105. ## Tests `backend/tests/test_upload_filename_safety.py`, parameterised over the real payloads from the audit: - `/etc/passwd`, `/proc/self/cwd/app/api/routes/health.py`, `/proc/self/cwd/circa.db` - `../../../../etc/cron.d/evil`, `../../circa.db`, `..\\..\\windows\\system32\\...` - `subdir/nested/payload.jpg`, `/absolute/path/photo.jpg` Each is checked twice: that the returned path is inside the temp directory, and that **actually writing through it** leaves nothing outside — `rglob` asserts the destination is the only file produced. Plus null bytes, extension allowlisting, collision safety between two uploads of the same name, and that album/sequence filename hints survive sanitizing. ## Not addressed here, deliberately - The route still allows `reviewer`, though its docstring says "Admin-only in Phase 1". Tightening that belongs to #73 with the rest of the permission matrix. - Streaming and the pre-check RAM buffer are #86. - Magic-byte validation and Pillow limits are #63 and #65. - The audit also recommends running the app as a user without write access to its own source tree. That belongs with the deployment work in #50 and is not enforceable from application code.

rbrooks referenced this issue from a commit

2026-07-30 18:21:01 +00:00

Stop exposing user ids; enforce the role matrix

claude-bot referenced this issue

2026-07-30 18:21:47 +00:00

No admin-only routes exist and the viewer role is unenforced #73

rbrooks referenced this issue from a commit

2026-07-30 23:08:25 +00:00

Run Pillow in a resource-limited child process

claude-bot referenced this issue

2026-07-30 23:08:49 +00:00

Attacker-controlled images reach Pillow with no limits or isolation #65