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/spec-gap-summary.md | 164 +++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 piolium/attack-surface/spec-gap-summary.md (limited to 'piolium/attack-surface/spec-gap-summary.md') diff --git a/piolium/attack-surface/spec-gap-summary.md b/piolium/attack-surface/spec-gap-summary.md new file mode 100644 index 0000000..5e52e81 --- /dev/null +++ b/piolium/attack-surface/spec-gap-summary.md @@ -0,0 +1,164 @@ +# P7 Spec, Framework Contract & Parser Gap Summary + +**Target:** salmon/minecraft-ui +**Date:** 2025-07-29 +**Phase:** P7 — Specification, Framework Contract & Parser Differential Analysis +**Mode:** deep + +--- + +## Overview + +This report documents gaps between authoritative specifications, framework contracts, and the codebase implementation for salmon/minecraft-ui. The analysis covers: + +- **RFC 9106** (Argon2) — password hashing parameter compliance +- **Java `.properties` format** (Oracle spec) — server.properties parsing +- **Source RCON Protocol** (Minecraft community spec — wiki.vg/RCON) — packet handling +- **RFC 6265** (HTTP State Management) — cookie parsing and session handling +- **Axum Framework Contracts** — middleware ordering, extension-based auth, multipart boundary parsing +- **Hidden Control Channels** — Docker environment injection, file path construction, host header trust +- **Deployment Assumptions** — reverse proxy, localhost binding, no TLS + +--- + +## Spec Gap Candidates + +### Gap 1: Java `.properties` — Missing Escape Sequence Processing + +- **Spec:** Oracle Java `.properties` file format +- **Requirement:** The format specifies escape sequences: `\n`, `\r`, `\t`, `\\`, `\uXXXX`, `\ ` (literal space), `\!`, `\#`, `\=`, `\:`, and multi-line continuation with trailing `\`. +- **Code Path:** `src/properties.rs:13-19` — `read_properties()` splits on `=` or `:` without escaping +- **Gap Type:** parsing | canonicalization +- **Impact:** Values containing `=`, `:`, or Java escape sequences in `server.properties` are misparsed. An attacker who can control `server.properties` values (e.g., via `level-seed` setting) can inject fake key-value pairs or corrupt parsing. The `level-seed` value in `server.properties` is written by `regenerate_world` via a line-replacement loop that creates `level-seed={seed}` — the seed value is not escaped for the `.properties` format. +- **Attack Vector:** Auth'd user sets a seed containing `\nlevel-type=FLAT\n` which, when written to `server.properties` via the naive line-replacement in `regenerate_world`, injects additional properties. On next server start, the injected properties alter server behavior. +- **Exploit Conditions:** Auth'd user; instance not active; access to `regenerate_world` handler. +- **Severity:** MEDIUM +- **Evidence:** `src/properties.rs:13-19` uses `split_once('=')` and `split_once(':')` with no escape processing. Java `.properties` spec requires backslash escaping for separators. + +### Gap 2: Argon2id (RFC 9106) — Non-Recommended Default Parameters + +- **RFC/Spec:** RFC 9106, Sections 3.1, 4, 7.4 +- **Requirement:** RFC 9106 §4 recommends as FIRST option: `Argon2id` with `t=1`, `p=4`, `m=2^21` (2 GiB). §7.4 states "Argon2id variant with t=1 and 2 GiB memory is the FIRST RECOMMENDED option". Salt SHOULD be 16 bytes and SHOULD be unique per password. +- **Code Path:** `src/auth.rs:28` — `Argon2::default().hash_password(password.as_bytes(), &salt)?` +- **Gap Type:** missing-check | parameter-choice +- **Impact:** `Argon2::default()` uses m_cost=19456 KiB (~19 MiB), t_cost=2 iterations, p_cost=1 lane. This is ~100x less memory than RFC 9106 recommends. While not a compliance vulnerability (any parameters are valid per the algorithm spec), the weak defaults reduce brute-force attack resistance relative to the RFC's recommended "default setting for all environments." +- **Attack Vector:** Offline password hash cracking benefits from the 19 MiB vs 2 GiB memory reduction (~100x cheaper per guess). +- **Exploit Conditions:** Attacker obtains password hash from database (SQLite file access or SQL injection). +- **Impact:** Accelerated offline brute-force by ~100x less memory per hash evaluation. +- **Severity:** MEDIUM +- **Evidence:** `src/auth.rs:28` uses `Argon2::default()` (m=19456 KiB, t=2, p=1). RFC 9106 §4 recommends m=2^21 KiB (2 GiB), t=1, p=4 for pass hashing. Source: `/home/salmon/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/argon2-0.5.3/src/params.rs:29` defines DEFAULT_M_COST = 19 * 1024. + +### Gap 3: RCON Protocol — Insufficient Response Read After EXECCOMMAND + +- **Spec:** Source RCON Protocol (Minecraft community convention — wiki.vg/RCON) +- **Requirement:** After sending `SERVERDATA_EXECCOMMAND` (type 2), the server responds with a `SERVERDATA_RESPONSE_VALUE` (type 0) packet containing the command output, followed by a second `SERVERDATA_RESPONSE_VALUE` (type 0) as an end marker. Multi-packet responses are possible. +- **Code Path:** `src/rcon.rs:72-76` — reads one response, then ignores errors on second +- **Gap Type:** state-machine | error-handling +- **Impact:** Reads only one guaranteed response. Fragmented multi-packet responses are truncated. The comment acknowledges this: "some servers send it, some don't; ignore errors." +- **Attack Vector:** Long RCON command output (> 1400 bytes) gets truncated. +- **Exploit Conditions:** Any RCON command with output exceeding ~1400 bytes. +- **Impact:** Truncated command response (information disclosure). +- **Severity:** LOW — protocol compliance only, no direct security bypass. +- **Evidence:** `src/rcon.rs:72` — `let response = self.read_packet(&mut stream)?;` followed by `let _ = self.read_packet(&mut stream);` with error ignored. + +### Gap 4: Java `.properties` — Key Continuation and Whitespace Separator Not Supported + +- **Spec:** Oracle Java `.properties` format +- **Code Path:** `src/properties.rs:13-19` +- **Gap Type:** parsing +- **Impact:** Lines with whitespace-only separators (e.g., `key value`) are silently ignored. +- **Severity:** LOW — informational; conventional Minecraft server.properties uses `=`. + +### Gap 5: Auth Middleware — Manual Cookie Parsing (RFC 6265) + +- **RFC/Spec:** RFC 6265 (HTTP State Management), Sections 4.2.2, 5 +- **Code Path:** `src/auth.rs:105-118` — manual `split(';')` then `split_once('=')` +- **Gap Type:** parsing | canonicalization +- **Impact:** The parser does NOT strip OWS around cookie names per RFC 6265 §4.2.2. Does NOT handle quoted cookie values. +- **Severity:** LOW — well-formed browser cookies not affected; no auth bypass possible. + +### Gap 6: Framework Contract — Logout Without Auth Middleware + +- **Contract:** Axum middleware; all state-changing ops require auth. +- **Code Path:** `src/routes/mod.rs:29` — `/logout` on outer router, not in `protected` group. +- **Gap Type:** middleware-ordering +- **Impact:** Anonymous requests can delete sessions (requires valid session ID — UUIDv4 impractical). +- **Severity:** LOW — theoretical; UUIDv4 unpredictability prevents exploitation. + +### Gap 7: Framework Contract — No Server-Side Session TTL Enforcement + +- **Contract:** Session cookie Max-Age=86400 but no server-side expiration check. +- **Code Path:** `src/auth.rs:93` — `Max-Age=86400`; `src/auth.rs:108-115` — no `created_at` check. +- **Gap Type:** missing-check +- **Impact:** Session rows remain valid indefinitely in DB. Stolen session ID (via XSS, log access, DB leak) works forever. +- **Attack Vector:** Session ID leak → permanent session hijacking. No server-side TTL means an attacker who obtains a session ID can reuse it indefinitely, long after the Max-Age expires. +- **Exploit Conditions:** Attacker must first obtain a valid session ID. +- **Impact:** Session hijacking window extended indefinitely. +- **Severity:** MEDIUM +- **Evidence:** `src/auth.rs:93` sets `Max-Age=86400` but `src/auth.rs:108-115` does `WHERE s.id = ?1` with no date filter. + +### Gap 8: Hidden Control Channel — Instance MOTD Injected Into Docker Env Without Sanitization + +- **Contract:** Docker env vars derived from instance settings; Docker env parsing splits on `\n`. +- **Code Path:** `src/docker.rs:103` — `format!("MOTD={}", instance.motd)` +- **Gap Type:** hidden-control-channel | missing-check +- **Impact:** MOTD containing newline characters (`%0a`) injects arbitrary additional env vars into the Docker container. For example, MOTD=`hello\nEULA=FALSE\nONLINE_MODE=true` would override security-critical env vars. All Docker env vars (VERSION, TYPE, MEMORY, MAX_PLAYERS, DIFFICULTY, RCON_PASSWORD, GAMEMODE, LEVEL_TYPE, LEVEL_SEED, PVP, MOTD) are set without sanitization. +- **Attack Vector:** Auth'd user edits instance MOTD → saves → triggers container restart → injected env vars alter Docker container behavior. +- **Exploit Conditions:** Auth'd user; container restarted after instance edit. +- **Impact:** Full Docker container environment variable injection — could override EULA, ONLINE_MODE, RCON settings, or inject arbitrary env vars. +- **Severity:** HIGH +- **Evidence:** `src/docker.rs:103` — `format!("MOTD={}", instance.motd)`. No escaping. MOTD sourced from `src/routes/instances.rs:206` via `Form` without sanitization. + +### Gap 9: Hidden Control Channel — Instance Name Used in File Paths Without Sanitization + +- **Contract:** Instance name → filesystem path. +- **Code Path:** `src/routes/instances.rs:173` — `server_directory.join("instances").join(&form.name)` +- **Gap Type:** hidden-control-channel | missing-check +- **Impact:** `std::path::Path::join` DOES traverse on `..` prefix. Instance name `../../evil` results in path outside intended tree. All subsequent operations (mod uploads, world import, backups) land at attacker-chosen locations. Files created: instance data directory, mod JAR files, world files. +- **Attack Vector:** Auth'd user creates instance with `../../evil_name` → data dir created at `server_directory/instances/../../evil_name/data` = `server_directory/evil_name/data`. Then upload mod or import world to write files at traversed location. +- **Exploit Conditions:** Auth'd user. +- **Impact:** Arbitrary file write relative to server_directory (WebUI process user permissions). +- **Severity:** HIGH +- **Evidence:** `src/routes/instances.rs:173` — `let instance_dir = state.config.server_directory.join("instances").join(&form.name).join("data");` No validation of `form.name` for `..` or path separators. Also stored in DB at `src/routes/instances.rs:119` without sanitization. + +### Gap 10: Hidden Control Channel — Content-Type Boundary Parsing Trusts Attacker Input + +- **Contract:** Multer multipart parser; boundary from Content-Type header. +- **Code Path:** `src/routes/mods.rs:140-149` — boundary parsed without validation +- **Gap Type:** hidden-control-channel | missing-check +- **Impact:** Attacker-controlled boundary value fed to `multer::Multipart::with_constraints`. While multer validates boundaries internally, a crafted boundary could cause field misidentification, size limit bypass, or field-skipping behavior. +- **Attack Vector:** Send crafted Content-Type header with boundary overlapping body content: malformed multipart parsing. +- **Exploit Conditions:** Auth'd user uploading mods. +- **Impact:** Potential DoS or file size limit bypass. +- **Severity:** MEDIUM +- **Evidence:** `src/routes/mods.rs:141-148` — boundary parsed from user-supplied `Content-Type`. + +### Gap 11: Hidden Control Channel — All Instance Settings In Docker Env Without Sanitization + +- **Contract:** All instance settings → Docker env. +- **Code Path:** `src/docker.rs:87-113` +- **Gap Type:** hidden-control-channel | missing-check +- **Impact:** Beyond MOTD (Gap 8), LEVEL_SEED, LEVEL_TYPE, GAMEMODE, DIFFICULTY, VERSION, RCON_PASSWORD are all injected without sanitization. +- **Severity:** HIGH — combined with Gap 8. + +### Gap 12: Framework Contract — RCON Password Exposed to All Authenticated Users + +- **Contract:** Supposed role-based access control for sensitive instance data. +- **Code Path:** `src/routes/instances.rs:313` — `rcon_password: row.get(15)?` in `edit_form` +- **Gap Type:** missing-check +- **Impact:** Any authenticated user can view any instance's RCON password by visiting `/instances/{id}/edit`. No superuser or instance-ownership check. RCON password grants full Minecraft server control. +- **Attack Vector:** Auth'd user navigates to `/instances/1/edit` → sees RCON password → uses RCON client to issue arbitrary commands. +- **Exploit Conditions:** Auth'd user. +- **Impact:** Full Minecraft server compromise via RCON — whitelist bypass, data destruction, command execution. +- **Severity:** HIGH +- **Evidence:** `src/routes/instances.rs:313` — `rcon_password` selected and returned. No superuser check. Compare with `src/routes/accounts.rs:35` where checks exist. + +--- + +## Cross-Reference with Phase 3 Domain Attack Research + +- **Gap 8/11** (Docker env injection) — NOT covered in Phase 3. New finding. +- **Gap 9** (Instance name path traversal) — NOT specifically covered. Phase 3 identified mod filename traversal (T2) but not instance name as traversal vector. +- **Gap 12** (RCON password exposure) — Covered in Phase 3 as T5. +- **Gap 7** (No session TTL) — Covered in Phase 3 domain research. +- **Gap 2** (Argon2 defaults) — NOT covered in Phase 3. -- cgit v1.2.3