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. --- .../manual-attack-surface-inventory.md | 311 +++++++++++++++++++++ 1 file changed, 311 insertions(+) create mode 100644 piolium/attack-surface/manual-attack-surface-inventory.md (limited to 'piolium/attack-surface/manual-attack-surface-inventory.md') diff --git a/piolium/attack-surface/manual-attack-surface-inventory.md b/piolium/attack-surface/manual-attack-surface-inventory.md new file mode 100644 index 0000000..c1793e4 --- /dev/null +++ b/piolium/attack-surface/manual-attack-surface-inventory.md @@ -0,0 +1,311 @@ +# Manual Attack Surface Inventory — Phase 8 Deep Probe + +**Target:** salmon/minecraft-ui +**Generated:** 2025-07-29 +**Phase:** P8 — Manual Attack Surface Probe +**Scope:** All Rust source files (16 files), templates, config + +--- + +## 1. Public Routes / URLs (No Auth Required) + +| Method | Path | Handler | Source File:Line | Input Sources | +|--------|------|---------|-----------------|---------------| +| GET | `/login` | `login_page` | src/routes/mod.rs:53 | None (static template) | +| POST | `/login` | `authenticate` | src/auth.rs:34 | `username`, `password` (Form) | +| POST | `/logout` | `logout` | src/auth.rs:130 | Cookie header (Cookie: mcui_session=...) | +| GET | `/static/*` | `ServeDir::new("static")` | src/routes/mod.rs:28 | File path after `/static/` | + +## 2. Authenticated Routes (All Behind `require_auth`) + +| Method | Path | Handler | File:Line | Key Inputs | Sink | +|--------|------|---------|-----------|------------|------| +| GET | `/` | `dashboard` | src/routes/dashboard.rs:19 | CurrentUser extension | SQL SELECT | +| GET | `/instances` | `list_instances` | src/routes/instances.rs:44 | None | SQL SELECT | +| GET | `/instances/new` | `create_form` | src/routes/instances.rs:70 | None | Outbound HTTP (Mojang API) | +| POST | `/instances/new` | `create_instance` | src/routes/instances.rs:87 | `name`, `version`, `seed`, `motd`, etc. | SQL INSERT, `create_dir_all`, RCON password gen | +| GET | `/instances/{id}/edit` | `edit_form` | src/routes/instances.rs:129 | Path `id` (i64) | SQL SELECT, `read_instance_seed`, RCON `seed` cmd | +| POST | `/instances/{id}/edit` | `edit_instance` | src/routes/instances.rs:176 | Path `id`, Form: difficulty, gamemode, motd | SQL UPDATE, `stop_container` | +| POST | `/instances/{id}/delete` | `delete_instance` | src/routes/instances.rs:198 | Path `id` | SQL UPDATE/DELETE, `remove_dir_all` | +| POST | `/instances/{id}/activate` | `activate_instance` | src/routes/instances.rs:217 | Path `id` | Docker `start_container`, RCON polling | +| POST | `/instances/deactivate` | `deactivate_instance` | src/routes/instances.rs:278 | None | Docker `stop_container` | +| GET | `/instances/{id}/world/export` | `export_world` | src/routes/instances.rs:288 | Path `id` | `read_dir`, `ZipWriter`, file read | +| POST | `/instances/{id}/world/import` | `import_world` | src/routes/instances.rs:328 | Path `id`, Multipart ZIP | `ZipArchive`, `File::create`, `remove_dir_all` | +| POST | `/instances/{id}/world/regenerate` | `regenerate_world` | src/routes/instances.rs:360 | Path `id`, Form: `seed` | `read_to_string`, `write`, `remove_dir_all` | +| GET | `/players` | `manage_players` | src/routes/players.rs:28 | None | `read_to_string` (whitelist/ops/bans JSON) | +| POST | `/players/whitelist/add` | `add_whitelist` | src/routes/players.rs:89 | `username` (Form) | RCON `send_command_async` | +| POST | `/players/whitelist/remove` | `remove_whitelist` | src/routes/players.rs:89 | `username` | RCON `send_command_async` | +| POST | `/players/op/add` | `add_op` | src/routes/players.rs:89 | `username` | RCON `send_command_async` | +| POST | `/players/op/remove` | `remove_op` | src/routes/players.rs:89 | `username` | RCON `send_command_async` | +| POST | `/players/ban/add` | `add_ban` | src/routes/players.rs:89 | `username` | RCON `send_command_async` | +| POST | `/players/ban/remove` | `remove_ban` | src/routes/players.rs:89 | `username` | RCON `send_command_async` | +| GET | `/instances/{id}/mods` | `list_mods` | src/routes/mods.rs:42 | Path `id` | SQL SELECT | +| POST | `/instances/{id}/mods` | `upload_mod` | src/routes/mods.rs:86 | Path `id`, Multipart .jar/.zip | `std::fs::write`, JAR parsing, SQL INSERT | +| POST | `/instances/{id}/mods/{mod_id}/delete` | `delete_mod` | src/routes/mods.rs:188 | Path `instance_id`, `mod_id` | `remove_file`, SQL DELETE | +| POST | `/instances/{id}/mods/rescan` | `rescan_mods` | src/routes/mods.rs:163 | Path `id` | `read_dir`, `std::fs::read`, JAR parsing | +| GET | `/instances/{id}/backups` | `list_backups` | src/routes/backups.rs:30 | Path `id` | SQL SELECT | +| POST | `/instances/{id}/backups/create` | `create_backup` | src/routes/backups.rs:54 | Path `id` | `File::create`, `ZipWriter`, SQL INSERT | +| POST | `/backups/{id}/restore` | `restore_backup` | src/routes/backups.rs:88 | Path `id` | SQL SELECT (STUB — no-op) | +| GET | `/backups/{id}/download` | `download_backup` | src/routes/backups.rs:93 | Path `id` | SQL SELECT, `std::fs::read` | +| GET | `/accounts` | `manage_accounts` | src/routes/accounts.rs:60 | CurrentUser (must be superuser) | SQL SELECT | +| POST | `/accounts/create` | `create_account` | src/routes/accounts.rs:95 | `username`, `password`, `is_superuser` | SQL INSERT | +| POST | `/accounts/delete` | `delete_account` | src/routes/accounts.rs:111 | `form.id` | SQL DELETE | +| POST | `/accounts/set-password` | `set_password` | src/routes/accounts.rs:142 | `form.id`, `new_password` | SQL UPDATE, session DELETE | +| GET | `/change-password` | `change_password_page` | src/routes/mod.rs:58 | CurrentUser | None | +| POST | `/change-password` | `change_password` | src/routes/accounts.rs:163 | `current_password`, `new_password` | SQL UPDATE, session DELETE | + +--- + +## 3. Attacker-Controlled Input Sources (By Category) + +### 3a. Form Fields (application/x-www-form-urlencoded) +| Field | Handler(s) | Max Size | Validation | Risk | +|-------|-----------|----------|------------|------| +| `username` | authenticate, player actions, create_account, set_password | ~64KB | None (SQL param prevents injection) | LOW (SQL safe), **HIGH** (RCON injection in player actions) | +| `password` | authenticate, change_password, create_account | ~64KB | Argon2id hashed | LOW | +| `name` | create_instance | ~64KB | **NONE** | **CRITICAL** — used in fs::create_dir_all, stored in DB, used in remove_dir_all | +| `seed` | create_instance, regenerate_world | ~64KB | **NONE** — written to server.properties | **MEDIUM** — properties injection | +| `motd` | create_instance, edit_instance | ~64KB | **NONE** — passed to Docker env | **HIGH** — Docker env injection | +| `version` | create_instance | ~64KB | **NONE** — passed to Docker env | MEDIUM | +| `difficulty` / `gamemode` / `max_players` | create_instance, edit_instance | i32 | Range limited | MEDIUM | +| `memory_mb` | create_instance, edit_instance | i32 | 512..max_memory_mb | LOW | +| `server_type` | create_instance | ~64KB | CHECK constraint in SQL | LOW (DB enforced) | +| `level_type` | create_instance | ~64KB | Default "DEFAULT" | MEDIUM — Docker env | +| `pvp` | create_instance, edit_instance | ~8 chars | Checkbox (Some("on") | LOW | +| `world_source` | create_instance | ~64KB | CHECK constraint in SQL | LOW | + +### 3b. Multipart File Uploads +| Field | Handler | Max Size | Validation | Risk | +|-------|---------|----------|------------|------| +| File bytes (.jar) | upload_mod | 500MB per field | JAR metadata validation + class scan | **HIGH** — file write | +| File bytes (.zip) | upload_mod | 500MB per field | ZIP → extract .jar → JAR validation | **HIGH** — file write, nested ZIP | +| File bytes (.zip) | import_world | Multipart stream | `enclosed_name()` check | **HIGH** — world file write, zip slip edge cases | +| Filename (from Content-Disposition) | upload_mod | ~255 chars | `file_name()` strips dir components | **MEDIUM** — special chars preserved | +| Content-Type / boundary | upload_mod | ~255 chars | **NONE** — directly parsed for multer | **LOW** — multer handles internally | + +### 3c. Path Parameters +| Parameter | Handler(s) | Validation | Risk | +|-----------|-----------|------------|------| +| `id` (i64) | All /instances/{id}/*, /backups/{id}/* | Axum Path (integer only) | LOW — SQL uses params | +| `instance_id` (i64) | delete_mod | Axum Path<(i64, i64)> | LOW | +| `mod_id` (i64) | delete_mod | Axum Path<(i64, i64)> | LOW | + +### 3d. Cookie / Header Sources +| Source | Handler | What It Provides | Risk | +|--------|---------|-----------------|------| +| `Cookie: mcui_session` | require_auth (all protected routes) | Session ID → Account | **MEDIUM** — no server-side TTL | +| `Cookie: mcui_session` | logout | Session ID for deletion | **LOW** — missing auth guard | +| `Content-Type: boundary=` | upload_mod | Multipart parsing boundary | **LOW** | + +--- + +## 4. Sinks (Dangerous Operations) + +### 4a. Filesystem Write Sinks +| Sink | File:Line | Data Source | Traversal Protection | +|------|-----------|-------------|---------------------| +| `std::fs::create_dir_all(&instance_dir)` | src/routes/instances.rs:205 | `form.name` (user input) | **NONE** — `Path::join` traverses on `..` | +| `std::fs::write(&dest, bytes)` | src/routes/mods.rs:98 | `safe_name` (filename from upload) | Partial — `file_name()` strips dir | +| `std::fs::File::create(&out_path)` | src/routes/instances.rs:210 | `file.enclosed_name()` (ZIP entry path) | Partial — `enclosed_name()` checks | +| `std::fs::File::create(&backup_path)` | src/routes/backups.rs:72 | `filename` (timestamp-based) | LOW — controlled by app | +| `std::fs::write(&props_path, ...)` | src/routes/instances.rs:396 | `form.seed` (user input) | **NONE** — content injection | +| `std::fs::remove_dir_all(&instance_dir)` | src/routes/instances.rs:392-393 | `name` from DB (originally user input) | **NONE** | +| `std::fs::remove_dir_all(&world_dir)` | src/routes/instances.rs:421 | Instance name from DB | Partial | +| `std::fs::remove_file(&mod_path)` | src/routes/mods.rs:226 | `filename` from DB | Partial | + +### 4b. Filesystem Read Sinks +| Sink | File:Line | Data Source | Risk | +|------|-----------|-------------|------| +| `std::fs::read(&backup_path)` | src/routes/backups.rs:106 | `filename` from DB | **MEDIUM** — filename from DB could be traversed | +| `std::fs::read_dir(&data_dir)` | src/routes/backups.rs:68 | Instance name from DB | LOW | +| `std::fs::read_to_string(&props_path)` | src/routes/instances.rs:386 | Instance name from DB | LOW | +| `std::fs::read_dir(&mods_dir)` | src/routes/mods.rs:175 | Instance name from DB | LOW | +| `std::fs::read(&path)` | src/routes/mods.rs:183 | File path from read_dir | LOW | + +### 4c. RCON Command Sinks +| Sink | File:Line | Data Source | Command Template | Risk | +|------|-----------|-------------|-----------------|------| +| `client.send_command_async(cmd)` | src/routes/players.rs:97 | `form.username` | `format!("whitelist add {}", form.username)` | **HIGH** — command injection | +| `client.send_command_async(cmd)` | src/routes/players.rs:97 | `form.username` | `format!("op {}", form.username)` | **HIGH** | +| `client.send_command_async(cmd)` | src/routes/players.rs:97 | `form.username` | `format!("ban {}", form.username)` | **HIGH** | +| `client.send_command_async("seed".into())` | src/routes/instances.rs:153 | Hardcoded "seed" | `"seed"` | LOW | +| `client.send_command_async("list".into())` | src/routes/instances.rs:251 | Hardcoded "list" | `"list"` | LOW | + +### 4d. Docker Control Sinks +| Sink | File:Line | Data Source | Risk | +|------|-----------|-------------|------| +| `docker.start_container(&instance)` | src/routes/instances.rs:235 | Full Instance struct | **HIGH** — all settings go to Docker env | +| `docker.stop_container()` | src/routes/instances.rs:195 | None | LOW | +| `recreate_container()` | src/docker.rs:62 | `instance.name`, `instance.motd`, all settings | **HIGH** — env injection | +| `env_strings` construction | src/docker.rs:87-113 | `instance.version`, `instance.motd`, `instance.rcon_password`, etc. | **HIGH** — no escaping | + +### 4e. SQL Query Sinks +All SQL queries use `rusqlite::params![]` macro which provides parameterized queries. SQL injection is not feasible. + +### 4f. Template Rendering Sinks +| Sink | File:Line | Data | Risk | +|------|-----------|------|------| +| `template.render().unwrap_or_default()` | Various | User-controlled strings (name, motd, etc.) | **LOW** — Askama is compile-time template engine, no XSS injection possible | + +--- + +## 5. Hidden Control Channels + +| Channel | File:Line | Processing | What It Controls | Security Concern | +|---------|-----------|-----------|-----------------|-----------------| +| `Cookie: mcui_session` value | src/auth.rs:107 | Raw `split(';')` then `split_once('=')` | Identity (session ID) | Naive cookie parsing, no OWS stripping per RFC 6265. Cookie value with `=` or `;` could confuse parsing | +| `Content-Type: boundary=` | src/routes/mods.rs:141-148 | Custom parsing: `split(';')` → `strip_prefix("boundary=")` → `trim_matches('"')` | Multer multipart boundary | No validation on boundary value; multer validates internally but custom extraction may produce empty boundary | +| Instance properties → Docker env | src/docker.rs:87-113 | Direct `format!()` calls | Docker container environment | **No escaping** — newlines in MOTD or seed can inject arbitrary env vars | +| Instance name → filesystem path | src/routes/instances.rs:205 | `server_directory.join("instances").join(&form.name)` | Filesystem paths | **No sanitization** — `..` in name causes directory traversal | +| RCON password from DB → rendered HTML | src/routes/instances.rs:313 | `row.get(15)` (rcon_password) | Exposed to all authenticated users | Any authenticated user can see the RCON password for any instance | +| Seed → server.properties | src/routes/instances.rs:386-397 | Direct `format!("level-seed={seed}")` | server.properties content | **No escaping** — can inject arbitrary properties lines | +| Username → RCON command | src/routes/players.rs:93-97 | `format!("whitelist add {}", form.username)` | Minecraft server state | **No sanitization** — can inject arbitrary RCON commands | + +--- + +## 6. Exploit-Relevant Paths (Verified with Code Evidence) + +### X1: Instance Name Path Traversal → Arbitrary Directory Write +``` +POST /instances/new → CreateInstanceForm.name = "../../../tmp/evil" +→ src/routes/instances.rs:205: std::fs::create_dir_all(server_dir/instances/../../../tmp/evil/data) +→ SQL INSERT at line 119 with name stored as-is +→ Later: GET /instances/{id}/edit → edit_form reads name from DB +→ Later: POST /instances/{id}/delete → remove_dir_all(server_dir/instances/../../../tmp/evil) +``` +**Evidence:** `Path::join` on user-controlled `form.name` at line 173. No sanitization on name before `create_dir_all` at line 205. + +### X2: RCON Command Injection via Player Username +``` +POST /players/op/add → PlayerAction.username = "attacker; op injected_player" +→ src/routes/players.rs:93: format!("op {}", form.username) +→ src/routes/players.rs:97: client.send_command_async("op attacker; op injected_player") +``` +**Evidence:** `player_action!` macro at line 89 generates direct format string. No sanitization, no validation of username characters. + +### X3: MOTD Docker Environment Injection +``` +POST /instances/{id}/edit → EditInstanceForm.motd = "hello\nEULA=FALSE\nONLINE_MODE=true" +→ SQL UPDATE at line 186 (stored in DB) +→ POST /instances/{id}/activate → reads Instance struct +→ src/docker.rs:103: format!("MOTD={}", instance.motd) → env_strings.push("MOTD=hello\nEULA=FALSE\nONLINE_MODE=true") +→ Docker container env includes injected vars +``` +**Evidence:** `format!("MOTD={}", instance.motd)` at src/docker.rs:103. No escaping of newlines or special characters. + +### X4: Level-Seed Injection into server.properties +``` +POST /instances/{id}/world/regenerate → RegenerateWorldForm.seed = "123\nlevel-type=FLAT\ngamerule=doDaylightCycle=false\n" +→ src/routes/instances.rs:397: format!("level-seed={seed}") +→ Written to server.properties via line replacement (line 390-393) +→ Injected lines become new properties +``` +**Evidence:** Line 397: `new_lines.push(format!("level-seed={seed}"));` No escaping. Java `.properties` format treats newline-separated lines as separate entries. + +### X5: Mod Filename Path Traversal (Partially Mitigated) +``` +POST /instances/{id}/mods → multipart file with filename "../../../tmp/evil.jar" +→ src/routes/mods.rs:98: process_jar_upload → Path::new(filename).file_name() +→ "evil.jar" preserved, "../.." stripped by file_name() +→ Mitigation: streps directory components BUT unsafe write to mods_dir/evil.jar +``` +**Evidence:** `safe_name` derived from `file_name()` at line 96, then `mods_dir.join(&safe_name)` at line 98. + +### X6: No Server-Side Session TTL +``` +POST /login → sets cookie Max-Age=86400 (24h) +src/auth.rs:108-115: require_auth middleware executes: + "SELECT ... FROM sessions s JOIN accounts a ON s.account_id = a.id + WHERE s.id = ?1" + No "AND s.created_at > datetime('now', '-1 day')" filter +→ Sessions persist indefinitely in DB +``` +**Evidence:** Session query at src/auth.rs:113 has no time-based filter. Session ID from leaked cookie works forever. + +### X7: No CSRF Protection on Any POST Endpoint +``` +All POST handlers: No token validation, no origin/referer check, no SameSite=Strict +→ CSRF on: /logout, /instances/new, /instances/{id}/delete, /players/op/add, etc. +``` +**Evidence:** No CSRF middleware in src/routes/mod.rs. Cookie has SameSite=Lax (not Strict). All POST operations lack CSRF tokens. + +### X8: POST /logout Missing Auth Guard +``` +POST /logout registered on outer Router NOT behind require_auth +→ src/auth.rs:130-152: Reads Cookie header, extracts session ID, DELETEs from DB +→ No authentication required for the request +``` +**Evidence:** src/routes/mod.rs:20 registers logout on outer Router, before `.merge(protected)` at line 33. + +### X9: RCON Password Exposed to All Authenticated Users +``` +GET /instances/{id}/edit → edit_form handler +→ src/routes/instances.rs:313: rcon_password: row.get(15)? +→ Rendered in HTML template (EditInstanceTemplate) +→ Any authenticated user can see RCON password for any instance +``` +**Evidence:** SELECT query at line 301 includes `rcon_password`. No superuser or ownership check before rendering. + +### X10: Weak RCON Password Generation +``` +src/routes/instances.rs:523-529: generate_rcon_password() +→ rand::thread_rng() (not OsRng/crypto RNG) +→ 36^16 space (lowercase alphanumeric) +``` +**Evidence:** `rand::thread_rng()` at line 524 is not cryptographically secure. 62^16 would be stronger. + +### X11: Docker Env Variables Set Without Sanitization (Multiple) +ALL instance fields injected into Docker container env without sanitization: +- `VERSION={version}` (src/docker.rs:93) +- `MAX_PLAYERS={max_players}` (src/docker.rs:95) +- `DIFFICULTY={difficulty}` (src/docker.rs:96) +- `RCON_PASSWORD={rcon_password}` (src/docker.rs:109) +- `MOTD={motd}` (src/docker.rs:103) +- `LEVEL_TYPE={level_type}` (src/docker.rs:111) +- `LEVEL_SEED={seed}` (src/docker.rs:116) +- `GAMEMODE={gamemode}` (src/docker.rs:112) + +**Evidence:** src/docker.rs:87-116 — All `format!()` calls with direct `instance.*` values. No escaping. + +### X12: Backup Restore is No-Op (Data Loss Risk) +``` +POST /backups/{id}/restore → restore_backup handler +→ src/routes/backups.rs:88-96: Looks up backup in DB → redirects +→ NO actual file restoration performed +``` +**Evidence:** Handler at line 88 does SELECT then returns Redirect. No file copy/restore operations. + +### X13: Mod Validation DoS via Crafted JAR +``` +POST /instances/{id}/mods → upload crafted JAR with malicious constant pool +→ src/mods.rs:120: ZipArchive::new(data) +→ src/mods.rs:185: scan_single_class(data) — walks constant pool +→ No timeout, no recursion depth limit, no size limits on class files +→ Crafted constant pool (e.g., giant CONSTANT_Utf8 entries) could cause OOM +``` +**Evidence:** Loop at line 173 (`while i < cp_count`) has no iteration limit beyond cp_count. No bounds on CONSTANT_Utf8 length. + +### X14: Recursive ZIP Extraction Without Depth Limit +``` +POST /instances/{id}/mods → upload ZIP containing JAR (which is also a ZIP) +→ src/routes/mods.rs:131: process_zip_upload → extracts JAR bytes +→ src/routes/mods.rs:149: process_jar_upload → validate_mod +→ src/mods.rs:120: ZipArchive::new(data) — re-opens JAR as ZIP +→ No depth counter; possible infinite recursion +``` +**Evidence:** process_zip_upload calls process_jar_upload which calls validate_mod which opens the JAR as ZIP. No depth tracking. + +--- + +## 7. Trust Boundary Crossings + +| Boundary | Crossing Point | Direction | Data | Protection | +|----------|---------------|-----------|------|------------| +| HTTP → Auth | require_auth middleware (src/auth.rs:55) | Inbound | Session cookie → Account | Session lookup in DB | +| Auth → Instance Data | All protected handlers | Inbound | Authenticated request → CRUD | None (flat auth model) | +| WebUI → Filesystem | create_dir_all, fs::write, File::create | Outbound | User-controlled name/file content | Partial (mod filename) | +| WebUI → Docker | docker::start_container | Outbound | Instance settings | **NONE** | +| WebUI → RCON | RconClient::send_command_async | Outbound | Player username, commands | **NONE** | +| WebUI → Database | rusqlite execute/query | Outbound | Form data | Parameterized queries | +| WebUI → HTTP (Mojang) | reqwest::Client::get | Outbound | None (hardcoded URL) | Trusted URL | +| Upload → Validation | validate_mod (src/mods.rs:99) | Internal | Raw JAR bytes | Metadata + class scan | -- cgit v1.2.3