Files

MechaCat02 ec4a2aa24a style: cargo fmt across audit-2026-06-11 tier-3 changes

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

2026-06-12 18:38:28 +02:00

12 KiB

Raw Blame History

Security Audit — File Storage & Path Traversal

Scope: v1.1.5 files::* SDK + admin API; FS layout <PICLOUD_FILES_ROOT>/files/<app_id>/<collection>/<id[0:2]>/<id>. Reviewed files:

/home/fabi/PiCloud/crates/manager-core/src/files_service.rs
/home/fabi/PiCloud/crates/manager-core/src/files_repo.rs
/home/fabi/PiCloud/crates/manager-core/src/files_api.rs
/home/fabi/PiCloud/crates/manager-core/src/files_sweep.rs
/home/fabi/PiCloud/crates/shared/src/files.rs
/home/fabi/PiCloud/crates/executor-core/src/sdk/files.rs
/home/fabi/PiCloud/dashboard/src/routes/apps/[slug]/files/+page.svelte
/home/fabi/PiCloud/dashboard/src/lib/api.ts
/home/fabi/PiCloud/caddy/Caddyfile

Counts by severity

Severity	Count
Critical	0
High	2
Medium	4
Low	4
Info	2

Findings

High

F-FS-001 — Missing MIME allowlist enables stored XSS via `Content-Disposition: inline` on the admin download endpoint

