summaryrefslogtreecommitdiffstats
path: root/piolium/attack-surface/patch-bypass-summary.md
blob: f7e7709b43581c526c85810887fc2d445d87c000 (plain) (blame)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
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<PlayerAction>,
        ) -> Result<Redirect, ...> {
            ...
            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.