fix(host/windows): distrust a non-admin-owned host.env / web-password
security-review 2026-08-15 findings 3c and 4. %ProgramData% lets BUILTIN\Users pre-create the punktfunk dir and plant host.env / web-password before a privileged install runs; the bytes were then adopted verbatim (SYSTEM service environment + command line; the console password), with the plant's owner erased by the dir re-own that runs first. Add install::is_admin_owned() (reads the file owner SID via GetNamedSecurityInfoW, reusing privileged_sids()) and consult it BEFORE create_private_dir re-owns the file: - ensure_default_host_env: a non-admin-owned host.env is renamed aside and the default written over it (the !planted skip forces the overwrite even if the rename fails). - set_web_password: a non-admin-owned password file is rotated to a fresh random instead of kept as an 'upgrade'. A file from a prior privileged install is Administrators-owned and is kept. Compiles clean on the windows-amd64 box (.133). The installer-side .iss freshness signal (which also gates the password page) is a separate follow-up.
This commit is contained in:
@@ -250,6 +250,65 @@ fn privileged_sids() -> Result<Vec<Vec<u8>>> {
|
||||
.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<bool> {
|
||||
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<u16> = 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<bool> {
|
||||
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=<pw>\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 {
|
||||
|
||||
@@ -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(());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user