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>
This commit is contained in:
@@ -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
|
/// 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
|
/// by other local users via a traversable config path). On Windows, applies a restrictive DACL
|
||||||
/// (Users-readable) `%ProgramData%` ACL, so secret *files* are individually locked down by
|
/// ([`restrict_dir_to_system_admins`]) so a local unprivileged user can't pre-create / plant files in
|
||||||
/// [`write_secret_file`]. Tightens an already-existing dir too.
|
/// 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<()> {
|
pub(crate) fn create_private_dir(dir: &std::path::Path) -> std::io::Result<()> {
|
||||||
#[cfg(unix)]
|
#[cfg(unix)]
|
||||||
{
|
{
|
||||||
@@ -286,7 +287,60 @@ pub(crate) fn create_private_dir(dir: &std::path::Path) -> std::io::Result<()> {
|
|||||||
}
|
}
|
||||||
#[cfg(not(unix))]
|
#[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 anyhow::{Context, Result};
|
||||||
use rand::RngCore;
|
use rand::RngCore;
|
||||||
use std::fs;
|
use std::fs;
|
||||||
use std::io::Write;
|
|
||||||
#[cfg(unix)]
|
|
||||||
use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
|
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
|
|
||||||
const ENV_VAR: &str = "PUNKTFUNK_MGMT_TOKEN";
|
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);
|
rand::thread_rng().fill_bytes(&mut buf);
|
||||||
let token = hex::encode(buf);
|
let token = hex::encode(buf);
|
||||||
let dir = crate::gamestream::config_dir();
|
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)?;
|
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)
|
Ok(token)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -55,19 +53,15 @@ fn parse_token(contents: &str) -> Option<String> {
|
|||||||
(!tok.is_empty()).then(|| tok.to_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<()> {
|
fn write_token(path: &Path, token: &str) -> Result<()> {
|
||||||
let mut opts = fs::OpenOptions::new();
|
let line = format!("PUNKTFUNK_MGMT_TOKEN={token}\n");
|
||||||
opts.write(true).create(true).truncate(true);
|
crate::gamestream::write_secret_file(path, line.as_bytes())
|
||||||
#[cfg(unix)]
|
.with_context(|| format!("write {}", path.display()))
|
||||||
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(())
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
@@ -95,6 +89,7 @@ mod tests {
|
|||||||
assert_eq!(parse_token(&read).as_deref(), Some("cafef00d"));
|
assert_eq!(parse_token(&read).as_deref(), Some("cafef00d"));
|
||||||
#[cfg(unix)]
|
#[cfg(unix)]
|
||||||
{
|
{
|
||||||
|
use std::os::unix::fs::PermissionsExt;
|
||||||
let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777;
|
let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777;
|
||||||
assert_eq!(mode, 0o600);
|
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 let Some(pw) = password {
|
||||||
if std::fs::write(pw_path, format!("PUNKTFUNK_UI_PASSWORD={pw}\n")).is_err() {
|
// Create the file EMPTY first, lock its DACL, THEN write the secret — so the cleartext
|
||||||
eprintln!("warning: could not write {}", pw_path.display());
|
// 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;
|
return;
|
||||||
}
|
}
|
||||||
// Lock down: drop inheritance, grant only Administrators (S-1-5-32-544) + SYSTEM (S-1-5-18).
|
// 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",
|
"*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.
|
/// stdout/stderr are redirected to `host.log` in the same dir.
|
||||||
pub fn service_log_path() -> PathBuf {
|
pub fn service_log_path() -> PathBuf {
|
||||||
let dir = crate::gamestream::config_dir().join("logs");
|
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")
|
dir.join("service.log")
|
||||||
}
|
}
|
||||||
|
|
||||||
fn host_log_path() -> PathBuf {
|
fn host_log_path() -> PathBuf {
|
||||||
let dir = crate::gamestream::config_dir().join("logs");
|
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")
|
dir.join("host.log")
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -684,7 +686,9 @@ fn ensure_default_host_env() -> Result<()> {
|
|||||||
return Ok(());
|
return Ok(());
|
||||||
}
|
}
|
||||||
if let Some(dir) = path.parent() {
|
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\
|
let default = "# punktfunk host configuration (read by the Windows service).\n\
|
||||||
# KEY=VALUE per line; '#' comments. Restart the service after editing:\n\
|
# KEY=VALUE per line; '#' comments. Restart the service after editing:\n\
|
||||||
@@ -707,7 +711,11 @@ fn ensure_default_host_env() -> Result<()> {
|
|||||||
\n\
|
\n\
|
||||||
# Force a specific render GPU by name substring (multi-GPU boxes only):\n\
|
# Force a specific render GPU by name substring (multi-GPU boxes only):\n\
|
||||||
# PUNKTFUNK_RENDER_ADAPTER=4090\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());
|
println!("Wrote default config: {}", path.display());
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user