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/variant-summary.md | 130 ++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 piolium/attack-surface/variant-summary.md (limited to 'piolium/attack-surface/variant-summary.md') diff --git a/piolium/attack-surface/variant-summary.md b/piolium/attack-surface/variant-summary.md new file mode 100644 index 0000000..0503ac1 --- /dev/null +++ b/piolium/attack-surface/variant-summary.md @@ -0,0 +1,130 @@ +# Variant Analysis Summary — Phase 12 + +**Generated:** 2025-07-29 +**Phase:** P12 (Variant Search) +**Status:** COMPLETE + +--- + +## Methodology + +For each of the 17 surviving Phase 10 findings, the entire codebase was searched for: +1. Same root cause pattern (unsanitized input → dangerous sink) +2. Same flow shape (source type → transformation → sink type) +3. Same attacker control level (authenticated/unauthenticated, direct/indirect) +4. Missing protections (no input validation, no escaping, no authorization check) + +--- + +## Results + +| Finding | Root Cause | Variants Found | New Drafts | +|---------|-----------|----------------|------------| +| p10-001 — Instance Name Path Traversal | Path::join() with unsanitized user string | **4 new** (world import, regenerate, player mgmt, backup create) | p12-001 through p12-004 | +| p10-002 — RCON Command Injection | format!() with unsanitized username in RCON command | 0 | — | +| p10-003 — Level-Seed Properties Injection | format!() with unsanitized seed in Java .properties file | 0 | — | +| p10-004 — RCON Password Exposed | Sensitive field in template rendering | 0 | — | +| p10-005 — Missing CSRF Protection | Architectural (no CSRF tokens anywhere) | 0 (all 31 endpoints) | — | +| p10-006 — No Server-Side Session TTL | Single query point | 0 | — | +| p10-007 — No Rate Limiting Login | Architectural (no middleware) | 0 | — | +| p10-008 — Logout No Auth Middleware | Single route registration | 0 | — | +| p10-009 — Cookie No Secure Flag | Single cookie setting | 0 | — | +| p10-010 — Flat Authorization Model | Architectural (no per-resource authz) | 0 | — | +| p10-011 — Background Task Stale is_active | Background task closes over stale DB filter | 0 (all 5 updates in same task) | — | +| p10-012 — Delete-Create Name Reuse Race | TOCTOU race window | 0 | — | +| p10-013 — No Idempotency | Architectural (no idempotency keys) | 0 | — | +| p10-014 — Mod Write Before DB Orphan | Ordering bug (file before DB insert) | 0 | — | +| p10-015 — Backup Orphan Race | Ordering bug (ZIP before DB insert) | 0 | — | +| p10-016 — Mod Validation Denial of Service | No timeout on CPU-bound parsing | 0 | — | +| p10-017 — No Encryption at Rest | Architectural (no SQLite encryption) | 0 | — | + +--- + +## Path Traversal Variant Details + +### Variant 1: World Import Path Traversal (p12-001) + +| Property | Value | +|----------|-------| +| Source | `instances.name` from DB (originally `form.name`) | +| Sink | `std::fs::File::create()` at `instances.rs:704` | +| Flow | POST `/instances/{id}/world/import` → load name from DB → `.join(&instance_name)` → `world_dir.join(zip_entry)` → `File::create()` | +| Constraint | `/data/world/` suffix appended after traversed name | +| Severity | HIGH | +| New? | Yes — NOT in AP-001 registry previously | + +### Variant 2: Regenerate World Path Traversal (p12-002) + +| Property | Value | +|----------|-------| +| Source | `instances.name` from DB (originally `form.name`) | +| Sink | `std::fs::remove_dir_all()` at `instances.rs:779` | +| Flow | POST `/instances/{id}/world/regenerate` → load name from DB → `.join(&instance_name)` → `remove_dir_all(world_dir)` | +| Constraint | `/data/world/` suffix appended after traversed name | +| Severity | HIGH | +| New? | Yes — NOT in AP-001 registry previously | + +### Variant 3: Player Management Path Traversal (p12-003) + +| Property | Value | +|----------|-------| +| Source | `instances.name` from active instance query | +| Sink | `std::fs::read_to_string()` via `read_json_list()` at `players.rs:59-76` | +| Flow | GET `/players` → `get_active_instance()` → `.join(&inst_name)` → `read_json_list(.../whitelist.json)` | +| Constraint | Only files named `whitelist.json`, `ops.json`, `banned-players.json` are read | +| Severity | MEDIUM (read-only, constrained to 3 filenames) | +| New? | Yes — NOT in AP-001 registry previously | + +### Variant 4: Backup Create Path Traversal (p12-004) + +| Property | Value | +|----------|-------| +| Source | `instances.name` from DB (originally `form.name`) | +| Sink | `std::fs::File::create()` at `backups.rs:114` | +| Flow | POST `/instances/{id}/backups/create` → load name from DB → `filename = {name}_{timestamp}.zip` → `.join(&filename)` → `File::create()` | +| Constraint | Timestamp suffix (`_YYYYMMDD_HHMMSS.zip`) attached to traversed name | +| Severity | MEDIUM (cannot control exact filename) | +| New? | Partially — backup filename path was listed in registry but created via Write (not Read) | + +--- + +## No Variants Found — Rationale + +### AP-002 (RCON Injection) +The `player_action!` macro generates 6 endpoints with unsanitized username → RCON command. All other RCON calls use hardcoded commands ("seed", "list"). No unsanitized input reaches any other `send_command_async()` call. + +### AP-003 (Properties Injection) +Only `regenerate_world` writes to `server.properties` with user data. The seed is also passed to Docker as `LEVEL_SEED` env var (docker.rs:141), but Docker API injection was ruled FALSE POSITIVE in Phase 10. No other Java .properties files are written with user data. + +### AP-004/005 (CSRF, Session TTL) +Architectural/design-level vulnerabilities. CSRF affects all 31 POST endpoints universally. Session TTL is a single missing WHERE clause in one query. No code-level variants. + +### Concurrency (p10-011 through p10-015) +Each concurrency finding is a unique race condition or ordering bug in a specific code flow. No identical patterns exist elsewhere in the codebase. The `WHERE is_active = 1` filter appears in the dashboard (read-only) and player management (read-only via `get_active_instance`), but these are synchronous reads that always return current state — unlike the background task which closes over the `is_active` state captured at task creation time. + +### DoS (p10-016) +The constant pool parser in `scan_single_class()` is the only CPU-bound parsing without a timeout. No other validation function lacks a timeout. `read_metadata()` opens a ZipArchive but its parsing is bounded by the ZIP entry count and file size. + +### Encryption (p10-017) +Single architectural gap — no encryption layer on SQLite. No variants. + +--- + +## Registry Updates + +The following new instances were added to AP-001 in the attack pattern registry: +- `src/routes/instances.rs:672 — world import via instance_name (FILE WRITE)` +- `src/routes/instances.rs:776 — regenerate world via instance_name (DIRECTORY DELETE)` +- `src/routes/players.rs:53 — player management via inst_name (FILE READ)` +- `src/routes/backups.rs:112 — backup create via filename (FILE WRITE)` + +--- + +## Conclusion + +**Total new variants found:** 4 +**All from:** AP-001 (Path Traversal via Unsanitized Instance Name) +**Total variant drafts written:** 4 (p12-001 through p12-004) +**Findings with zero variants:** 13 (p10-002 through p10-017, except p10-001) + +The path traversal pattern (AP-001) is the most pervasive vulnerability class in the codebase, with now 12 confirmed instances across 4 route modules (instances.rs, mods.rs, backups.rs, players.rs). -- cgit v1.2.3