fix(host/security): Windows DACL hardening — close audit #2, #3, #8, #11

Windows local-privilege findings from design/security-review-2026-06-28.md. These are #[cfg(windows)] paths (verify in CI / on the box; this Linux dev VM can't compile MSVC). They follow the existing write_secret_file/icacls patterns; the cross-platform parts are cargo check/clippy/test green. - #2 [HIGH]: route the mgmt bearer token write through the shared write_secret_file so it gets the SAME Windows DACL (SYSTEM/Administrators) as the host key — it was cfg(unix)-only and left Users-readable, leaking full mgmt admin authority to any local user. - #3 [HIGH]: create_private_dir now applies a restrictive DACL to the %ProgramData%\punktfunk config directory (re-owns to Administrators to defeat a pre-creation, strips inheritance, SYSTEM/Admins/OWNER full + Users read-only) so a local user can't plant host.env/apps.json that the SYSTEM service trusts (env/arg-injection LPE). host.env is now written DACL-locked via write_secret_file; the config + logs dirs go through create_private_dir. - #8 [LOW]: write the web-console password file empty, icacls-lock it, THEN write the secret — closes the brief write-then-icacls TOCTOU window. - #11 [LOW]: the SYSTEM logs dir is DACL-locked (Users read-only, no create), so a local user can't pre-plant host.log as a reparse/hardlink to redirect SYSTEM's writes (subsumed by the #3 dir lockdown). Deferred: #5 (host<->UMDF gamepad/IDD shared-section Everyone:GENERIC_ALL). The section SDDL is intentionally permissive because the UMDF driver opens it under a restricted token of unknown SID/integrity; scoping it blind would likely break the live-validated gamepad/IDD pipeline, so it needs on-box validation first. Tracked in the report. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-28 22:14:19 +00:00
parent 3532e35b75
commit 6f903f79bc
4 changed files with 91 additions and 27 deletions
@@ -267,9 +267,10 @@ pub(crate) fn config_dir() -> PathBuf {
 }

 /// Create `dir` (and parents) owner-private — **0700** on Unix (so the host's secrets aren't readable