Severity: High (storage half — rendering half is agent 7's call)
Location: crates/manager-core/src/files_api.rs:153-161 (get_file), crates/shared/src/files.rs (NewFile::validate)
Summary: get_file echoes the script-supplied content_type straight into the response Content-Type header, paired with Content-Disposition: inline; filename="...". There is no allowlist on content_type at the SDK boundary — NewFile::validate only caps its length at 127 bytes. A Rhai script can files::create(#{ name: "x.svg", content_type: "image/svg+xml", data: <svg onload=...> }) (or text/html), and any logged-in operator who clicks Download in the dashboard renders the attacker payload same-origin under /api/v1/admin/..., with the picloud admin session cookie attached. That is admin-session theft.
Storage-side fix: maintain a small allowlist (image/png|jpeg|gif|webp, application/pdf, application/octet-stream, text/plain, application/json) at NewFile::validate + FileUpdate::validate. Anything else stored becomes application/octet-stream on serve, OR the admin endpoint forces Content-Disposition: attachment and X-Content-Type-Options: nosniff + Content-Security-Policy: sandbox for non-allowlisted types.
Cross-ref: agent 7 owns the rendering / response-header conclusion (CSP, sandbox, nosniff, force-attachment). Flag jointly.

F-FS-002 — Admin files API skips collection validation; underlying `head`/`get`/`list`/`delete` skip the FS-path guard too

Severity: High (defense-in-depth gap; not exploitable today, but one bug away from arbitrary-FS-read/write/unlink)
Location: crates/manager-core/src/files_api.rs:78-184 (no validate_collection); crates/manager-core/src/files_repo.rs:321,395 (guard_collection is only called from create and update — head, get, list, delete do not call it).
Summary: The admin endpoints accept collection as a path/query parameter and pass it raw to the repo. The repo's head/get/list only execute parametrized SQL (no immediate FS access), and delete only builds final_path and remove_files after a matching DB row is found. Today this is safe because create/update validate before insert, so no row can exist with a traversal collection. Tomorrow it's one missed validation away from full filesystem reach. Specifically: FsFilesRepo::get → read_verify_at(..., collection, id, ...) and FsFilesRepo::delete → self.final_path(app_id, collection, id) + std::fs::remove_file(path) both build paths from the unvalidated collection. A future code path that inserts a row with collection = "../../etc" (a buggy migration, an admin restore tool, a backup loader) would immediately give cross-app file-read and unlink.
Fix shape: call validate_files_collection at the top of every admin endpoint (list_files, get_file, delete_file) AND add Self::guard_collection(collection)?; to FsFilesRepo::{head, get, list, delete}. Both layers — depth — because the audit posture for this surface is "no traversal possible regardless of caller."

Medium

F-FS-003 — No per-app storage quota; one tenant can fill the host disk

Severity: Medium (cross-ref agent 8 for the host-DoS angle)
Location: crates/manager-core/src/files_service.rs, crates/manager-core/src/files_repo.rs — no quota check anywhere.
Summary: Only the per-file cap (PICLOUD_FILES_MAX_FILE_SIZE_BYTES, default 100 MB) is enforced. An app can create unlimited rows of up to 100 MB each via any public Rhai script that calls files::create. With multi-tenant isolation as a stated goal, this is the cheapest path to host-level DoS: one anonymous public HTTP script + a loop. CLAUDE.md acknowledges per-app quotas are deferred to v1.2, but the threat surface exists today.
Fix shape: track SUM(size_bytes) GROUP BY app_id in a small counter table (files_app_usage), increment/decrement in the same tx as the files row mutation, reject create/update when projected usage exceeds PICLOUD_FILES_APP_QUOTA_BYTES (default e.g. 5 GB).

F-FS-004 — Content-Disposition filename is not RFC 5987-encoded; non-ASCII names lose data or break the header

Severity: Medium
Location: crates/manager-core/src/files_api.rs:153-156
Summary:
```
let disposition = format!(
    "inline; filename=\"{}\"",
    meta.name.replace('"', "").replace(['\r', '\n'], "")
);
```
CRLF and " are stripped (good — no header injection), but no RFC 5987 filename*=UTF-8''<pct-encoded> form. Non-ASCII filenames (café.pdf, 合同.pdf) either round-trip through a lossy ASCII strip (browsers vary), or become an invalid filename="..." token if any high-bit byte is preserved. Combined with inline, the response then renders without an obvious filename, weakening the user's ability to spot a malicious upload.
Fix shape: emit both forms — filename="<ASCII-sanitized fallback>"; filename*=UTF-8''<percent-encoded> — per RFC 6266 §5.

F-FS-005 — Temp file mode honors umask (typically 0644) instead of 0600

Severity: Medium (downgraded by the 0o700 shard-dir guard, but defense-in-depth misses)
Location: crates/manager-core/src/files_repo.rs:267 — std::fs::File::create(&tmp)
Summary: The temp file gets default OpenOptions::create(true).write(true).truncate(true) mode, masked through the process umask. On a typical Linux box that's 0644. The 0o700 parent shard dir protects against world access today, but if PICLOUD_FILES_ROOT ever lives somewhere mounted with nosuid,nodev for a service user, or if the operator chowns the root to a less-restrictive group, file bytes leak.
Fix shape: on Unix, set OpenOptionsExt::mode(0o600) on the temp file create; then set_permissions(0o600) after rename to harden against the umask not applying as expected.

F-FS-006 — No symlink-aware path resolution; a stray symlink inside the root escapes

Severity: Medium (theoretical — there is no code path that creates symlinks under PICLOUD_FILES_ROOT, but the sweeper and the read path do not detect them)
Location: crates/manager-core/src/files_repo.rs:282-311 (read_verify_at); crates/manager-core/src/files_sweep.rs:75-111 (walk)
Summary: read_verify_at opens via std::fs::read(&path) which follows symlinks. The orphan sweeper's walk uses entry.file_type() (which returns the symlink type if applicable), and skips non-files — symlinks-to-files would be skipped, but symlinks-to-dirs are also skipped, OK. The bigger gap is read: if any code path (a future backup-restore, an operator drop-in, a docker volume mount) plants a symlink at <root>/files/<app>/<col>/<sh>/<id> pointing to /etc/shadow, a subsequent get_file reads it. There's no canonicalize + prefix check.
Fix shape: in read_verify_at / write_atomic_at, after constructing the path, canonicalize it and verify the canonical form starts with the canonicalized files-root; reject otherwise. For the sweeper, use symlink_metadata instead of metadata to avoid traversing.

Low

Severity: Low (auth is required via cookie — but the URL is unbearer-able)
Location: crates/manager-core/src/files_api.rs:130-165, dashboard/src/lib/api.ts:956-957
Summary: api.files.downloadUrl(...) returns a plain /api/v1/admin/... URL relying on the picloud_session cookie for auth. Authn is enforced (auth middleware + require(... AppFilesRead)), so it is not open. The only concern is operator hygiene: if an admin copies the URL and shares it (intending to share the file), the recipient gets auth-free 403 instead of the file — fine, defense-wise. No fix required; just noting.

F-FS-008 — Filename-byte-length cap of 255 doesn't account for Windows / FAT-style ceilings or POSIX `NAME_MAX` on platforms with smaller caps

Severity: Low
Location: crates/shared/src/files.rs:27,197
Summary: 255 bytes is fine on ext4/btrfs/xfs. On some platforms (HFS+: 255 UTF-16 code units; ZFS: usually OK), edge cases. The name is also reflected into Content-Disposition and the dashboard table, so the cap is more a header-hygiene concern than an FS one.

F-FS-009 — `head`/`get`/`list` don't reject NUL bytes in `collection` before SQL

Severity: Low (Postgres rejects NUL in TEXT, so the SQL fails cleanly with a 500 — but the user sees "internal error" instead of "invalid collection")
Location: crates/manager-core/src/files_repo.rs:347-516
Summary: Adding Self::guard_collection (see F-FS-002) gives a precise 4xx instead of a 5xx for \0 in collection names.

F-FS-010 — `Content-Length` from `bytes.len()` is set, but no `Cache-Control` / `X-Content-Type-Options` on the download

Severity: Low
Location: crates/manager-core/src/files_api.rs:158-164
Summary: Missing X-Content-Type-Options: nosniff lets the browser MIME-sniff a stored text/plain payload into text/html and render JS. Pairs with F-FS-001 as the response-header hardening pass.

Info

F-FS-011 — File IDs are server-side `Uuid::new_v4()`; safe from caller-controlled bytes

Location: crates/manager-core/src/files_repo.rs:322 and :268 (in-memory test). UUIDs are formatted via to_string() (lowercase hex + dashes, never traversal-shaped). Good.

F-FS-012 — Atomic write protocol is correct; delete order is correct

Location: files_repo.rs:217-277 and :431-473. Temp + fsync + rename + dir-fsync; row delete inside tx then unlink after commit. The "metadata gone, bytes still on disk" leak is bounded (rare crash window after the tx commit) and is reclaimable by a future reconcile sweep (currently only *.tmp.* orphans are reaped by files_sweep.rs). No bug — just noting the design is sound and the residual leak is documented.

Cross-references for other agents

Agent 7 (XSS / rendering): F-FS-001 — the storage side allows arbitrary content_type; rendering side owns CSP / X-Content-Type-Options: nosniff / forced attachment. Joint sev = High.
Agent 8 (DoS / resource): F-FS-003 (no per-app quota) — same concern they may have raised for kv::set / docs::* / pubsub / queue unbounded payloads (cf. F-S-001 in AUDIT.md). Files has a per-row cap, but no per-app aggregate.
Agent 10 (CLI / config files): CLI credentials are 0o600 (verified, crates/picloud-cli/src/config.rs:98,107); no finding from the FS angle.

Most-important takeaway

Today nothing escapes PICLOUD_FILES_ROOT — create and update guard collection, and FS paths are only ever built from server-generated UUIDs and validated collections. The exposure that is live is F-FS-001: a public-route script can stash text/html or image/svg+xml content that the operator's browser renders same-origin under the admin session cookie. The exposure that is one mistake away is F-FS-002: every read/delete path through the admin API trusts collection-validation done elsewhere, and the repo's head/get/list/delete don't have the belt-and-suspenders guard that create/update carry. Add the missing validate_files_collection calls at the admin layer and guard_collection calls at the repo layer, and the surface becomes traversal-proof regardless of caller. The MIME allowlist + Content-Disposition: attachment for non-image types closes the stored-XSS class jointly with agent 7's response-header pass.

12 KiB Raw Blame History