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. --- src/routes/instances.rs | 103 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 80 insertions(+), 23 deletions(-) (limited to 'src/routes/instances.rs') diff --git a/src/routes/instances.rs b/src/routes/instances.rs index f1c3e37..d3b8294 100644 --- a/src/routes/instances.rs +++ b/src/routes/instances.rs @@ -11,6 +11,7 @@ use serde::Deserialize; use crate::auth::CurrentUser; use crate::models::Instance; use crate::routes::AppState; +use crate::sanitize; #[derive(Template)] #[template(path = "instances/list.html")] @@ -198,11 +199,32 @@ async fn create_instance( .ok(); drop(conn); + // Validate and sanitize instance name to prevent path traversal + let safe_name = match sanitize::sanitize_instance_name(&form.name) { + Some(n) => n, + None => { + // Invalid name, redirect back + return Ok(Redirect::to("/instances/new")); + } + }; + + // Double-check: the DB insert used the original form.name, but we want the safe version. + // Update the DB record to use the sanitised name. + { + let conn = state.db.conn(); + if let Err(e) = conn.execute( + "UPDATE instances SET name = ?1 WHERE name = ?2", + params![safe_name, form.name], + ) { + tracing::error!("Failed to update instance name to sanitized version: {e}"); + } + } + let instance_dir = state .config .server_directory .join("instances") - .join(&form.name) + .join(&safe_name) .join("data"); let _ = std::fs::create_dir_all(&instance_dir); @@ -389,8 +411,13 @@ async fn delete_instance( drop(conn); if let Some(name) = name { - let instance_dir = state.config.server_directory.join("instances").join(&name); - let _ = std::fs::remove_dir_all(&instance_dir); + // Sanitize name to prevent path traversal during cleanup + if let Some(safe_name) = sanitize::sanitize_instance_name(&name) { + let instance_dir = state.config.server_directory.join("instances").join(&safe_name); + let _ = std::fs::remove_dir_all(&instance_dir); + } else { + tracing::warn!("Refusing to delete directory for invalid instance name: {name:?}"); + } } Ok(Redirect::to("/instances")) @@ -450,15 +477,18 @@ async fn activate_instance( }; // Pull image + start container in background so the UI stays responsive + // FIX: Use concrete instance ID instead of `WHERE is_active = 1` to avoid + // stale updates trampling a different instance if activation changes. let docker = state.docker.clone(); let db = state.db.clone(); let rcon_pw = instance.rcon_password.clone(); + let instance_id = instance.id; tokio::spawn(async move { { let conn = db.conn(); conn.execute( - "UPDATE instances SET container_state = 'pulling' WHERE is_active = 1", - [], + "UPDATE instances SET container_state = 'pulling' WHERE id = ?1", + params![instance_id], ) .ok(); } @@ -467,8 +497,8 @@ async fn activate_instance( { let conn = db.conn(); conn.execute( - "UPDATE instances SET container_state = 'booting' WHERE is_active = 1", - [], + "UPDATE instances SET container_state = 'booting' WHERE id = ?1", + params![instance_id], ) .ok(); } @@ -479,8 +509,8 @@ async fn activate_instance( if rcon.send_command_async("list".into()).await.is_ok() { let conn = db.conn(); conn.execute( - "UPDATE instances SET container_state = 'running' WHERE is_active = 1", - [], + "UPDATE instances SET container_state = 'running' WHERE id = ?1", + params![instance_id], ) .ok(); tracing::info!("Server is ready (RCON responded)"); @@ -490,8 +520,8 @@ async fn activate_instance( tracing::error!("Server did not become ready within 60 seconds"); let conn = db.conn(); conn.execute( - "UPDATE instances SET container_state = 'error' WHERE is_active = 1", - [], + "UPDATE instances SET container_state = 'error' WHERE id = ?1", + params![instance_id], ) .ok(); } @@ -499,8 +529,8 @@ async fn activate_instance( tracing::error!("Failed to start container: {e}"); let conn = db.conn(); conn.execute( - "UPDATE instances SET container_state = 'error' WHERE is_active = 1", - [], + "UPDATE instances SET container_state = 'error' WHERE id = ?1", + params![instance_id], ) .ok(); } @@ -586,11 +616,17 @@ async fn export_world( .map_err(|_| Redirect::to("/instances"))?; drop(conn); + // Sanitize instance name to prevent path traversal + let safe_name = match sanitize::sanitize_instance_name(&instance_name) { + Some(n) => n, + None => return Err(Redirect::to("/instances")), + }; + let world_dir = state .config .server_directory .join("instances") - .join(&instance_name) + .join(&safe_name) .join("data") .join("world"); @@ -661,6 +697,12 @@ async fn import_world( } }; + // Sanitize instance name to prevent path traversal + let safe_name = match sanitize::sanitize_instance_name(&instance_name) { + Some(n) => n, + None => return Ok(Redirect::to(&format!("/instances/{id}/edit"))), + }; + if is_active { return Ok(Redirect::to(&format!("/instances/{id}/edit"))); } @@ -669,7 +711,7 @@ async fn import_world( .config .server_directory .join("instances") - .join(&instance_name) + .join(&safe_name) .join("data"); // Extract uploaded ZIP @@ -691,10 +733,16 @@ async fn import_world( let mut file = archive .by_index(i) .map_err(|_| Redirect::to(&format!("/instances/{id}/edit")))?; + // Use enclosed_name() for zip-slip protection, then verify within world_dir let out_path = match file.enclosed_name() { Some(p) => world_dir.join(p), None => continue, }; + // Additional safety: verify the resolved path is within world_dir + if !out_path.starts_with(&world_dir) { + tracing::warn!("Zip-slip attempt blocked: {out_path:?}"); + continue; + } if file.is_dir() { let _ = std::fs::create_dir_all(&out_path); } else { @@ -728,38 +776,47 @@ async fn regenerate_world( .map_err(|_| Redirect::to("/instances"))?; drop(conn); + // Sanitize instance name to prevent path traversal + let safe_name = match sanitize::sanitize_instance_name(&instance_name) { + Some(n) => n, + None => return Ok(Redirect::to(&format!("/instances/{id}/edit"))), + }; + if is_active { return Ok(Redirect::to(&format!("/instances/{id}/edit"))); } + // Sanitize seed to prevent Java properties injection + let seed = form.seed.unwrap_or_default(); + let safe_seed = sanitize::sanitize_seed(&seed); + let props_path = state .config .server_directory .join("instances") - .join(&instance_name) + .join(&safe_name) .join("data") .join("server.properties"); - // Update level-seed in server.properties - let seed = form.seed.unwrap_or_default(); + // Update level-seed in server.properties with sanitized value let contents = std::fs::read_to_string(&props_path).unwrap_or_default(); let mut new_lines = Vec::new(); let mut found = false; for line in contents.lines() { if line.trim_start().starts_with("level-seed") { - new_lines.push(format!("level-seed={seed}")); + new_lines.push(format!("level-seed={safe_seed}")); found = true; } else { new_lines.push(line.to_string()); } } if !found { - new_lines.push(format!("level-seed={seed}")); + new_lines.push(format!("level-seed={safe_seed}")); } let _ = std::fs::write(&props_path, new_lines.join("\n")); - // Update DB record - let db_seed: Option<&str> = if seed.is_empty() { None } else { Some(&seed) }; + // Update DB record with sanitized seed + let db_seed: Option<&str> = if safe_seed.is_empty() { None } else { Some(&safe_seed) }; let conn = state.db.conn(); conn.execute( "UPDATE instances SET world_seed = ?1, updated_at = datetime('now') WHERE id = ?2", @@ -773,7 +830,7 @@ async fn regenerate_world( .config .server_directory .join("instances") - .join(&instance_name) + .join(&safe_name) .join("data") .join("world"); let _ = std::fs::remove_dir_all(&world_dir); -- cgit v1.2.3