-/// by other local users via a traversable config path). Best-effort on Windows: the dir inherits the
-/// (Users-readable) `%ProgramData%` ACL, so secret *files* are individually locked down by
-/// [`write_secret_file`]. Tightens an already-existing dir too.
+/// by other local users via a traversable config path). On Windows, applies a restrictive DACL
+/// ([`restrict_dir_to_system_admins`]) so a local unprivileged user can't pre-create / plant files in
+/// the config tree (the default `%ProgramData%` ACL grants Users *create*; security-review
+/// 2026-06-28 #3/#11). Tightens (and re-owns) an already-existing dir too.
 pub(crate) fn create_private_dir(dir: &std::path::Path) -> std::io::Result<()> {
    #[cfg(unix)]
    {
@@ -286,7 +287,60 @@ pub(crate) fn create_private_dir(dir: &std::path::Path) -> std::io::Result<()> {
    }
    #[cfg(not(unix))]
    {
-        std::fs::create_dir_all(dir)
+        let r = std::fs::create_dir_all(dir);
+        #[cfg(windows)]
+        restrict_dir_to_system_admins(dir);
+        r
+    }
+}
+
+/// Best-effort Windows DACL lockdown of the config *directory* (the companion to
+/// [`restrict_to_system_admins`] for files). The default `%ProgramData%` ACL lets `BUILTIN\Users`
+/// create subfolders/files (and become `CREATOR OWNER`), so a non-admin could pre-create the
+/// `punktfunk` dir or plant a `host.env`/`apps.json` that the privileged SYSTEM service then trusts
+/// (LPE; security-review 2026-06-28 #3). This re-owns the dir to Administrators (defeating a
+/// pre-creation), strips inheritance, and sets an explicit DACL: SYSTEM/Administrators/OWNER full
+/// (object+container inherit so child files/dirs inherit it), and Users **read-only** (so existing
+/// reads of non-secret config keep working but a local user can no longer write/plant). Secret files
+/// are additionally locked to SYSTEM/Admins by [`write_secret_file`]. Hard-coded SIDs
+/// (locale-independent) via the absolute `%SystemRoot%` path; never fatal.
+#[cfg(windows)]
+fn restrict_dir_to_system_admins(dir: &std::path::Path) {
+    let icacls = std::env::var("SystemRoot")
+        .map(|r| format!("{r}\\System32\\icacls.exe"))
+        .unwrap_or_else(|_| "icacls".to_string());
+    // Reset ownership of the directory object to Administrators first, so a dir a non-admin may have
+    // pre-created can't keep OWNER control (an owner can always rewrite the DACL). No `/T` — re-owning
+    // the dir itself is what defeats the pre-creation; recursing a large captures tree each call is
+    // needless churn (secret files are individually owner-locked by `write_secret_file`).
+    let _ = std::process::Command::new(&icacls)
+        .arg(dir.as_os_str())
+        .args(["/setowner", "*S-1-5-32-544"]) // BUILTIN\Administrators
+        .stdout(std::process::Stdio::null())
+        .stderr(std::process::Stdio::null())
+        .status();
+    let status = std::process::Command::new(&icacls)
+        .arg(dir.as_os_str())
+        .args([
+            "/inheritance:r",
+            "/grant:r",
+            "*S-1-5-18:(OI)(CI)(F)", // NT AUTHORITY\SYSTEM
+            "/grant:r",
+            "*S-1-5-32-544:(OI)(CI)(F)", // BUILTIN\Administrators
+            "/grant:r",
+            "*S-1-3-4:(OI)(CI)(F)", // OWNER RIGHTS
+            "/grant:r",
+            "*S-1-5-32-545:(OI)(CI)(RX)", // BUILTIN\Users — read-only (no create/write → no plant)
+        ])
+        .stdout(std::process::Stdio::null())
+        .stderr(std::process::Stdio::null())
+        .status();
+    match status {
+        Ok(s) if s.success() => {}
+        _ => tracing::warn!(
+            dir = %dir.display(),
+            "config-dir DACL hardening did not fully succeed — a local user may be able to plant config files"
+        ),
    }
 }

@@ -11,9 +11,6 @@
 use anyhow::{Context, Result};
 use rand::RngCore;
 use std::fs;
-use std::io::Write;
-#[cfg(unix)]
-use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
 use std::path::Path;

 const ENV_VAR: &str = "PUNKTFUNK_MGMT_TOKEN";
@@ -38,9 +35,10 @@ pub fn load_or_generate() -> Result<String> {
    rand::thread_rng().fill_bytes(&mut buf);
    let token = hex::encode(buf);
    let dir = crate::gamestream::config_dir();
-    fs::create_dir_all(&dir).with_context(|| format!("create {}", dir.display()))?;
+    // Owner-private dir (0700 Unix / DACL-locked Windows) so the token can't leak via the config path.
+    crate::gamestream::create_private_dir(&dir).with_context(|| format!("create {}", dir.display()))?;
    write_token(&path, &token)?;
-    tracing::info!(path = %path.display(), "generated and persisted management API token (0600)");
+    tracing::info!(path = %path.display(), "generated and persisted management API token (owner-only)");
    Ok(token)
 }

@@ -55,19 +53,15 @@ fn parse_token(contents: &str) -> Option<String> {
    (!tok.is_empty()).then(|| tok.to_string())
 }

-/// Write `PUNKTFUNK_MGMT_TOKEN=<token>` to `path`, mode 0600 (never briefly world-readable).
+/// Write `PUNKTFUNK_MGMT_TOKEN=<token>` to `path` as an owner-only secret — 0600 on Unix AND
+/// DACL-locked to SYSTEM/Administrators on Windows. Routes through the shared `write_secret_file` so
+/// the mgmt bearer token (full admin authority) gets the SAME Windows lockdown as the host key; the
+/// bespoke `cfg(unix)`-only writer used to leave it readable by any local user (security-review
+/// 2026-06-28 #2).
 fn write_token(path: &Path, token: &str) -> Result<()> {
-    let mut opts = fs::OpenOptions::new();
-    opts.write(true).create(true).truncate(true);
-    #[cfg(unix)]
-    opts.mode(0o600);
-    let mut f = opts
-        .open(path)
-        .with_context(|| format!("write {}", path.display()))?;
-    writeln!(f, "PUNKTFUNK_MGMT_TOKEN={token}")?;
-    #[cfg(unix)]
-    let _ = fs::set_permissions(path, fs::Permissions::from_mode(0o600));
-    Ok(())
+    let line = format!("PUNKTFUNK_MGMT_TOKEN={token}\n");
+    crate::gamestream::write_secret_file(path, line.as_bytes())
+        .with_context(|| format!("write {}", path.display()))
 }

 #[cfg(test)]
@@ -95,6 +89,7 @@ mod tests {
        assert_eq!(parse_token(&read).as_deref(), Some("cafef00d"));
        #[cfg(unix)]
        {
+            use std::os::unix::fs::PermissionsExt;
            let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777;
            assert_eq!(mode, 0o600);
        }
@@ -271,8 +271,11 @@ fn set_web_password(pw_path: &Path, pw_file: Option<&str>) {
            }
        });
    if let Some(pw) = password {
-        if std::fs::write(pw_path, format!("PUNKTFUNK_UI_PASSWORD={pw}\n")).is_err() {
-            eprintln!("warning: could not write {}", pw_path.display());
+        // Create the file EMPTY first, lock its DACL, THEN write the secret — so the cleartext
+        // password is never present at the inherited (Users-readable) %ProgramData% ACL, even for
+        // the brief window before icacls runs (security-review 2026-06-28 #8).
+        if std::fs::write(pw_path, b"").is_err() {
+            eprintln!("warning: could not create {}", pw_path.display());
            return;
        }
        // Lock down: drop inheritance, grant only Administrators (S-1-5-32-544) + SYSTEM (S-1-5-18).
@@ -287,6 +290,10 @@ fn set_web_password(pw_path: &Path, pw_file: Option<&str>) {
                "*S-1-5-18:F",
            ],
        );
+        // Now write the secret into the already-locked file (truncate keeps the explicit DACL).
+        if std::fs::write(pw_path, format!("PUNKTFUNK_UI_PASSWORD={pw}\n")).is_err() {
+            eprintln!("warning: could not write {}", pw_path.display());
+        }
    }
 }

