From 7e0d8490e636f9629eb5ace164eb037b17c84ac2 Mon Sep 17 00:00:00 2001 From: "wallydz-bot[bot]" <2909976+wallydz-bot[bot]@users.noreply.github.com> Date: Sat, 21 Feb 2026 20:33:58 +0100 Subject: [PATCH 1/3] docs: add DECISIONS SECURITY_REVIEW TEST_PLAN\n\nPhase 8 deliverables. Baseline verified. Swarm consolidated. --- DECISIONS.md | 1 + SECURITY_REVIEW.md | 1 + TEST_PLAN.md | 1 + 3 files changed, 3 insertions(+) create mode 100644 DECISIONS.md create mode 100644 SECURITY_REVIEW.md create mode 100644 TEST_PLAN.md diff --git a/DECISIONS.md b/DECISIONS.md new file mode 100644 index 0000000..c60e9f3 --- /dev/null +++ b/DECISIONS.md @@ -0,0 +1 @@ +# VPNht Desktop Production Readiness Audit\n\n## Decision 1: Terminate Swarm\n**Problem:** Subagents partial (clone only) or hallucinated Popcorn Time (memory bleed).\n**Options:** Respawn refined | Manual high-impact from D/prev REPORT.\n**Chosen:** Manual.\n**Why:** D concrete diffs (IPC split, Linux KS), baseline pass, time-critical.\n**Risk:** Miss low-prio feats. **Mitigate:** P2 roadmap in REPORT.md.\n\n## Decision 2: Skip Local Rust Builds\n**Problem:** arm64 qemu missing libwebkit2gtk/javascriptcoregtk pc files.\n**Options:** apt/Docker | CI-only.\n**Chosen:** CI-only.\n**Why:** Existing workflows ubuntu-22.04 matrix pass Tauri builds.\n**Risk:** Local dev slower. **Mitigate:** docker-tauri if needed.\n\n## Decision 3: PR Order\nP0: CI enhancements, D refactors (security), tests.\nP1: Real API mocks replace.\nP2: UX feats. \ No newline at end of file diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md new file mode 100644 index 0000000..9dbebfa --- /dev/null +++ b/SECURITY_REVIEW.md @@ -0,0 +1 @@ +# SECURITY_REVIEW.md\n\n## Threat Model\nAssets: creds (keyring), VPN keys, auth tokens, DNS, conn state.\nBoundaries: Frontend-Rust IPC, App-VPN daemon cmds, App-API/updates.\nAttackers: Malicious IPC, leak creds, bypass KS, MITM updates.\n\n## Findings (from audit + D)\n| Sev | ID | Desc | File | Status |\n|-----|----|------|------|--------|\n| Crit | SEC-01 | Mock API no real auth | commands.rs | P0 |\n| High | SEC-03 | IPC store_secure scoped | commands.rs | Fixed D |\n| High | SEC-04 | KS priv Linux iptables | killswitch/linux.rs | Fixed D |\n| Med | SEC-08 | CSP unsafe-inline | tauri.conf | P1 |\n\n## Fixes Implemented\n- IPC allowlist (ALLOWED_STORAGE_KEYS)\n- Linux KS iptables chain\n- AppError structured\n\n## Remaining\n- Real API replace mocks\n- Windows KS\n- Updater pinning\n\nNext: cargo audit CI. \ No newline at end of file diff --git a/TEST_PLAN.md b/TEST_PLAN.md new file mode 100644 index 0000000..4c294eb --- /dev/null +++ b/TEST_PLAN.md @@ -0,0 +1 @@ +# TEST_PLAN.md\n\n## Run Locally\nnpm test\ncd src-tauri && cargo test\n\n## Coverage\n73 vitest pass (utils/stores/VPN flow mocks).\nCargo 2 tests.\nGaps: E2E IPC, real WG mocks, KS.\n\n## CI\nworkflows/test.yml: vitest cargo.\nAdd: e2e playwright stubs. \ No newline at end of file From 0aa7cdaebdf406df9ae4fd4c35631b3ed49b22b0 Mon Sep 17 00:00:00 2001 From: "wallydz-bot[bot]" <2909976+wallydz-bot[bot]@users.noreply.github.com> Date: Sat, 21 Feb 2026 20:39:19 +0100 Subject: [PATCH 2/3] fix(security): prevent command injection in macOS run_privileged_command The previous implementation used format!() to build shell scripts, allowing potential command injection if cmd or args contained special characters like quotes, backticks, or semicolons. Fix by implementing proper shell escaping using single-quote escaping, which safely handles all special characters including embedded quotes. Before (vulnerable): do shell script "cmd args" with administrator privileges After (safe): do shell script 'cmd' 'arg1' 'arg2' with administrator privileges Also improved Command builder to use .arg() instead of .args() with mutable Vec references for cleaner code. --- src-tauri/src/killswitch.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src-tauri/src/killswitch.rs b/src-tauri/src/killswitch.rs index 25605d7..1606ddd 100644 --- a/src-tauri/src/killswitch.rs +++ b/src-tauri/src/killswitch.rs @@ -36,10 +36,26 @@ fn run_privileged_command(cmd: &str, args: &[&str]) -> Result Result { // macOS uses osascript for privilege escalation - let mut full_args = vec!["-e", &format!("do shell script \"{} {}\" with administrator privileges", cmd, args.join(" "))]; + // Build shell-escaped command to prevent injection attacks + fn shell_escape(s: &str) -> String { + // Use single quotes, escaping any embedded single quotes + // This is the safest shell escaping method + format!("'{}'", s.replace("'", "'\\''")) + } + + let mut command_parts = vec![shell_escape(cmd)]; + for arg in args { + command_parts.push(shell_escape(arg)); + } + + let script = format!( + "do shell script {} with administrator privileges", + command_parts.join(" ") + ); Command::new("osascript") - .args(&full_args) + .arg("-e") + .arg(&script) .output() .map_err(|e| format!("Failed to run privileged command: {}", e)) } From c0e42aa140bf2ddd2b3477a3db3b08199a0098bc Mon Sep 17 00:00:00 2001 From: "wallydz-bot[bot]" <2909976+wallydz-bot[bot]@users.noreply.github.com> Date: Sat, 21 Feb 2026 21:10:35 +0100 Subject: [PATCH 3/3] feat(killswitch): implement Windows firewall kill switch using netsh advfirewall - Add setup_wfp() for Windows: blocks all outbound traffic via netsh - Add netsh advfirewall firewall add rule for block/allow rules - Use PowerShell New-NetFirewallRule for WireGuard interface filtering - Allow DHCP/DNS for essential connectivity - Implement teardown_wfp() for cleanup on disable - Add Drop impl to ensure cleanup on struct destruction - Add unit tests for interface name sanitization - Remove password requirement (assumes elevated process) netsh syntax: - Block: netsh advfirewall firewall add rule name="VPNht Kill Switch Block" dir=out action=block remoteip=any enable=yes profile=any - Delete: netsh advfirewall firewall delete rule name="VPNht Kill Switch Block" Fixes Windows kill switch implementation --- src-tauri/src/killswitch.rs | 251 +++++++++++++++++++++++++++++++++++- 1 file changed, 244 insertions(+), 7 deletions(-) diff --git a/src-tauri/src/killswitch.rs b/src-tauri/src/killswitch.rs index 1606ddd..6a1faec 100644 --- a/src-tauri/src/killswitch.rs +++ b/src-tauri/src/killswitch.rs @@ -61,19 +61,51 @@ fn run_privileged_command(cmd: &str, args: &[&str]) -> Result Result { - // Windows uses runas - let mut full_args = vec!["/user:Administrator", &format!("{} {}", cmd, args.join(" "))]; - - Command::new("runas") - .args(&full_args) +fn run_command(cmd: &str, args: &[&str]) -> Result { + // On Windows, netsh commands work without elevation if app has appropriate permissions + // For firewall rules, the app should be run as administrator or have UAC consent + Command::new(cmd) + .args(args) .output() - .map_err(|e| format!("Failed to run privileged command: {}", e)) + .map_err(|e| format!("Failed to run command '{}': {}", cmd, e)) +} + +#[cfg(target_os = "windows")] +fn run_powershell(script: &str) -> Result { + // Run PowerShell command + Command::new("powershell") + .args(["-NoProfile", "-NonInteractive", "-Command", script]) + .output() + .map_err(|e| format!("Failed to run PowerShell: {}", e)) +} + +#[cfg(target_os = "windows")] +fn is_elevated() -> bool { + // Check if running with administrator privileges + let output = run_powershell( + "(New-Object Security.Principal.WindowsPrincipal([Security.Principal.WindowsIdentity]::GetCurrent())).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)" + ); + + match output { + Ok(o) => { + let stdout = String::from_utf8_lossy(&o.stdout).trim().to_string(); + stdout == "True" + }, + Err(_) => false + } } +// Windows firewall rule names +#[cfg(target_os = "windows")] +const FIREWALL_RULE_BLOCK: &str = "VPNht Kill Switch Block"; +#[cfg(target_os = "windows")] +const FIREWALL_RULE_ALLOW_WG: &str = "VPNht Kill Switch Allow WireGuard"; + pub struct KillSwitch { enabled: bool, firewall_rules: Vec, + #[cfg(target_os = "windows")] + wireguard_interface: Option, } impl KillSwitch { @@ -81,6 +113,8 @@ impl KillSwitch { Self { enabled: false, firewall_rules: Vec::new(), + #[cfg(target_os = "windows")] + wireguard_interface: None, } } @@ -179,4 +213,207 @@ impl KillSwitch { info!("All non-VPN traffic blocked"); Ok(()) } + + // Windows implementation using netsh advfirewall + #[cfg(target_os = "windows")] + fn setup_wfp(&mut self) -> Result<(), String> { + info!("Setting up Windows Firewall Kill Switch"); + + // Check if we have elevation + if !is_elevated() { + warn!("Kill Switch may require administrator privileges"); + } + + // Remove any existing rules first (cleanup from previous session) + self.remove_firewall_rules()?; + + // Find WireGuard interface + let wg_interface = self.find_wireguard_interface()?; + if wg_interface.is_some() { + info!("Found WireGuard interface: {:?}", wg_interface); + } + + // Create block rule for all outbound traffic + // netsh advfirewall firewall add rule name="..." dir=out action=block remoteip=any + let block_output = run_command("netsh", &[ + "advfirewall", + "firewall", + "add", + "rule", + &format!("name={}", FIREWALL_RULE_BLOCK), + "dir=out", + "action=block", + "remoteip=any", + "enable=yes", + "profile=any", + ])?; + + if !block_output.status.success() { + let stderr = String::from_utf8_lossy(&block_output.stderr); + return Err(format!("Failed to create block rule: {}", stderr)); + } + info!("Created firewall block rule: {}", FIREWALL_RULE_BLOCK); + + // Allow WireGuard interface traffic if found + if let Some(ref iface) = wg_interface { + self.wireguard_interface = Some(iface.clone()); + + // Use PowerShell for interface-specific rule (netsh doesn't support interface filtering) + let allow_script = format!( + r#"New-NetFirewallRule -DisplayName '{}' -Direction Outbound -Action Allow -InterfaceAlias '{}' -Enabled True -Profile Any"#, + FIREWALL_RULE_ALLOW_WG, iface + ); + + let allow_output = run_powershell(&allow_script)?; + if allow_output.status.success() { + info!("Created firewall allow rule for interface: {}", iface); + } else { + warn!("Could not create WireGuard allow rule, block rule still active"); + } + } + + // Also allow DHCP and DNS for initial connection + self.allow_essential_traffic()?; + + Ok(()) + } + + #[cfg(target_os = "windows")] + fn teardown_wfp(&mut self) -> Result<(), String> { + info!("Tearing down Windows Firewall Kill Switch"); + self.remove_firewall_rules() + } + + #[cfg(target_os = "windows")] + fn remove_firewall_rules(&self) -> Result<(), String> { + // Remove block rule + let _ = run_command("netsh", &[ + "advfirewall", + "firewall", + "delete", + "rule", + &format!("name={}", FIREWALL_RULE_BLOCK), + ]); + info!("Removed firewall block rule"); + + // Remove WireGuard allow rule (via PowerShell) + let remove_script = format!( + r#"Remove-NetFirewallRule -DisplayName '{}' -ErrorAction SilentlyContinue"#, + FIREWALL_RULE_ALLOW_WG + ); + let _ = run_powershell(&remove_script); + info!("Removed WireGuard allow rule"); + + Ok(()) + } + + #[cfg(target_os = "windows")] + fn find_wireguard_interface(&self) -> Result, String> { + // Find WireGuard interface using PowerShell + // Common WireGuard interface names: "wg0", "WireGuard", or any with "wg" prefix + let script = r#" + $adapters = Get-NetAdapter | Where-Object { + $_.InterfaceDescription -like "*WireGuard*" -or + $_.Name -like "wg*" -or + $_.Name -like "*WireGuard*" + } + if ($adapters) { + $adapters[0].Name + } + "#; + + let output = run_powershell(script)?; + let iface_name = String::from_utf8_lossy(&output.stdout).trim().to_string(); + + if iface_name.is_empty() { + Ok(None) + } else { + Ok(Some(iface_name)) + } + } + + #[cfg(target_os = "windows")] + fn allow_essential_traffic(&self) -> Result<(), String> { + // Allow DHCP (UDP 67, 68) for network connectivity + let dhcp_out = run_command("netsh", &[ + "advfirewall", + "firewall", + "add", + "rule", + "name=VPNht Kill Switch DHCP Out", + "dir=out", + "action=allow", + "protocol=udp", + "localport=68", + "remoteport=67", + "enable=yes", + ])?; + + // Allow DNS (UDP/TCP 53) for name resolution + let dns_out = run_command("netsh", &[ + "advfirewall", + "firewall", + "add", + "rule", + "name=VPNht Kill Switch DNS Out", + "dir=out", + "action=allow", + "protocol=any", + "remoteport=53", + "enable=yes", + ])?; + + info!("Allowed essential traffic (DHCP, DNS)"); + Ok(()) + } +} + +impl Drop for KillSwitch { + fn drop(&mut self) { + // Ensure firewall rules are cleaned up when KillSwitch is dropped + if self.enabled { + #[cfg(target_os = "windows")] + { + let _ = self.teardown_wfp(); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_sanitize_interface_name_valid() { + assert_eq!(sanitize_interface_name("wg0"), Ok("wg0".to_string())); + assert_eq!(sanitize_interface_name("WireGuard-1"), Ok("WireGuard-1".to_string())); + assert_eq!(sanitize_interface_name("vpn_test"), Ok("vpn_test".to_string())); + } + + #[test] + fn test_sanitize_interface_name_too_long() { + let long_name = "this_interface_name_is_way_too_long"; + assert!(sanitize_interface_name(long_name).is_err()); + } + + #[test] + fn test_sanitize_interface_name_invalid_chars() { + assert!(sanitize_interface_name("wg 0").is_err()); // space + assert!(sanitize_interface_name("wg@0").is_err()); // @ + assert!(sanitize_interface_name("wg.0").is_err()); // . + } + + #[test] + fn test_killswitch_new() { + let ks = KillSwitch::new(); + assert!(!ks.enabled); + } + + #[cfg(target_os = "windows")] + #[test] + fn test_firewall_rule_names() { + assert_eq!(FIREWALL_RULE_BLOCK, "VPNht Kill Switch Block"); + assert_eq!(FIREWALL_RULE_ALLOW_WG, "VPNht Kill Switch Allow WireGuard"); + } } \ No newline at end of file