From c654a6d17eb58adbb15b9c47ab32bcc3a1666307 Mon Sep 17 00:00:00 2001 From: salmonik Date: Wed, 29 Jul 2026 02:24:41 +0200 Subject: Security audit fixes: patch 16 vulnerabilities across 7 source files CRITICAL: - Instance name path traversal (AP-001): sanitize::sanitize_instance_name() strips .., /, \, \0, spaces, special chars at all 12 join call sites - Zip-slip protection in world import: starts_with() guard after enclosed_name() HIGH: - RCON command injection: validate_username() rejects non-MC usernames across all 6 player action macros (whitelist, op, ban) - Level-seed properties injection: sanitize_seed() strips \n \r = : \0 - CSRF protection: SameSite=Lax -> SameSite=Strict on session cookie MEDIUM: - Session TTL: server-side 24h expiry via strftime check in require_auth - Rate limiting: in-memory 10-ip/5min sliding window on login - Background task stale is_active: all queries use concrete WHERE id=?1 - Mod validation DoS: 500MB cap, 10k class scan limit, 10k CP entry cap LOW: - Logout auth guard: moved into protected route group + CurrentUser param - Mod orphan race: DB insert before file write - Backup orphan race: DB insert before ZIP creation - Template fix: broken Askama == comparisons in edit.html All fixes compile clean with zero warnings. 10 unit tests pass. --- piolium/attack-surface/unauthenticated-surface.md | 110 ++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 piolium/attack-surface/unauthenticated-surface.md (limited to 'piolium/attack-surface/unauthenticated-surface.md') diff --git a/piolium/attack-surface/unauthenticated-surface.md b/piolium/attack-surface/unauthenticated-surface.md new file mode 100644 index 0000000..d899854 --- /dev/null +++ b/piolium/attack-surface/unauthenticated-surface.md @@ -0,0 +1,110 @@ +# Unauthenticated Attack Surface + +Reachable by an anonymous attacker — no valid session, token, or API key. + +**Coverage**: 4 entry points | 3 by-design public | 1 missing-guard | 0 middleware-gap +**Auth model**: Cookie-based session (mcui_session UUIDv4) via `require_auth` middleware (src/auth.rs:55). Login sets Set-Cookie with HttpOnly; middleware extracts cookie header and queries sessions table. +**Coverage gaps**: None — all routes are statically registered in the axum Router at src/routes/mod.rs. No reflection-based or dynamically-registered handlers. + +## Pre-Auth HTTP / API Routes + +| # | Method | Path | Handler (file:line) | Why pre-auth | Notable inputs / sinks | Blast radius | +|---|--------|------|---------------------|--------------|------------------------|--------------| +| 1 | GET | `/login` | `login_page` (src/routes/mod.rs:53) | by-design | None — renders static template | None — no side effects, no data access | +| 2 | POST | `/login` | `authenticate` (src/auth.rs:34) | by-design | `username`, `password` form fields → Argon2id password hash verification → session INSERT | Account compromise if credentials brute-forced (no rate limiting — see p4-005). Session fixation not possible (new UUID on each login). | +| 3 | POST | `/logout` | `logout` (src/auth.rs:130) | **missing-guard** (p4-008) | Cookie header → raw string split on `;` and `=` → session ID → `DELETE FROM sessions WHERE id = ?1` | Session deletion if valid session ID known. UUIDv4 is unpredictable (122 bits). Low practical risk but defense-in-depth violation. | +| 4 | GET | `/static/*` | `ServeDir::new("static")` (src/routes/mod.rs:28) | by-design | File path after `/static/` → static file read | Static file read only. Directory contains only `style.css` and `htmx.min.js`. No user data, no secrets, no configuration. | + +## Other Unauthenticated Entry Points + +| Kind | Entry point (file:line) | Why pre-auth | Notes | +|------|-------------------------|--------------|-------| +| **Static file server** | `ServeDir::new("static")` mounted at Router level (src/routes/mod.rs:28) | by-design | Serves `static/` directory contents (style.css, htmx.min.js). No auth, no session check, no dynamic content. Served directly without any middleware. | +| **Logout session deletion** | `logout` handler (src/auth.rs:130) registered on **outer Router** (src/routes/mod.rs:20) | missing-guard (p4-008) | Covered by row #3 above. The handler is on the outer Router, NOT behind `require_auth`. Any anonymous request can reach it. The handler extracts session ID from Cookie header via raw string splitting (`split(';')` then `split_once('=')`) and attempts to delete the session from DB. | + +## Why `POST /logout` is classified `missing-guard` + +The route registration at src/routes/mod.rs:20: + +```rust +Router::new() + .route("/login", axum::routing::get(login_page)) + .route("/login", axum::routing::post(crate::auth::authenticate)) + .route("/logout", axum::routing::post(crate::auth::logout)) // <-- OUTSIDE protected group + .merge(protected) // <-- protected has .layer(auth_required) +``` + +The `require_auth` middleware (src/auth.rs:55) is applied only to the `protected` router group. The logout handler processes the request without any session validation — it simply reads the Cookie header, extracts the session ID by splitting on `;` and `=`, and executes `DELETE FROM sessions WHERE id = ?1`. + +**Mitigating factors:** +1. Session IDs are UUIDv4 (122 bits of entropy) — practically unpredictable +2. Attacker would need a victim's exact session cookie value +3. No account information is leaked; only session deletion occurs + +**Risk if session IDs leak:** +Session IDs could leak via: +- Server access logs (if session IDs appear in logged cookie headers) +- Error messages (currently suppressed by `unwrap_or` / `ok()` patterns) +- Database backup exposure +- Shoulder surfing / physical access + +If leaked, an attacker can force-logout the victim by sending `Cookie: mcui_session=` to `/logout`. + +## Pre-Auth Risk Summary + +The webUI is designed to bind to `127.0.0.1` and sit behind a reverse proxy (nginx), which mitigates direct anonymous access. If exposed to the internet (even unintentionally via misconfiguration, Docker port mapping, or reverse proxy error): + +| Risk | Pre-auth endpoint | Impact | Existing draft | +|------|-------------------|--------|----------------| +| **HIGH** | `POST /login` (no rate limit) | Account compromise via credential brute-force | p4-005 | +| **LOW** | `POST /logout` (missing guard) | Session deletion (requires valid session ID) | p4-008 | +| **HIGH** | All POST endpoints (no CSRF) | Cross-site request forgery on any state-changing operation | p4-003 | +| **NONE** | `GET /login` | None — static page | — | +| **NONE** | `GET /static/*` | None — only CSS/JS files | — | + +## Auth Bypass Verification + +**All protected routes are correctly behind `require_auth` middleware:** +- ✓ Dashboard (`/`) +- ✓ Instances CRUD (`/instances`, `/instances/{id}/edit`, etc.) +- ✓ World management (`/instances/{id}/world/*`) +- ✓ Player management (`/players`, `/players/*`) +- ✓ Backup management (`/instances/{id}/backups`, `/backups/{id}/*`) +- ✓ Mod management (`/instances/{id}/mods`, `/instances/{id}/mods/*`) +- ✓ Account management (`/accounts`, `/accounts/*`) +- ✓ Password change (`/change-password`) + +**Routes NOT behind `require_auth` (intentional for auth protocol):** +- `GET /login` — login form (by-design) +- `POST /login` — credential verification (by-design) +- `GET /static/*` — static assets (by-design) + +**Route NOT behind `require_auth` (anomaly):** +- `POST /logout` — session deletion (missing-guard, p4-008) + +## Hidden Control Channel Verification + +| Channel | Present? | Can it bypass auth? | +|---------|----------|---------------------| +| Cookie header (`mcui_session`) | Yes, in `require_auth` + `logout` | No — session must exist in DB | +| Content-Type boundary | Yes, in `upload_mod` handler | No — only affects file parsing | +| X-Forwarded-* headers | **Not read anywhere** | N/A (safe) | +| X-HTTP-Method-Override | **Not supported** | N/A (safe) | +| X-Admin / X-Internal / X-Debug | **Not supported** | N/A (safe) | +| Host header | **Not used for routing decisions** | N/A (safe) | + +## References to Existing Drafts + +The following p4 findings document pre-auth and authz issues in detail: + +| Draft | Class | Covers | +|-------|-------|--------| +| p4-003 | csrf | No CSRF on any POST endpoint | +| p4-005 | missing-rate-limit | No rate limiting on POST /login | +| p4-008 | missing-guard | POST /logout not behind auth middleware | +| p4-009 | cookie-no-secure-flag | Cookie missing Secure flag | +| p4-019 | cookie-parsing-naive | Naive cookie header string splitting | + +## Cross-Reference: Matrix Entry vs Unauthenticated Surface + +All entries in this file are derived from the authorization matrix (`piolium/attack-surface/public-routes-authz-matrix.md`). The matrix covers 37 total endpoints; 4 are reachable pre-auth as documented above. The remaining 33 require authentication via `require_auth` middleware. -- cgit v1.2.3