diff --git a/crates/punktfunk-host/src/windows/install.rs b/crates/punktfunk-host/src/windows/install.rs index ab16acb7..cb30649c 100644 --- a/crates/punktfunk-host/src/windows/install.rs +++ b/crates/punktfunk-host/src/windows/install.rs @@ -250,6 +250,65 @@ fn privileged_sids() -> Result>> { .collect() } +/// Owner check for a SINGLE secret file: `Some(true)` if owned by SYSTEM / Administrators / +/// TrustedInstaller, `Some(false)` if owned by any other (non-privileged) account, `None` if the +/// owner could not be determined. Used to distrust a `host.env` / `web-password` a non-admin +/// pre-created under `%ProgramData%` before a privileged install ran — the file's bytes would +/// otherwise be adopted verbatim into the SYSTEM service's environment / the console password +/// (security-review 2026-08-15 findings 3c and 4). Reads the security descriptor directly, like +/// [`ensure_admin_only_source`], to stay locale-independent. Must be consulted BEFORE any +/// `create_private_dir` re-owns the file to Administrators and erases the signal. +#[cfg(windows)] +pub(crate) fn is_admin_owned(path: &Path) -> Option { + use std::os::windows::ffi::OsStrExt; + use windows::core::PCWSTR; + use windows::Win32::Foundation::{LocalFree, HLOCAL}; + use windows::Win32::Security::Authorization::{GetNamedSecurityInfoW, SE_FILE_OBJECT}; + use windows::Win32::Security::{ + EqualSid, IsValidSid, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, + }; + + let wide: Vec = path + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + let mut owner = PSID::default(); + let mut sd = PSECURITY_DESCRIPTOR::default(); + // SAFETY: `wide` is NUL-terminated and outlives the call; the out-params are live locals; the + // returned descriptor is the single allocation, LocalFree'd below (owner points into it). + let rc = unsafe { + GetNamedSecurityInfoW( + PCWSTR(wide.as_ptr()), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + Some(&mut owner), + None, + None, + None, + &mut sd, + ) + }; + let verdict = (|| -> Option { + rc.ok().ok()?; + let privileged = privileged_sids().ok()?; + // SAFETY: `owner` points into the descriptor returned above; IsValidSid is the probe. + if owner.is_invalid() || !unsafe { IsValidSid(owner) }.as_bool() { + return None; + } + let admin = privileged.iter().any(|p| { + // SAFETY: `owner` passed IsValidSid; `p` is an owned, length-exact SID copy. + unsafe { EqualSid(owner, PSID(p.as_ptr().cast_mut().cast())) }.is_ok() + }); + Some(admin) + })(); + // SAFETY: `sd` is the single LocalAlloc'd descriptor GetNamedSecurityInfoW returned. + unsafe { + let _ = LocalFree(Some(HLOCAL(sd.0))); + } + verdict +} + /// The subject CN both driver-signing certs carry (`build-pf-vdisplay.ps1` / /// `build-gamepad-drivers.ps1`). certutil matches a CertId against the subject, so this is how we /// find our own certs again without parsing any localized output — see `purge_driver_certs`. @@ -757,12 +816,30 @@ fn web_setup(args: &[String]) -> Result<()> { /// Source: a non-empty `--password-file` (fresh install) > keep existing (upgrade) > random fallback. /// Writes `PUNKTFUNK_UI_PASSWORD=\n` (LF, no BOM) + ACLs it to Administrators + SYSTEM only. fn set_web_password(pw_path: &Path, pw_file: Option<&str>) { + // A password file that exists but is owned by a NON-admin was planted by an unprivileged user + // before this privileged install (`%ProgramData%` CREATOR OWNER). The installer's + // `FreshWebInstall := not FileExists` check then mistakes it for an upgrade, skips the password + // page, and adopts the attacker's console password. Distrust it: rename aside and rotate to a + // fresh random below (`!planted` forces the random branch even if the rename failed). A password + // file from a prior privileged install is Administrators-owned and is kept. security-review + // 2026-08-15 finding 4. + let planted = pw_path.exists() && is_admin_owned(pw_path) == Some(false); + if planted { + let mut aside = pw_path.to_path_buf().into_os_string(); + aside.push(".untrusted"); + let aside = std::path::PathBuf::from(aside); + let _ = std::fs::remove_file(&aside); + let _ = std::fs::rename(pw_path, &aside); + println!( + "web console password file was owned by a non-admin (planted before install) — rotating to a fresh password" + ); + } let password = pw_file .and_then(|f| std::fs::read_to_string(f).ok()) .map(|s| s.trim().to_string()) .filter(|s| !s.is_empty()) .or_else(|| { - if pw_path.exists() { + if pw_path.exists() && !planted { println!("keeping existing web console password"); None } else { diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index 07a18b1b..f5353415 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -1366,6 +1366,32 @@ fn uninstall() -> Result<()> { /// defaults to `auto` — the host picks NVENC (NVIDIA) / AMF (AMD) / QSV (Intel) from the GPU vendor. fn ensure_default_host_env() -> Result<()> { let path = host_env_path(); + // If a host.env already exists but is owned by a NON-admin account, it was pre-created by an + // unprivileged user before this privileged install ran (`%ProgramData%` grants BUILTIN\Users + // add-subdirectory + CREATOR OWNER). Its bytes become the SYSTEM service's environment and the + // command line it launches, so it must NOT be adopted verbatim. Checked HERE, before + // `create_private_dir` below re-owns it to Administrators and erases the only signal that + // distinguishes a planted file from a legitimately-provisioned one. A legitimate host.env from a + // prior privileged install is Administrators-owned and passes. security-review 2026-08-15 #3c. + let planted = path.exists() && crate::install::is_admin_owned(&path) == Some(false); + if planted { + // Best-effort rename-aside for forensics; the security guarantee is the `!planted` skip + // below, which drops through to overwriting the file with the default even if this fails. + let mut aside = path.clone().into_os_string(); + aside.push(".untrusted"); + let aside = std::path::PathBuf::from(aside); + let _ = std::fs::remove_file(&aside); + match std::fs::rename(&path, &aside) { + Ok(()) => tracing::warn!( + path = %path.display(), aside = %aside.display(), + "host.env was owned by a non-admin account (planted before install) — renamed aside; writing the default" + ), + Err(e) => tracing::error!( + error = %e, path = %path.display(), + "host.env is non-admin-owned and could not be renamed aside — overwriting it with the default" + ), + } + } // Harden the config dir FIRST, unconditionally — before the `exists()` check, not inside the // branch that creates the file. // @@ -1379,10 +1405,12 @@ fn ensure_default_host_env() -> Result<()> { if let Some(dir) = path.parent() { pf_paths::create_private_dir(dir).ok(); } - if path.exists() { + if path.exists() && !planted { // An existing host.env may predate the hardening (or have been planted before it ran), in // which case it is still owned by whoever created it — and an owner can rewrite the DACL it // inherited. Re-apply the SYSTEM/Administrators lock to the FILE as well as the directory. + // (A non-admin-owned file is `planted` above and is NOT adopted — it falls through to the + // default write below, overwriting it even if the rename-aside failed.) pf_paths::restrict_existing_secret_file(&path); return Ok(()); }