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/deep-probe-summary.md | 164 +++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 piolium/attack-surface/deep-probe-summary.md (limited to 'piolium/attack-surface/deep-probe-summary.md') diff --git a/piolium/attack-surface/deep-probe-summary.md b/piolium/attack-surface/deep-probe-summary.md new file mode 100644 index 0000000..b3fb8df --- /dev/null +++ b/piolium/attack-surface/deep-probe-summary.md @@ -0,0 +1,164 @@ +# Deep Probe Summary: salmon/minecraft-ui + +**Generated:** 2025-07-29 +**Phase:** P8 — Manual Attack Surface Probe +**Status:** Complete +**Loops:** 1 +**Total findings:** 10 +**CRITICAL:** 1 (P8-003) +**HIGH:** 5 (P8-001, P8-002, P8-004, P8-006, P8-009) +**MEDIUM:** 3 (P8-005, P8-007, P8-010) +**LOW:** 1 (P8-008) + +--- + +## Probe Methodology + +This manual deep probe conducted: + +1. **Full source code review** of all 16 Rust source files (1,540+ lines analyzed) +2. **Attack surface inventory** — 38 HTTP routes catalogued with entry points, sinks, and trust boundaries +3. **Backward reasoning** — trace from sinks (file write, RCON command, Docker env, SQL) back to attacker inputs +4. **Contradiction reasoning** — identify gaps between intended security boundaries and actual code +5. **Cross-model analysis** — chain multiple low-severity issues into higher-severity attack paths +6. **Verification** — all findings verified with file:line code evidence via read/grep + +--- + +## Finding Summary + +| ID | Title | Severity | Category | Attack Vector | Existing Draft | +|----|-------|----------|----------|---------------|---------------| +| P8-001 | MOTD Docker Environment Injection | **HIGH** | CWE-77, CWE-93 | Newline injection in MOTD overrides Docker container env vars | Enhanced from p4-016 | +| P8-002 | RCON Command Injection | **HIGH** | CWE-77, CWE-88 | Unsanitized username in RCON command strings | Updated p4-002 | +| P8-003 | Instance Name Path Traversal Chain | **CRITICAL** | CWE-22 | `..` in instance name enables arbitrary file write/read/delete | Expanded p4-001 | +| P8-004 | Level-Seed Properties Injection | **HIGH** | CWE-93, CWE-88 | Seed with newlines injects arbitrary Java properties | Enhanced p4-020 | +| P8-005 | No Server-Side Session Expiration | **MEDIUM** | CWE-613 | Stolen session cookies never expire | Confirmed p4-004 | +| P8-006 | Missing CSRF Protection | **HIGH** | CWE-352 | All POST endpoints vulnerable to cross-site request forgery | Confirmed p4-003 | +| P8-007 | POST /logout Missing Auth Guard | **MEDIUM** | CWE-306 | Logout handler not behind require_auth middleware | Confirmed p4-008 | +| P8-008 | Weak RCON Password Generation | **LOW** | CWE-338 | thread_rng() used instead of OsRng | Confirmed p4-007 | +| P8-009 | RCON Password Exposed to All Users | **HIGH** | CWE-200 | rcon_password in edit form for any authenticated user | Confirmed p4-006 | +| P8-010 | Mod JAR Validation Denial of Service | **MEDIUM** | CWE-770, CWE-674 | Crafted JAR constant pool, no timeout, no recursion limit | Enhanced p4-014 | + +--- + +## Key Findings — Detailed Analysis + +### P8-003: Instance Name Path Traversal Chain (CRITICAL) + +**The most severe finding.** The instance `name` field is used in 8 different filesystem operations without any sanitization. Rust's `Path::join()` follows `..` components, enabling: + +- **Arbitrary directory creation** via `create_dir_all` at instance creation +- **Arbitrary file write** via mod upload (`fs::write`) and world import (`File::create`) +- **Arbitrary directory deletion** via `remove_dir_all` at instance deletion +- **Arbitrary file read** via world export and backup download + +The full attack chain: +1. Create instance with `name=../../../tmp/pwned` +2. Upload mod/world to write files at `/tmp/pwned/data/mods/evil.so` +3. Delete instance to remove `/tmp/pwned` directory tree + +**Code path:** `src/routes/instances.rs:205` — `Path::join(&form.name)` with no validation → `std::fs::create_dir_all()` + +### P8-001: MOTD Docker Environment Injection (HIGH) + +The MOTD field is passed directly into Docker container environment variables without newline escaping. Docker treats `\n` in env values as variable delimiters, allowing injection of arbitrary env vars. + +**Code path:** `src/docker.rs:128` — `format!("MOTD={}", instance.motd)` → Docker container env → injected vars override security settings like `EULA`, `ONLINE_MODE`. + +### P8-002: RCON Command Injection (HIGH) + +The `player_action!` macro constructs RCON commands via `format!($command, form.username)` with no sanitization. Spaces, semicolons, and special characters in usernames can inject arbitrary RCON commands. + +**Code path:** `src/routes/players.rs:97` — `format!("op {}", form.username)` → `client.send_command_async(cmd)` → Minecraft server console. + +### P8-006: Missing CSRF Protection (HIGH) + +All 31 POST endpoints lack CSRF tokens. The session cookie uses `SameSite=Lax` which allows cross-site top-level POST navigations. Combined with other findings (path traversal, RCON injection, instance deletion), CSRF enables fully weaponized cross-site attacks. + +### P8-009: RCON Password Exposure (HIGH) + +The `GET /instances/{id}/edit` endpoint returns `rcon_password` for any instance to any authenticated user. No authorization check beyond `require_auth`. + +**Code path:** `src/routes/instances.rs:313` — `rcon_password: row.get(15)?` — rendered in `EditInstanceTemplate`. + +### P8-004: Level-Seed Properties Injection (HIGH) + +The `seed` field in world regeneration is written to `server.properties` without escaping. Newlines and special characters inject arbitrary Java `.properties` entries. + +**Code path:** `src/routes/instances.rs:397` — `format!("level-seed={seed}")` → `std::fs::write(&props_path)`. + +--- + +## Cross-Model Attack Chains + +### Chain 1: CSRF + Path Traversal for Full System Compromise +1. CSRF (P8-006) tricks authenticated admin to create instance with traversed name +2. Path traversal (P8-003) enables writing arbitrary files outside data directory +3. Combined impact: full file system write without user interaction + +### Chain 2: RCON Password Exposure + Command Injection +1. Any authenticated user views RCON password (P8-009) +2. Same user exploits RCON command injection (P8-002) for server compromise +3. Combined impact: horizontal privilege escalation to Minecraft server operator + +### Chain 3: Session Leak + No TTL +1. Session cookie leaked through any channel (logs, DB backup, etc.) +2. No server-side TTL (P8-005) means session never expires +3. POST /logout (P8-007) allows anonymous invalidation of leaked session +4. Combined impact: indefinite session hijacking window + +--- + +## Coverage Summary + +| Entry Point | Finding Coverage | Notes | +|------------|-----------------|-------| +| POST /login | P8-006 (CSRF) | No rate limiting finding also relevant | +| POST /logout | P8-006 (CSRF), P8-007 (Missing Auth) | Not behind require_auth | +| GET /static/* | None | Safe — static files only | +| POST /instances/new | P8-003 (Path Traversal), P8-006 (CSRF) | CRITICAL chain | +| POST /instances/{id}/edit | P8-001 (MOTD injection), P8-006 (CSRF) | Docker env injection path | +| POST /instances/{id}/delete | P8-003 (Path Traversal), P8-006 (CSRF) | Directory deletion | +| POST /instances/{id}/activate | P8-001 (MOTD injection chain) | Docker container start | +| POST /instances/{id}/world/regenerate | P8-004 (Properties Injection), P8-006 (CSRF) | server.properties injection | +| GET /instances/{id}/edit | P8-009 (RCON password exposure) | Sensitive data leak | +| POST /players/*/add, /players/*/remove | P8-002 (RCON command injection), P8-006 (CSRF) | 6 endpoints | +| POST /instances/{id}/mods | P8-003 (Path traversal via mod name), P8-010 (DoS), P8-006 (CSRF) | File write + DoS | +| POST /instances/{id}/mods/{mid}/delete | P8-003 (Path traversal), P8-006 (CSRF) | File delete | +| POST /instances/{id}/mods/rescan | P8-010 (DoS), P8-006 (CSRF) | File read + validation | +| GET /instances/{id}/world/export | P8-003 (Path traversal read) | File read | +| POST /instances/{id}/world/import | P8-003 (Path traversal write), P8-006 (CSRF) | File write (ZIP) | +| GET /backups/{id}/download | P8-003 (Path traversal read) | File read | +| POST /instances/{id}/backups/create | P8-006 (CSRF) | File write | +| GET /accounts, POST /accounts/* | P8-006 (CSRF) | Superuser operations | +| POST /change-password | P8-006 (CSRF) | Password change via CSRF | +| All authenticated routes | P8-005 (No session TTL) | Persistent session hijacking | +| Session management | P8-005, P8-007 | No TTL, logout without auth | + +--- + +## Missing Coverage / Gaps + +1. **No rate limiting** — `POST /login` has no brute-force protection (existing p4-005, not re-drafted here) +2. **No CSRF on any POST** — universal issue (P8-006) +3. **Flat authorization model** — any authenticated user can access any instance (by-design for single-user/small-team) +4. **No audit logging** — no record of state-changing operations (existing p4-017) +5. **No encryption at rest** — SQLite database unencrypted on disk (existing p4-021) +6. **No backup restore functionality** — `restore_backup` is a stub (existing p4-024) +7. **Cookie without Secure flag** — missing when behind HTTPS proxy (existing p4-009) + +--- + +## Recommendations (Priority Order) + +1. **CRITICAL: Sanitize instance name** — validate against `^[a-zA-Z0-9_-]+$` before filesystem use +2. **HIGH: Strip newlines from Docker env vars** — sanitize MOTD and all user-controlled fields before Docker env injection +3. **HIGH: Validate RCON usernames** — restrict to `[a-zA-Z0-9_]` (3-16 chars) before command construction +4. **HIGH: Add CSRF protection** — SameSite=Strict cookie + CSRF tokens or origin validation +5. **HIGH: Remove RCON password from edit form** — expose only to superusers +6. **HIGH: Escape seed value in server.properties** — reject newlines and control characters +7. **MEDIUM: Add server-side session TTL** — filter sessions by `created_at > datetime('now', '-1 day')` +8. **MEDIUM: Move logout behind require_auth** — defense-in-depth +9. **MEDIUM: Add timeout to mod validation** — prevent DoS via crafted JARs +10. **LOW: Use OsRng for RCON password** — cryptographic PRNG for secrets -- cgit v1.2.3