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. --- .../attack-surface/state-concurrency-summary.md | 46 ++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 piolium/attack-surface/state-concurrency-summary.md (limited to 'piolium/attack-surface/state-concurrency-summary.md') diff --git a/piolium/attack-surface/state-concurrency-summary.md b/piolium/attack-surface/state-concurrency-summary.md new file mode 100644 index 0000000..48677b3 --- /dev/null +++ b/piolium/attack-surface/state-concurrency-summary.md @@ -0,0 +1,46 @@ +# State & Concurrency Audit + +## State-Holding Entities Catalogued + +| Entity | Table | State Columns | Allowable Values | Concurrency Risk | +|--------|-------|---------------|-----------------|------------------| +| Account | `accounts` | `is_superuser` | 0/1 | Low (rarely mutated) | +| Account | `accounts` | `password_change_required` | 0/1 | Low | +| Session | `sessions` | `created_at` | datetime | **High** — no TTL enforcement | +| Instance | `instances` | `is_active` | 0/1 | **CRITICAL** — background task stale writes | +| Instance | `instances` | `container_state` | stopped/pulling/booting/running/error | **CRITICAL** — not bound to instance ID in background writes | +| Instance | `instances` | `has_modpack` | 0/1 | Medium — RMW across mutex acquisitions | +| Instance | `instances` | `name` | TEXT (UNIQUE) | **HIGH** — name-reuse race between delete and create | +| Mod | `mods` | `accepted` | 0/1 | Low | +| Backup | `backups` | `filename`, `created_at` | TEXT, datetime | Medium — orphan ZIP on concurrent delete | +| All | — | — | — | **No idempotency infrastructure** | + +**Total state-holding entities catalogued:** 10 (across 5 tables) + +## Concurrency Primitives Observed + +| Primitive | Location | Scope | Limitation | +|-----------|----------|-------|------------| +| `Arc>` | `src/db.rs:7` | SQLite connection | Serialises per-statement, not per-transaction | +| `MutexGuard::drop()` implicit | Every `let conn = state.db.conn()` / `drop(conn)` pattern | Statement boundaries | Mutex released between read and write in multiple handlers | +| Tokio async tasks (background spawn) | `src/routes/instances.rs:457` | container lifecycle | **No instance ID binding** — uses `WHERE is_active = 1` | +| SQLite WAL mode | `src/db.rs:19` (PRAGMA) | Write-ahead logging | Allows concurrent reads during writes but does not prevent any of the races documented | + +**No distributed locks, no advisory locks, no `SELECT ... FOR UPDATE`, no explicit BEGIN/COMMIT boundaries.** + +## Idempotency Infrastructure + +**Absent.** No `idempotency_key` table, no deduplication tokens, no request ID storage, no unique constraints leveraged for dedup (except `accounts.username` and `instances.name`). Every POST handler is vulnerable to replay/double-submit. + +## Drafts Filed + +| Draft | Severity | Class | Summary | +|-------|----------|-------|---------| +| p6-001 | CRITICAL | toctou | Background task in `activate_instance` uses `WHERE is_active = 1` (no instance ID bound). After deactivation/reactivation, stale task writes `container_state` on the wrong instance. | +| p6-002 | HIGH | toctou | `delete_instance` releases DB mutex before `remove_dir_all`. Concurrent `create_instance` with same name has its data directory wiped. Cross-resource TOCTOU with data loss. | +| p6-003 | MEDIUM | state-machine-violation | `edit_instance` stops container on active instances without restarting. `is_active=1` and stale `container_state='running'` persist. Instance appears running but isn't. | +| p6-004 | HIGH | idempotency | No idempotency infrastructure on any POST handler. Mod upload, backup create, login, account create all vulnerable to replay/double-submit. | +| p6-005 | MEDIUM | rmw-no-txn | `process_jar_upload` writes file to disk before DB INSERT. Crash between file write and DB insert leaves orphaned mod file. Also performs `has_modpack` update across separate mutex acquisition. | +| p6-006 | MEDIUM | toctou | `create_backup` reads instance name, releases mutex, creates ZIP file (long I/O window), then re-acquires mutex. Concurrent instance deletion causes FK violation — orphaned ZIP file with no DB record. | + +**Total draft findings filed: 6** \ No newline at end of file -- cgit v1.2.3