diff --git a/crates/punktfunk-host/src/gamestream/mod.rs b/crates/punktfunk-host/src/gamestream/mod.rs index e304bb3..47b5f34 100644 --- a/crates/punktfunk-host/src/gamestream/mod.rs +++ b/crates/punktfunk-host/src/gamestream/mod.rs @@ -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" + ), } } diff --git a/crates/punktfunk-host/src/mgmt_token.rs b/crates/punktfunk-host/src/mgmt_token.rs index 5a4dd2a..05c09ff 100644 --- a/crates/punktfunk-host/src/mgmt_token.rs +++ b/crates/punktfunk-host/src/mgmt_token.rs @@ -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 { 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 { (!tok.is_empty()).then(|| tok.to_string()) } -/// Write `PUNKTFUNK_MGMT_TOKEN=` to `path`, mode 0600 (never briefly world-readable). +/// Write `PUNKTFUNK_MGMT_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); } diff --git a/crates/punktfunk-host/src/windows/install.rs b/crates/punktfunk-host/src/windows/install.rs index 4bde029..d43bfe6 100644 --- a/crates/punktfunk-host/src/windows/install.rs +++ b/crates/punktfunk-host/src/windows/install.rs @@ -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()); + } } } diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index 61bdaba..9bcc270 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -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(()) }