@@ -114,13 +114,15 @@ pub fn main(args: &[String]) -> Result<()> {
 /// stdout/stderr are redirected to `host.log` in the same dir.
 pub fn service_log_path() -> PathBuf {
    let dir = crate::gamestream::config_dir().join("logs");
-    let _ = std::fs::create_dir_all(&dir);
+    // DACL-locked (Users read-only, no create) so a local user can't pre-plant SYSTEM log files as
+    // reparse points / hardlinks to redirect the SYSTEM service's writes (security-review #11).
+    let _ = crate::gamestream::create_private_dir(&dir);
    dir.join("service.log")
 }

 fn host_log_path() -> PathBuf {
    let dir = crate::gamestream::config_dir().join("logs");
-    let _ = std::fs::create_dir_all(&dir);
+    let _ = crate::gamestream::create_private_dir(&dir);
    dir.join("host.log")
 }

@@ -684,7 +686,9 @@ fn ensure_default_host_env() -> Result<()> {
        return Ok(());
    }
    if let Some(dir) = path.parent() {
-        std::fs::create_dir_all(dir).ok();
+        // DACL-lock the config dir on creation so a local user can't pre-create it and plant a
+        // host.env (which feeds the SYSTEM service's env + command line) — security-review #3.
+        crate::gamestream::create_private_dir(dir).ok();
    }
    let default = "# punktfunk host configuration (read by the Windows service).\n\
        # KEY=VALUE per line; '#' comments. Restart the service after editing:\n\
@@ -707,7 +711,11 @@ fn ensure_default_host_env() -> Result<()> {
        \n\
        # Force a specific render GPU by name substring (multi-GPU boxes only):\n\
        # PUNKTFUNK_RENDER_ADAPTER=4090\n";
-    std::fs::write(&path, default).with_context(|| format!("write {}", path.display()))?;
+    // Write host.env DACL-locked to SYSTEM/Administrators: it controls the SYSTEM service's
+    // environment + launched command line, so a local user must not be able to read or tamper with
+    // it (security-review 2026-06-28 #3).
+    crate::gamestream::write_secret_file(&path, default.as_bytes())
+        .with_context(|| format!("write {}", path.display()))?;
    println!("Wrote default config: {}", path.display());
    Ok(())
 }