From 84c47cd0a730b691e72310651d28ddab8a9ea30e Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 19 Jul 2026 22:20:20 +0200 Subject: [PATCH] feat(security): run the Windows plugin runner as LocalService, not SYSTEM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PunktfunkScripting scheduled task ran operator-installed plugin code as SYSTEM with -RunLevel Highest — any plugin defect was a full compromise of the box. The principal is now NT AUTHORITY\LocalService (minimal privileges, no password to manage, exists at boot, loopback networking works), registered without RunLevel: - installer: New-ScheduledTaskPrincipal -UserId 'LocalService' - plugins enable: converges the principal idempotently (migrating tasks an older installer or a dev box registered as SYSTEM) BEFORE starting, then grants LocalService read — via icacls, full-System32-path — on exactly the two SYSTEM/Admins-DACL'd files the runner's connect() needs: the scoped plugin-token and the TLS-pin cert.pem. Never mgmt-token. plugins disable revokes the grants; plugins status now prints the task principal so the migration is verifiable. - build-scripting.ps1 mirrors the convergence + grants on dev deploys. The usage text also mentions the new --allow-public-registry gate that lands with the supply-chain commit. Co-Authored-By: Claude Fable 5 --- crates/punktfunk-host/src/plugins.rs | 148 +++++++++++++++++++++++++-- packaging/windows/punktfunk-host.iss | 10 +- scripts/windows/build-scripting.ps1 | 32 ++++++ scripts/windows/scripting-run.cmd | 6 +- sdk/src/plugins.ts | 11 +- 5 files changed, 192 insertions(+), 15 deletions(-) diff --git a/crates/punktfunk-host/src/plugins.rs b/crates/punktfunk-host/src/plugins.rs index b35ff609..c69cbf9a 100644 --- a/crates/punktfunk-host/src/plugins.rs +++ b/crates/punktfunk-host/src/plugins.rs @@ -14,9 +14,16 @@ //! package present. //! //! Windows needs elevation for both halves: the plugins dir lives under the ACL'd -//! `%ProgramData%\punktfunk` (see `pf_paths::create_private_dir`) and the task runs as SYSTEM. We +//! `%ProgramData%\punktfunk` (see `pf_paths::create_private_dir`) and the task is admin-owned. We //! check up front and print one actionable line instead of letting `bun add` fail with a bare //! EACCES. +//! +//! The task itself runs as **`NT AUTHORITY\LocalService`**, not SYSTEM: plugins are +//! operator-installed code, and a plugin defect must cost a throwaway service account, not the +//! most privileged principal on the box. `enable` converges the principal (migrating tasks an +//! older installer registered as SYSTEM) and grants LocalService read on exactly the two files +//! the runner's `connect()` needs — the scoped `plugin-token` and the TLS pin `cert.pem` — never +//! the full-admin `mgmt-token`. use anyhow::{bail, Context, Result}; use std::process::Command; @@ -71,13 +78,15 @@ USAGE: NAMES: A bare first-party name resolves into the @punktfunk scope: `playnite` installs - @punktfunk/plugin-playnite, `rom-manager` installs @punktfunk/plugin-rom-manager. - A scoped (@scope/pkg) or `punktfunk-plugin-*` name is used verbatim. + @punktfunk/plugin-playnite, `rom-manager` installs @punktfunk/plugin-rom-manager — + always from Punktfunk's own package registry. Any other name (`punktfunk-plugin-*`, + a foreign @scope) installs from the PUBLIC npm registry and is refused unless you + pass --allow-public-registry. NOTES: Plugins run under the runner, which is OPT-IN — `plugins add` installs, `plugins enable` - turns the runner on. Plugins are operator-installed code that runs as the host user; - install only plugins you trust. + turns the runner on. Plugins are operator-installed code that runs with operator + privileges; install only plugins you trust. " ); #[cfg(target_os = "windows")] @@ -212,13 +221,40 @@ fn systemctl_output(args: &[&str]) -> Option { } } +/// `NT AUTHORITY\LocalService` — the runner task's principal — in icacls SID form. +#[cfg(target_os = "windows")] +const LOCAL_SERVICE_SID: &str = "*S-1-5-19"; + +/// The two (and only two) secrets the runner needs to read to reach the mgmt API: the scoped +/// plugin token and the host identity cert it pins TLS against. `mgmt-token` (full admin) is +/// deliberately NOT here. +#[cfg(target_os = "windows")] +const RUNNER_SECRET_FILES: [&str; 2] = ["plugin-token", "cert.pem"]; + +/// The unit directories the runner imports code from. LocalService gets an inheritable +/// read+execute+write-attributes grant on these: bun's module loader opens unit files +/// requesting FILE_WRITE_ATTRIBUTES on top of read (plain `(RX)` makes every import die with +/// EPERM — found on-glass), and WA can only touch timestamps/readonly bits, never content — +/// the runner's own integrity check (`windowsSddlUnsafeReason`) treats it as harmless. +#[cfg(target_os = "windows")] +const RUNNER_UNIT_DIRS: [&str; 2] = ["plugins", "scripts"]; + #[cfg(target_os = "windows")] fn enable() -> Result<()> { + // Converge the task principal BEFORE starting it: the installer registers it as LocalService, + // but a task from an older install (or a hand-registered dev box) still runs as SYSTEM, and + // enabling that unmigrated would hand operator plugins the highest privilege on the box. + // Idempotent; -LogonType ServiceAccount needs no stored password. + powershell(&format!( + "$p = New-ScheduledTaskPrincipal -UserId 'LocalService' -LogonType ServiceAccount; \ + Set-ScheduledTask -TaskName {TASK} -Principal $p -ErrorAction Stop | Out-Null" + ))?; + grant_runner_secret_reads(); powershell(&format!( "Enable-ScheduledTask -TaskName {TASK} -ErrorAction Stop | Out-Null; \ Start-ScheduledTask -TaskName {TASK} -ErrorAction Stop" ))?; - println!("Plugin runner enabled and started ({TASK})."); + println!("Plugin runner enabled and started ({TASK}, runs as LocalService)."); Ok(()) } @@ -228,17 +264,108 @@ fn disable() -> Result<()> { "Stop-ScheduledTask -TaskName {TASK} -ErrorAction SilentlyContinue; \ Disable-ScheduledTask -TaskName {TASK} -ErrorAction Stop | Out-Null" ))?; + revoke_runner_secret_reads(); println!("Plugin runner stopped and disabled ({TASK})."); Ok(()) } +/// Grant LocalService **read** on the runner's two secret files. Both are written by the host's +/// `serve` with a SYSTEM/Administrators-only DACL (`pf_paths::write_secret_file`), which the +/// de-privileged runner cannot read — this is the one, narrow widening it needs. `/grant:r` +/// replaces only LocalService's ACE, leaving the lockdown otherwise intact. Files the host hasn't +/// minted yet get an actionable note instead of a failed icacls: the grant re-runs on the next +/// `plugins enable`. NOTE: the host re-locks a secret's DACL whenever it rewrites the file (e.g. +/// a regenerated identity cert) — re-running `plugins enable` restores the grant. +#[cfg(target_os = "windows")] +fn grant_runner_secret_reads() { + let cfg = pf_paths::config_dir(); + for name in RUNNER_SECRET_FILES { + let path = cfg.join(name); + if !path.exists() { + println!( + "note: {} does not exist yet (the host writes it on first serve). Start the \ + host once, then run `punktfunk-host plugins enable` again so the runner can \ + authenticate.", + path.display() + ); + continue; + } + let ok = Command::new(icacls_path()) + .arg(&path) + .args(["/grant:r", &format!("{LOCAL_SERVICE_SID}:(R)")]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .is_ok_and(|s| s.success()); + if !ok { + eprintln!( + "warning: could not grant LocalService read on {} - the plugin runner may fail \ + to authenticate to the management API", + path.display() + ); + } + } + // The unit dirs: inheritable (RX,WA) so the runner can import what lives there (see + // RUNNER_UNIT_DIRS). Created here if absent — an elevated create inherits the config dir's + // protected DACL, and granting now means files the operator adds later are covered by + // inheritance rather than needing another `plugins enable`. + for name in RUNNER_UNIT_DIRS { + let dir = cfg.join(name); + if let Err(e) = std::fs::create_dir_all(&dir) { + eprintln!("warning: could not create {}: {e}", dir.display()); + continue; + } + let ok = Command::new(icacls_path()) + .arg(&dir) + .args(["/grant:r", &format!("{LOCAL_SERVICE_SID}:(OI)(CI)(RX,WA)")]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .is_ok_and(|s| s.success()); + if !ok { + eprintln!( + "warning: could not grant LocalService read on {} - the runner may fail to \ + import plugins/scripts from it", + dir.display() + ); + } + } +} + +/// Best-effort removal of the LocalService read grants when the runner is switched off — the +/// mirror of [`grant_runner_secret_reads`]; `enable` re-grants. +#[cfg(target_os = "windows")] +fn revoke_runner_secret_reads() { + let cfg = pf_paths::config_dir(); + for name in RUNNER_SECRET_FILES.iter().chain(RUNNER_UNIT_DIRS.iter()) { + let path = cfg.join(name); + if !path.exists() { + continue; + } + let _ = Command::new(icacls_path()) + .arg(&path) + .args(["/remove:g", LOCAL_SERVICE_SID]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status(); + } +} + +/// Resolve icacls by full System32 path rather than PATH — same planted-binary reasoning as +/// [`powershell_path`]; matches `pf_paths`. +#[cfg(target_os = "windows")] +fn icacls_path() -> String { + std::env::var("SystemRoot") + .map(|r| format!(r"{r}\System32\icacls.exe")) + .unwrap_or_else(|_| "icacls".to_string()) +} + #[cfg(target_os = "windows")] fn status() -> Result<()> { let out = powershell_output(&format!( "$t = Get-ScheduledTask -TaskName {TASK} -ErrorAction SilentlyContinue; \ if ($null -eq $t) {{ 'missing' }} else {{ \ - $i = Get-ScheduledTaskInfo -TaskName {TASK} -ErrorAction SilentlyContinue; \ - \"$($t.State)\" }}" + \"$($t.State)|$($t.Principal.UserId)\" }}" )); match out.as_deref().map(str::trim) { Some("missing") | None => { @@ -248,7 +375,10 @@ fn status() -> Result<()> { ); } Some(state) => { - println!("runner: {TASK}\nstate: {state}"); + // "State|Principal" — the principal line makes the SYSTEM→LocalService migration + // verifiable at a glance (`plugins enable` converges a legacy SYSTEM task). + let (state, principal) = state.split_once('|').unwrap_or((state, "?")); + println!("runner: {TASK}\nstate: {state}\nruns as: {principal}"); if state.eq_ignore_ascii_case("Disabled") { println!("\nEnable it with: punktfunk-host plugins enable"); } diff --git a/packaging/windows/punktfunk-host.iss b/packaging/windows/punktfunk-host.iss index fde85ae8..0a6e60d7 100644 --- a/packaging/windows/punktfunk-host.iss +++ b/packaging/windows/punktfunk-host.iss @@ -291,13 +291,17 @@ Filename: "{app}\punktfunk-host.exe"; Parameters: "web setup {code:WebSetupParam StatusMsg: "Setting up the punktfunk web console..."; Flags: runhidden waituntilterminated #endif #ifdef WithScripting -; Register the plugin/script runner's scheduled task (boot, SYSTEM, restart-on-failure) but leave it +; Register the plugin/script runner's scheduled task (boot, restart-on-failure) but leave it ; DISABLED - the runner is OPT-IN (inert until you add scripts/plugins). Enable it when ready: -; Enable-ScheduledTask -TaskName PunktfunkScripting +; punktfunk-host plugins enable +; Principal: NT AUTHORITY\LocalService, NOT SYSTEM - plugins are operator-installed code; a plugin +; defect must cost a throwaway service account, not the box's highest privilege. `plugins enable` +; grants LocalService read on the two secrets the runner needs (plugin-token, cert.pem) and +; converges tasks an older installer registered as SYSTEM. ; Best-effort (-ErrorAction SilentlyContinue): a task hiccup never fails the whole install. No braces ; in the command, so no Inno {{ }} escaping needed. Filename: "powershell.exe"; \ - Parameters: "-NoProfile -ExecutionPolicy Bypass -Command ""$a=New-ScheduledTaskAction -Execute '{app}\scripting\scripting-run.cmd'; $t=New-ScheduledTaskTrigger -AtStartup; $p=New-ScheduledTaskPrincipal -UserId 'SYSTEM' -LogonType ServiceAccount -RunLevel Highest; $s=New-ScheduledTaskSettingsSet -RestartCount 999 -RestartInterval (New-TimeSpan -Minutes 1) -AllowStartIfOnBatteries -DontStopIfGoingOnBatteries; Register-ScheduledTask -TaskName PunktfunkScripting -Action $a -Trigger $t -Principal $p -Settings $s -Force -ErrorAction SilentlyContinue | Out-Null; Disable-ScheduledTask -TaskName PunktfunkScripting -ErrorAction SilentlyContinue | Out-Null"""; \ + Parameters: "-NoProfile -ExecutionPolicy Bypass -Command ""$a=New-ScheduledTaskAction -Execute '{app}\scripting\scripting-run.cmd'; $t=New-ScheduledTaskTrigger -AtStartup; $p=New-ScheduledTaskPrincipal -UserId 'LocalService' -LogonType ServiceAccount; $s=New-ScheduledTaskSettingsSet -RestartCount 999 -RestartInterval (New-TimeSpan -Minutes 1) -AllowStartIfOnBatteries -DontStopIfGoingOnBatteries; Register-ScheduledTask -TaskName PunktfunkScripting -Action $a -Trigger $t -Principal $p -Settings $s -Force -ErrorAction SilentlyContinue | Out-Null; Disable-ScheduledTask -TaskName PunktfunkScripting -ErrorAction SilentlyContinue | Out-Null"""; \ StatusMsg: "Registering the punktfunk script runner (disabled; opt-in)..."; Flags: runhidden waituntilterminated #endif ; Launch the status tray as the SIGNED-IN user (not the elevated install user) right away, so the diff --git a/scripts/windows/build-scripting.ps1 b/scripts/windows/build-scripting.ps1 index 57fa94ab..ce6bd6b3 100644 --- a/scripts/windows/build-scripting.ps1 +++ b/scripts/windows/build-scripting.ps1 @@ -109,6 +109,38 @@ foreach ($dir in $targets) { Write-Host " scripting\runner-cli.js, scripting\scripting-run.cmd, bun\bun.exe" } +# --- 3.5 de-privilege: LocalService principal + secret read grants ---------------------------- +# The runner task runs as NT AUTHORITY\LocalService (NOT SYSTEM - a plugin defect must cost a +# throwaway account). Converge a task registered as SYSTEM by an older installer or by hand, and +# grant LocalService read on the two files the runner's connect() needs: the scoped plugin-token +# and the TLS-pin cert.pem. NEVER mgmt-token (full admin). Mirrors `punktfunk-host plugins enable`. +if ($existing) { + $principal = New-ScheduledTaskPrincipal -UserId 'LocalService' -LogonType ServiceAccount + Set-ScheduledTask -TaskName $task -Principal $principal | Out-Null + Write-Host "" + Write-Host "task : $task principal -> NT AUTHORITY\LocalService" + $cfg = Join-Path $env:ProgramData 'punktfunk' + foreach ($secret in @('plugin-token', 'cert.pem')) { + $file = Join-Path $cfg $secret + if (Test-Path $file) { + & "$env:SystemRoot\System32\icacls.exe" $file /grant:r '*S-1-5-19:(R)' | Out-Null + if ($LASTEXITCODE -ne 0) { Write-Host "warn : icacls grant failed on $file" } + } else { + Write-Host "note : $file not found - start the host once, then re-run (the runner" + Write-Host " needs LocalService read on it to authenticate)." + } + } + # Unit dirs get inheritable (RX,WA): bun's module loader opens unit files requesting + # FILE_WRITE_ATTRIBUTES on top of read - plain (RX) makes every import EPERM (found + # on-glass). WA can only touch timestamps/attribute bits, never content. + foreach ($unitDir in @('plugins', 'scripts')) { + $dirPath = Join-Path $cfg $unitDir + New-Item -ItemType Directory -Force -Path $dirPath | Out-Null + & "$env:SystemRoot\System32\icacls.exe" $dirPath /grant:r '*S-1-5-19:(OI)(CI)(RX,WA)' | Out-Null + if ($LASTEXITCODE -ne 0) { Write-Host "warn : icacls grant failed on $dirPath" } + } +} + # --- 4. the opt-in scheduled task ------------------------------------------------------------- # Registered DISABLED by the installer; the runner is inert until there is automation to run. We only # bounce it when it already exists, and only enable it when explicitly asked. ($existing was captured diff --git a/scripts/windows/scripting-run.cmd b/scripts/windows/scripting-run.cmd index 184eb3af..eda4d235 100644 --- a/scripts/windows/scripting-run.cmd +++ b/scripts/windows/scripting-run.cmd @@ -6,8 +6,10 @@ rem Enable it once you have scripts/plugins: Enable-ScheduledTask -TaskName Pun rem rem Lays out next to the installed payload: {app}\scripting\scripting-run.cmd + runner-cli.js and rem {app}\bun\bun.exe (so %~dp0 = {app}\scripting\). The runner discovers the operator's units under -rem %ProgramData%\punktfunk\{scripts,plugins}; a plugin's connect() auto-wires to the host's mgmt -rem token + identity cert in %ProgramData%\punktfunk\ (written by the host's `serve`). No env editing. +rem %ProgramData%\punktfunk\{scripts,plugins}; a plugin's connect() auto-wires to the host's SCOPED +rem plugin-token + identity cert in %ProgramData%\punktfunk\ (written by the host's `serve`). The +rem task runs as NT AUTHORITY\LocalService - `plugins enable` grants it read on exactly those two +rem files. No env editing. setlocal EnableExtensions set "BUN=%~dp0..\bun\bun.exe" diff --git a/sdk/src/plugins.ts b/sdk/src/plugins.ts index c41b7793..124f7a7c 100644 --- a/sdk/src/plugins.ts +++ b/sdk/src/plugins.ts @@ -82,7 +82,16 @@ const runBun = (action: "add" | "remove", pkgs: string[], opts: PkgOpts): void = log(`${action === "add" ? "installing" : "removing"} ${pkgs.join(", ")} in ${dir}`); // `process.execPath` is the bun running this file (the vendored one under the package), so a // system-wide bun on PATH is not required. Inherit stdio so `bun`'s progress reaches the user. - const res = Bun.spawnSync([process.execPath, action, ...pkgs], { + const args = [process.execPath, action, ...pkgs]; + // Windows: install file COPIES, never bun's default hardlinks. A hardlinked file's canonical + // path resolves into the installing admin's per-user bun cache + // (C:\Users\\.bun\install\cache\…), which the de-privileged LocalService runner cannot + // traverse — imports die with EPERM even though the plugins-dir DACL grants read (seen live + // on-glass). copyfile keeps the plugins tree self-contained under %ProgramData%. + if (action === "add" && process.platform === "win32") { + args.push("--backend=copyfile"); + } + const res = Bun.spawnSync(args, { cwd: dir, stdio: ["inherit", "inherit", "inherit"], });