diff --git a/ostp-gui/src-tauri/src/lib.rs b/ostp-gui/src-tauri/src/lib.rs index bb6700a..d67d0eb 100644 --- a/ostp-gui/src-tauri/src/lib.rs +++ b/ostp-gui/src-tauri/src/lib.rs @@ -134,16 +134,84 @@ struct AppState(Mutex); // ── Config helpers ──────────────────────────────────────────────────────────── +/// Per-user config location, used whenever the config cannot live next to the +/// executable. +fn user_config_path() -> PathBuf { + let base = std::env::var_os(if cfg!(windows) { "APPDATA" } else { "HOME" }) + .map(PathBuf::from) + .unwrap_or_else(std::env::temp_dir); + let dir = if cfg!(windows) { base.join("OSTP") } else { base.join(".config").join("ostp") }; + dir.join("config.json") +} + +/// Where the GUI reads and writes its configuration. +/// +/// Portable installs keep the config beside the executable, which is what the +/// zip has always done, and that is preserved wherever the directory is +/// actually writable. +/// +/// What it must never do again is fall back to a bare relative `config.json`. +/// That resolves against the process working directory, which for a Start Menu +/// shortcut is whatever Windows chose — often `C:\Windows\System32`. Reading +/// and saving settings then failed with "Access is denied" (os error 5), and on +/// a writable working directory it would have been worse still: settings would +/// silently persist somewhere unrelated and appear to vanish. +/// +/// Writability is measured rather than inferred from the install location. An +/// installer can put the app anywhere — a per-machine install onto a data drive +/// may well be writable, while Program Files is not — so the location alone +/// says nothing. fn get_config_path() -> PathBuf { if let Ok(exe_path) = std::env::current_exe() { if let Some(parent) = exe_path.parent() { - let path = parent.join("config.json"); - if path.exists() { - return path; + let portable = parent.join("config.json"); + if portable.exists() { + if is_file_writable(&portable) { + return portable; + } + // Read-only beside the exe: unusable as the live file, but its + // contents are still worth carrying over once. + let user = user_config_path(); + if !user.exists() { + if let Some(dir) = user.parent() { + let _ = std::fs::create_dir_all(dir); + } + let _ = std::fs::copy(&portable, &user); + } + } else if is_dir_writable(parent) { + // No config yet and the directory takes writes: a portable + // unzip, so keep the config travelling with the folder. + return portable; } } } - PathBuf::from("config.json") + + let path = user_config_path(); + if let Some(dir) = path.parent() { + let _ = std::fs::create_dir_all(dir); + } + path +} + +/// Whether an existing file can actually be written to. +/// +/// Answered by opening it, not by reading permission bits: on Windows the +/// effective answer depends on the ACL and on virtualization, and `readonly()` +/// reflects neither. +fn is_file_writable(path: &std::path::Path) -> bool { + std::fs::OpenOptions::new().append(true).open(path).is_ok() +} + +/// Whether new files can be created in a directory, tested by doing it. +fn is_dir_writable(dir: &std::path::Path) -> bool { + let probe = dir.join(format!(".ostp-write-test-{}", std::process::id())); + match std::fs::File::create(&probe) { + Ok(_) => { + let _ = std::fs::remove_file(&probe); + true + } + Err(_) => false, + } } fn map_to_client_config(raw: &ClientConfigRaw, mode: &str) -> ostp_client::config::ClientConfig { @@ -836,21 +904,8 @@ fn helper_args_file() -> PathBuf { base.join("OSTP").join("helper-args.json") } -/// Minimal XML text escaping for the values interpolated into the task -/// definition. Paths and usernames are attacker-irrelevant here but can easily -/// contain `&`, which would otherwise produce invalid XML and a confusing -/// schtasks parse failure. -#[cfg(target_os = "windows")] -fn xml_escape(s: &str) -> String { - s.replace('&', "&") - .replace('<', "<") - .replace('>', ">") - .replace('"', """) - .replace('\'', "'") -} - -/// Reverse of [`xml_escape`]. `&` must be undone last or `&lt;` would -/// come back as `<`. +/// Undoes XML entity escaping. `&` must be handled last, or `&lt;` +/// would come back as `<`. #[cfg(target_os = "windows")] fn xml_unescape(s: &str) -> String { s.replace(""", "\"") @@ -923,151 +978,6 @@ fn helper_task_matches(exe: &std::path::Path) -> bool { } } -/// Register the Scheduled Task. This is the ONLY step that needs elevation, and -/// it happens once per machine; every later tunnel start reuses the task. -/// -/// RunLevel=HIGHEST makes the task run elevated, and because a task launch is -/// not an elevation request, Windows shows no consent dialog for it. -#[cfg(target_os = "windows")] -fn install_helper_task(exe: &std::path::Path) -> anyhow::Result<()> { - let args_file = helper_args_file(); - if let Some(dir) = args_file.parent() { - std::fs::create_dir_all(dir)?; - } - - // Register from an XML definition rather than /TR. The command line would - // otherwise need the exe path and the args path quoted INSIDE an already - // quoted /TR value, escaped again through ShellExecuteW — a notoriously - // brittle chain when either path contains a space, which both of these do - // by default (Program Files, and usernames with spaces). XML also lets the - // battery and time-limit settings below be stated explicitly. - let user = format!( - "{}\\{}", - std::env::var("USERDOMAIN").unwrap_or_else(|_| "%COMPUTERNAME%".into()), - std::env::var("USERNAME").unwrap_or_default() - ); - let xml = format!( - r#" - - - Runs the OSTP TUN helper elevated so enabling the tunnel does not prompt for consent every time. - - - - {user} - InteractiveToken - HighestAvailable - - - - Parallel - false - false - false - false - PT0S - true - false - true - - - - {exe} - --args-file "{args}" - - - -"#, - user = xml_escape(&user), - exe = xml_escape(&exe.display().to_string()), - args = xml_escape(&args_file.display().to_string()), - ); - - // schtasks /Create /XML expects UTF-16LE with a BOM. - let xml_path = std::env::temp_dir().join(format!("ostp_task_{}.xml", rand::random::())); - let mut utf16: Vec = vec![0xFF, 0xFE]; - for unit in xml.encode_utf16() { - utf16.extend_from_slice(&unit.to_le_bytes()); - } - std::fs::write(&xml_path, &utf16)?; - - // Registering a HighestAvailable task is itself privileged: this is the one - // prompt, and it happens once per machine. - // - // Elevate through PowerShell's Start-Process -Wait rather than - // ShellExecuteW. ShellExecuteW returns as soon as the elevated process is - // LAUNCHED, so the XML below was being deleted while schtasks was still - // starting up — registration then failed, leaving the user with a consent - // prompt that accomplished nothing, followed by a second prompt from the - // fallback path. -Wait makes the deletion safe and lets the exit code be - // checked instead of guessed at by polling. - // - // ArgumentList takes an array, so the task name and XML path never need - // quoting or escaping through a command line, only PowerShell's own - // single-quote doubling. - let ps = format!( - "$p = Start-Process -FilePath 'schtasks.exe' -Verb RunAs -Wait -PassThru \ - -WindowStyle Hidden -ArgumentList @('/Create','/TN','{}','/XML','{}','/F'); \ - exit $p.ExitCode", - ps_quote(HELPER_TASK_NAME), - ps_quote(&xml_path.display().to_string()), - ); - - let status = quiet_command("powershell") - .args(["-NoProfile", "-NonInteractive", "-WindowStyle", "Hidden", "-Command", &ps]) - .status(); - - // 1223 is ERROR_CANCELLED: the consent prompt was declined. Nothing was - // launched, so there is no point waiting for a task to appear. - if let Ok(s) = &status { - if s.code() == Some(1223) { - let _ = std::fs::remove_file(&xml_path); - anyhow::bail!("the consent prompt was declined"); - } - } - - // The exit code is advisory only, never proof of success. `-Verb RunAs` - // launches through ShellExecute, and a non-elevated parent frequently - // cannot read the elevated child's exit code — `$p.ExitCode` then yields - // $null, and `exit $null` leaves PowerShell reporting 0. A failed - // registration would sail straight through a `s.success()` check. - // - // Worse, -Wait does not reliably block until the elevated process exits. - // Deleting the XML right after the call raced schtasks reading it — the - // exact bug that made the previous attempt fail — so wait for the task - // itself to show up. These queries are windowless, so unlike the earlier - // polling loop they cost the user nothing to watch. - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); - let mut registered = false; - while std::time::Instant::now() < deadline { - if helper_task_matches(exe) { - registered = true; - break; - } - std::thread::sleep(std::time::Duration::from_millis(250)); - } - - // Only now is deleting it safe. - let _ = std::fs::remove_file(&xml_path); - - if registered { - return Ok(()); - } - match status { - Ok(s) => anyhow::bail!( - "the scheduled task did not appear after registration (powershell exit {:?})", - s.code() - ), - Err(e) => anyhow::bail!("could not run powershell to register the task: {e}"), - } -} - -/// Escape a value for embedding in a PowerShell single-quoted string. -#[cfg(target_os = "windows")] -fn ps_quote(s: &str) -> String { - s.replace('\'', "''") -} - #[cfg(target_os = "windows")] fn launch_as_admin(exe: &std::path::PathBuf, token: &str, port: u16) -> anyhow::Result<()> { // Preferred path: hand the parameters over in a file and trigger the @@ -1083,11 +993,12 @@ fn launch_as_admin(exe: &std::path::PathBuf, token: &str, port: u16) -> anyhow:: let wrote_args = std::fs::write(&args_file, payload.to_string()).is_ok(); if wrote_args { - if !helper_task_matches(exe) { - if let Err(e) = install_helper_task(exe) { - eprintln!("[OSTP] could not register the helper task ({e}); falling back to a direct elevated launch"); - } - } + // Deliberately does NOT create the task when it is missing. Registering + // one is privileged, so the app could only do it by raising the very + // prompt this exists to avoid — and it would then charge the user two + // prompts for the privilege. Creating it belongs to the installer, + // which is already elevated. Without it we simply fall through to the + // direct elevated launch, which prompts once per connect as before. if helper_task_matches(exe) { let run = quiet_command("schtasks") .args(["/Run", "/TN", HELPER_TASK_NAME]) diff --git a/ostp-gui/src-tauri/windows/hooks.nsh b/ostp-gui/src-tauri/windows/hooks.nsh index 2609a15..99ea14a 100644 --- a/ostp-gui/src-tauri/windows/hooks.nsh +++ b/ostp-gui/src-tauri/windows/hooks.nsh @@ -2,102 +2,70 @@ ; without a consent prompt. ; ; This belongs in the installer, not in the app. Registering a task that runs -; with elevated rights is itself a privileged operation, so an unprivileged GUI -; can only get one by raising a UAC prompt — which is the very thing we are -; trying to remove. The installer already runs elevated, so here it costs -; nothing: the user consents once, to the install, and never again per connect. +; elevated is itself a privileged operation, so an unprivileged GUI could only +; obtain one by raising the very prompt we are trying to remove. The installer +; already runs elevated (installMode is perMachine), so here it costs nothing: +; the user consents once, to the install, and never again per connect. ; -; The task carries no usable trigger (a one-shot dated in the past), because it -; exists solely to be started on demand by the app. - -!macro OSTP_WRITE_TASK_XML OUTFILE - ; NSIS is built in Unicode mode here, so FileWrite emits UTF-16LE — which is - ; what `schtasks /XML` requires. It still needs the byte-order mark itself. - FileOpen $R1 "${OUTFILE}" w - FileWriteWord $R1 65279 - FileWrite $R1 '$\r$\n' - FileWrite $R1 '$\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' Starts the OSTP TUN helper elevated so connecting does not prompt for consent every time.$\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - ; S-1-5-32-545 is BUILTIN\Users by SID rather than by name: the name is - ; localized ("Пользователи" on a Russian Windows) and would not resolve. - ; Combined with InteractiveToken this makes the task run as whichever user - ; actually launches it, so a machine-wide install still works for every - ; account instead of only the one that happened to run the installer. - FileWrite $R1 ' S-1-5-32-545$\r$\n' - FileWrite $R1 ' InteractiveToken$\r$\n' - FileWrite $R1 ' HighestAvailable$\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - ; Parallel: reconnecting before a previous helper has fully exited must not - ; be silently dropped as a duplicate instance. - FileWrite $R1 ' Parallel$\r$\n' - ; A VPN is most needed on battery, and a tunnel must not be killed on unplug. - FileWrite $R1 ' false$\r$\n' - FileWrite $R1 ' false$\r$\n' - FileWrite $R1 ' false$\r$\n' - FileWrite $R1 ' false$\r$\n' - ; PT0S disables the execution time limit; the default would tear the tunnel - ; down after three days. - FileWrite $R1 ' PT0S$\r$\n' - FileWrite $R1 ' true$\r$\n' - FileWrite $R1 ' false$\r$\n' - FileWrite $R1 ' true$\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $INSTDIR\ostp-tun-helper.exe$\r$\n' - ; The port and auth token change per launch and a task stores a fixed command - ; line, so they travel in this file instead. %LOCALAPPDATA% is deliberately - ; left unexpanded: Task Scheduler expands it when the task runs, which lands - ; on the profile of whoever launched it rather than the installing user's. - FileWrite $R1 ' --args-file "%LOCALAPPDATA%\OSTP\helper-args.json"$\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 ' $\r$\n' - FileWrite $R1 '$\r$\n' - FileClose $R1 -!macroend +; The task carries no trigger at all — it exists solely to be started on demand. !macro NSIS_HOOK_POSTINSTALL ; Bundled resources land in $INSTDIR\resources, but the helper loads wintun ; with a plain LoadLibrary, which searches its own directory — so put a copy - ; beside the executables. - DetailPrint "Placing wintun.dll next to the helper..." - CopyFiles /SILENT "$INSTDIR\resources\wintun.dll" "$INSTDIR\wintun.dll" + ; beside the executables. The destination is the directory, not a file path: + ; CopyFiles takes a target directory, and naming the file made it fail. + ${If} ${FileExists} "$INSTDIR\resources\wintun.dll" + DetailPrint "Placing wintun.dll next to the helper..." + CopyFiles /SILENT "$INSTDIR\resources\wintun.dll" "$INSTDIR" + ${Else} + DetailPrint "WARNING: resources\wintun.dll is missing; TUN mode will not start." + ${EndIf} + ; Registered through PowerShell's ScheduledTasks module rather than + ; `schtasks /XML`. Generating the XML from NSIS wrote a UTF-16 byte-order mark + ; ahead of content whose encoding depended on whether makensis was built in + ; Unicode mode, and schtasks rejected the result outright: + ; "The task XML is malformed. (1,2)::ERROR: incorrect document syntax" + ; The cmdlets take the same settings as arguments, so no file is written and + ; there is no encoding to get wrong. + ; + ; The command is delimited with backticks, NSIS's third quote character, so + ; that PowerShell's own single quotes and the shell's double quotes can both + ; appear literally — inside a single-quoted NSIS string the first PowerShell + ; quote would have terminated the argument early. + ; + ; $$ is an escaped literal dollar for PowerShell's variables; a bare $ would + ; be read by NSIS as one of its own. The helper argument is assembled with + ; [char]34 instead of nested quotes so that a username containing a space + ; still yields a correctly quoted path, without three levels of escaping. + ; + ; The principal is the SID S-1-5-32-545 (BUILTIN\Users) rather than the + ; installing user, so a per-machine install serves every account instead of + ; only whoever ran the installer. The SID is used because the name is + ; localized and would not resolve. %LOCALAPPDATA% is likewise left unexpanded + ; for Task Scheduler to resolve per running user. DetailPrint "Registering the OSTP TUN helper task..." - !insertmacro OSTP_WRITE_TASK_XML "$PLUGINSDIR\ostp-helper-task.xml" - - ; /F overwrites an existing registration, so reinstalling or upgrading to a - ; different directory repoints the task instead of leaving a stale path — the - ; app verifies the registered path at runtime and would otherwise have to - ; re-register it with a prompt. - nsExec::ExecToLog 'schtasks.exe /Create /TN "OSTP TUN Helper" /XML "$PLUGINSDIR\ostp-helper-task.xml" /F' + nsExec::ExecToLog `powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command "$$act = New-ScheduledTaskAction -Execute '$INSTDIR\ostp-tun-helper.exe' -Argument ('--args-file ' + [char]34 + '%LOCALAPPDATA%\OSTP\helper-args.json' + [char]34); $$prn = New-ScheduledTaskPrincipal -GroupId 'S-1-5-32-545' -RunLevel Highest; $$set = New-ScheduledTaskSettingsSet -AllowStartIfOnBatteries -DontStopIfGoingOnBatteries -ExecutionTimeLimit ([TimeSpan]::Zero) -MultipleInstances Parallel; Register-ScheduledTask -TaskName 'OSTP TUN Helper' -Action $$act -Principal $$prn -Settings $$set -Force | Out-Null"` Pop $R0 - Delete "$PLUGINSDIR\ostp-helper-task.xml" ${If} $R0 == 0 - DetailPrint "Helper task registered; connecting will not prompt for consent." + DetailPrint "Helper task registered; connecting will not ask for consent." ${Else} - ; Not fatal. The app keeps a fallback that registers the task itself on - ; first connect, at the cost of the one prompt this was meant to avoid. - DetailPrint "Could not register the helper task (schtasks returned $R0)." - DetailPrint "OSTP will still work, but the first connect will ask for consent." + ; Not fatal: the app still works, it just falls back to an elevated launch + ; that asks for consent on each connect. + DetailPrint "Could not register the helper task (exit $R0)." + DetailPrint "OSTP will still work, but every connect will ask for consent." ${EndIf} !macroend !macro NSIS_HOOK_PREUNINSTALL - ; Leaving the task behind would point at a deleted executable, and the app - ; treats a mismatched path as grounds to re-register. + ; Leaving the task behind would point it at a deleted executable, and + ; `schtasks /Run` reports success for merely accepting such a request — the + ; app would wait on a helper that never starts. DetailPrint "Removing the OSTP TUN helper task..." nsExec::ExecToLog 'schtasks.exe /Delete /TN "OSTP TUN Helper" /F' Pop $R0 - ; This copy was made by the install hook, so the uninstaller does not know - ; about it and would otherwise leave it behind. + ; Copied by the install hook, so the uninstaller has no record of it. Delete "$INSTDIR\wintun.dll" !macroend