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/patch-bypass-summary.md | 305 +++++++++++++++++++++++++ 1 file changed, 305 insertions(+) create mode 100644 piolium/attack-surface/patch-bypass-summary.md (limited to 'piolium/attack-surface/patch-bypass-summary.md') diff --git a/piolium/attack-surface/patch-bypass-summary.md b/piolium/attack-surface/patch-bypass-summary.md new file mode 100644 index 0000000..f7e7709 --- /dev/null +++ b/piolium/attack-surface/patch-bypass-summary.md @@ -0,0 +1,305 @@ +# Patch History & Bypass Summary + +**Audit ID:** `2026-07-28T23:03:56.306Z` +**Repository:** `/home/salmon/Documents/minecraft-ui` +**Branch:** `master` (HEAD `74d2ada`) +**Scan Window:** 500 commits / 60 days (all 6 commits captured) + +--- + +## Repository Overview + +Minecraft server management webUI built with **Rust/Axum** + **SQLite** + **Docker** (itzg/minecraft-server). +Manages multiple server instances, mods, backups, world files, whitelist/ops/bans. + +**Total commits in window:** 6 +**Security-dedicated commits:** **0** — no commits explicitly fix a disclosed vulnerability. +All commits are feature additions, cleanup, or initial scaffolding. + +--- + +## Commit Inventory & Security Relevance + +| # | Commit | Date | Type | Security Relevance | +|---|--------|------|------|--------------------| +| 1 | `0c2e661` | 2026-07-12 | Architecture | Setup only. No code. | +| 2 | `ae7ef10` | 2026-07-12 | First Stable | Introduced auth (Argon2 hashing, sessions), RCON, Docker, instance CRUD, backups. | +| 3 | `9ad52d0` | 2026-07-12 | Second Stable | Added world import/export/regenerate; `properties.rs` parser. **Introduced Zip upload surface.** | +| 4 | `ce30a87` | 2026-07-12 | Third Stable | Removed dead code; added RCON readiness poll; **added `file_name()` basename extraction** for path safety. | +| 5 | `97920e4` | 2026-07-12 | Config | Auto-create `config.toml` from default if missing. | +| 6 | `74d2ada` | 2026-07-29 | Mod Feature | Mod validation engine + upload; whitelist toggle; dynamic Java tags; **session cookie debug logging**; removed `hx-boost` (was breaking multipart). | + +**No advisory metadata exists for any commit.** Every change is a functional addition, not a security fix. + +--- + +## Security-Relevant Diffs (Per Commit) + +### Commit `ae7ef10` — Initial Authentication System + +**What was introduced:** +- Argon2 password hashing + verification +- Session-based auth with UUID v4 session IDs +- HttpOnly + SameSite=Lax cookie (max-age 86400s) +- Superuser guard on account management routes +- `require_auth` middleware applied to all protected routes + +**Potential bypass vectors evaluated:** + +| Vector | Assessment | +|--------|------------| +| Session prediction | UUID v4 is cryptographically random; no predictability. **Sound.** | +| Cookie theft via XSS | HttpOnly prevents JS access. But no `Secure` flag → cookie sent in cleartext over HTTP. **Bypassable** — network-level theft possible on LAN/public wifi. | +| Session fixation | New UUID generated on every login. No way to pre-set session ID. **Sound.** | +| SQL injection in session lookup | Uses parameterized queries. **Sound.** | +| Timing attack on password | Argon2 constant-time comparison. **Sound.** | +| Brute force | No rate limiting on `/login` endpoint. **Bypassable.** | +| Missing Secure flag | Cookie lacks `Secure`; SameSite=Lax helps but not on first request. **Bypassable.** | + +**Verdict: Relocated** — the auth primitive is sound in isolation, but the missing `Secure` flag and absent rate limiting shift the attack surface to network interception and brute force. + +--- + +### Commit `9ad52d0` — World Import (ZIP extraction) + +**What was introduced:** +- `import_world()` — multipart ZIP upload → extracted to `data/world/` +- `export_world()` — directory zipped and streamed to browser +- `regenerate_world()` — deletes world, rewrites `server.properties` + +**Security controls in import_world:** +- `file.enclosed_name()` from the `zip` crate prevents Zip Slip (path traversal) + +**Potential bypass vectors:** + +| Vector | Assessment | +|--------|------------| +| Zip Slip via symlink | `enclosed_name()` rejects entries whose path escapes the target dir. **Sound.** | +| Arbitrary file overwrite | Only files within `world_dir` can be written. **Sound.** | +| Zip bomb | No size limit on uploaded field. `bytes().await` loads entire upload into memory. **Bypassable** — OOM denial of service. | +| World seed manipulation | `regenerate_world()` writes user-supplied seed to `server.properties` without validation of seed content (e.g., newlines). Could inject extra properties. **Bypassable** — property injection. | + +**Specific bypass — property injection via seed:** +```rust +// In regenerate_world: +let seed = form.seed.unwrap_or_default(); +// ... +new_lines.push(format!("level-seed={seed}")); +``` +A seed containing newlines (`level-seed=123\nmax-players=999`) injects additional properties. + +**Verdict: Bypassable** — property injection via seed field; no upload size limits. + +--- + +### Commit `ce30a87` — Path Safety via `file_name()` + +**What was introduced:** +- Backup creation now uses `path.file_name()` to get basename before adding to ZIP +- Removed dead `container_status()` code +- RCON readiness polling (waits up to 60s for server to respond) + +**Security-relevant change:** +The switch to `file_name()` prevents path traversal in backup content (files inside `data/` are only stored by basename): +```rust +// Before (ae7ef10): +if let Some(name) = path.file_name() { ... } + +// After (ce30a87 — same pattern, already present) +``` + +This was already safe in the initial commit — no change. The commit just removed unused template fields and the `ContainerStatus` enum. + +**Verdict: Not a security fix.** No bypass applicable. + +--- + +### Commit `74d2ada` — Mod Validation Engine + +**What was introduced:** +- `src/mods.rs` — two-layer mod validation (metadata + class-file constant-pool scan) +- `src/routes/mods.rs` — CRUD routes for mod upload, rescan, delete +- Mods table in DB +- Whitelist enable/disable checkbox on edit page +- Dynamic Java image tag based on MC version +- Session cookie debug logging + +**Mod validation policy:** +- Only mods that **explicitly declare** themselves client-only in metadata are rejected +- Class-file scanning is informational only — never causes rejection +- Many Forge mods that work on servers reference `net/minecraft/client/...` (rendering/GUI) + +**Potential bypass vectors:** + +| Vector | Assessment | +|--------|------------| +| Malicious mod upload | No signature verification, no integrity check. Any server-capable mod with malicious code is accepted. **Bypassable by design** — the validation only catches honest client-only declarations. | +| Metadata spoofing | A server-side malicious mod declaring `side=CLIENT` in `mods.toml` would be **rejected** even though it works on servers. But a server-side malicious mod with `side=BOTH` or no metadata sails through. **Sound for the declared policy.** | +| Class scan evasion | Mod obfuscation that doesn't reference `net/minecraft/server/...` could bypass detection, but the scan is informational-only, so no security boundary to bypass. | +| Zip content extraction | `process_zip_upload()` extracts nested JARs from ZIPs. Uses `file_name()` basename. **Sound** — no directory traversal. | +| Path traversal in filename | `safe_name = Path::new(filename).file_name()` strips directory components. **Sound.** | +| Arbitrary file write via mod | Once accepted, mod JAR is written to `mods_dir`. No validation that file is actually a valid JAR. But Docker container mounts that dir, so any file ends up in `/data/mods/`. **Sound** — jar extension enforced by route, no symlink. | + +**Whitelist toggle fix:** +The `ENABLE_WHITELIST` env var was hardcoded `true` before this commit. Now it's configurable per-instance. This is a **feature addition** masquerading as a hardening — whitelist being off is a valid configuration choice, not a vulnerability. + +**Session cookie debug logging:** +Added tracing lines that log `raw_cookies` to stderr. If tracing output is captured in production logs, session IDs could leak into log files. **Potential information disclosure.** + +**Verdict: Sound for declared policy**, but the policy itself is weak — any malicious server-side mod is accepted. No bypass of the validation logic exists. + +--- + +## Cross-Cutting Vulnerability Analysis + +### 1. RCON Command Injection [UNFIXED] + +**Location:** `src/routes/players.rs` — `player_action!` macro + +**Mechanism:** +```rust +macro_rules! player_action { + ($name:ident, $command:expr) => { + async fn $name( + ... + Form(form): Form, + ) -> Result { + ... + let cmd = format!($command, form.username); + // e.g., "whitelist add {}", "op {}", "ban {}", etc. + client.send_command_async(cmd.clone()).await + } + }; +} +``` + +**Impact:** Username is directly interpolated into an RCON command string. An attacker can inject additional RCON commands via a crafted username: +``` +POST /players/whitelist/add +username=player1;say%20pwned +``` +Would execute: `whitelist add player1;say pwned` — both commands run. + +**Bypass:** No validation or sanitization of `form.username`. No character restrictions. RCON protocol sends commands as null-terminated strings; the `;` is interpreted by the Minecraft server's command parser, which chains commands. + +**Cluster:** This is a command injection class bug. No commit has attempted to fix it. + +--- + +### 2. Missing CSRF Protection [UNFIXED] + +**Location:** All form handlers + +**Mechanism:** No CSRF tokens on any POST form. `SameSite=Lax` on cookies provides partial protection (not for GET-based state changes, but all state changes use POST). However, `SameSite=Lax` sends cookies on top-level navigations from external sites, and `POST` forms from cross-origin without `SameSite=Strict` are still sent with cookies in some browser configurations. + +**Impact:** Cross-site request forgery on all admin actions (create instance, delete, activate, upload mods, manage players, change passwords, etc.). + +**Bypass:** No CSRF token validation exists anywhere. Attacker site can POST to any endpoint and the victim's browser will include the session cookie for top-level navigations. + +**Cluster:** No commit addresses CSRF. + +--- + +### 3. No Rate Limiting [UNFIXED] + +**Location:** `/login` endpoint, all POST handlers + +**Mechanism:** No rate limiting on any endpoint. Brute-force password attacks against the login form are unbounded. + +**Impact:** Unlimited login attempts for credential brute force. + +**Bypass:** Directly exploitable — no commit adds any throttling. + +--- + +### 4. Information Disclosure — World Seed via RCON [DESIGN ISSUE] + +**Location:** `src/routes/instances.rs` — `edit_form()` + +**Mechanism:** The world seed is fetched via RCON `seed` command and displayed on the edit page. Any authenticated user can see the seed. + +**Impact:** World seed is considered sensitive by some server operators (enables griefers to locate structures). The seed is also stored in DB and `server.properties`. + +**Not a patch bypass** — no commit has attempted to restrict seed access. + +--- + +### 5. Insecure Default Credentials [SOFT] + +**Location:** `config.toml` + +**Config:** +```toml +initial_superuser = "admin" +initial_superuser_password = "admin" +``` + +**Mechanism:** Default credentials are hardcoded in the example config. On first run, these create the initial superuser. `password_change_required=1` is set, forcing a password change on first login. + +**Bypass:** If an attacker gains access before the admin changes the password (e.g., upon deployment), they can log in with `admin:admin`. The forced password change can be bypassed if the attacker changes it to their own password. + +**Verdict: Relocated** — the forced-change mitigates but doesn't eliminate the window of exposure. + +--- + +### 6. Missing Authorization Boundaries [DESIGN ISSUE] + +**Location:** All instance routes + +**Mechanism:** All authenticated users can manage all instances. No per-instance access control. The `is_superuser` check only gates account management, not instance operations. + +**Impact:** Any user can create, delete, modify, activate, upload mods to, or back up any instance. Single tenancy by design, but problematic if user accounts are meant to isolate administrators. + +**Not a patch bypass** — no commit has attempted per-instance authorization. + +--- + +## Bypass Verdict Summary + +| # | Issue | Affected Since | Fix Status | Bypass Verdict | +|---|-------|---------------|------------|----------------| +| 1 | RCON command injection via username | `ae7ef10` | **UNFIXED** | Bypassable — no sanitization | +| 2 | Missing CSRF tokens | `ae7ef10` | **UNFIXED** | Bypassable — no protection | +| 3 | No login rate limiting | `ae7ef10` | **UNFIXED** | Bypassable — unbounded attempts | +| 4 | Cookie missing Secure flag | `ae7ef10` | **UNFIXED** | Bypassable — cleartext on HTTP | +| 5 | Property injection via world seed | `9ad52d0` | **UNFIXED** | Bypassable — no seed validation | +| 6 | Mod validation (policy weakness) | `74d2ada` | **BY DESIGN** | Sound — policy allows server mods | +| 7 | Zip bomb / no upload size limit | `9ad52d0` | **UNFIXED** | Bypassable — OOM DoS | +| 8 | Default admin credentials | `97920e4` | Relocated | Bypassable during initial window | +| 9 | Session log leakage | `74d2ada` | **UNFIXED** | Bypassable — if logs accessible | +| 10 | Static file serving without auth | `ae7ef10` | **BY DESIGN** | Sound — static dir is controlled | + +--- + +## Cluster IDs + +| Cluster ID | Commits | Theme | +|-----------|---------|-------| +| `C001` | `ae7ef10`, `9ad52d0`, `ce30a87`, `74d2ada` | Auth & Session Management | +| `C002` | `9ad52d0`, `ce30a87`, `74d2ada` | File Upload & Path Safety | +| `C003` | `9ad52d0`, `74d2ada` | Docker Container Configuration | +| `C004` | `74d2ada` | Mod Validation Engine | + +--- + +## Undisclosed Fix Candidates + +No commits in this repository appear to be silent (undisclosed) security fixes. All commits are feature additions or cleanup. The repository has no advisory history. + +However, the following patterns suggest latent vulnerabilities that **should** have been fixed but remain open: + +1. **RCON injection in `player_action!` macro** — the most severe unfixed issue (CWE-77) +2. **Property injection in `regenerate_world`** — seed field allows injecting arbitrary `server.properties` lines +3. **World import ZIP bomb** — no size limit on multipart field in `import_world()` + +--- + +## Conclusion + +- **0 security patches** found in the commit history. +- **3 critical unfixed vulnerabilities** (RCON injection, CSRF, rate limiting). +- **3 moderate unfixed issues** (cookie Secure flag, property injection, ZIP bomb). +- **2 design-level weaknesses** (mod validation policy, static file serving). +- No existing fix to bypass — the security controls that exist were built in from the initial commit and their bypassability is assessed above. + +The most actionable finding is **RCON command injection** (CWE-77) in the player management routes, which allows any authenticated user to execute arbitrary Minecraft server commands through crafted usernames. -- cgit v1.2.3