diff --git a/CHANGELOG.md b/CHANGELOG.md index 1febeaf..7f33229 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # ShellKnight Changelog +## [v2026.09.26.001] - 2026-09-26 + +- **State directory hardened against local privilege escalation (critical):** `C:\ProgramData\ShellKnight` holds `config.json` (read at startup), `run.ps1` (the native `ShellKnight` scheduled task executes it as SYSTEM every 8 hours) and the `Logs`, `JSON` and `Intel` folders. By default ProgramData lets `BUILTIN\Users` create files and folders in its subfolders, and `CREATOR OWNER` gets full control of what they create. So on an endpoint where ShellKnight had never run, a standard user could pre-create `C:\ProgramData\ShellKnight` (or `run.ps1`, or `config.json`) and own it. Owning `run.ps1` lets them choose the code SYSTEM runs on the next scheduled run - a local privilege escalation. Owning `config.json` lets them set `BattlefieldURL` and `BattlefieldApiKey`, redirecting the run report and the tenant API key to a URL of their own. Once SYSTEM has created a file a user cannot modify it, but if the user owns the folder they can still delete and replace its files. +- **What changed:** before `config.json` is read or the scheduled task is trusted, a new guard (running during config load, ahead of `Initialize-Logging`) creates or repairs the folder with an explicit ACL and re-checks the sensitive files: + - **Explicit ACL, set by SID:** `SYSTEM` (`S-1-5-18`) and `Administrators` (`S-1-5-32-544`) full control, `Users` (`S-1-5-32-545`) read and execute, all inheritable to files and subfolders; owner set to Administrators; inheritance removed (`icacls /inheritance:r`) so ProgramData's `Users`-create ACEs no longer apply. SIDs are used throughout, never localized names, because the built-in groups are `Administratoren` / `Benutzer` on a German box but the SIDs are the same everywhere. Removing the parent's inherited ACEs and setting new inheritable ones propagates to existing `Logs` / `JSON` / `Intel` children that still inherit, so no recursive `/T` sweep runs on every check-in. + - **A folder a user already owns is rebuilt:** if the folder exists but `SYSTEM` or `Administrators` does not own it, it is removed with its contents and recreated. A user-owned tree has no run history worth keeping and may carry access-control entries the guard cannot enumerate. + - **`config.json` and `run.ps1` are owner-checked:** either one not owned by `SYSTEM` or `Administrators` is deleted, so a planted `config.json` is never read and a planted `run.ps1` is never executed. `run.ps1` is rewritten by the self-schedule block later in the run; a removed `config.json` means built-in defaults and environment variables stand in for that run. +- **Model:** owner is read with `(Get-Acl -LiteralPath ...).GetOwner([System.Security.Principal.SecurityIdentifier]).Value`, the same non-throwing pattern the Intel cache trust check uses (v2026.09.25.004); an owner that cannot be read is treated as untrusted. The whole guard is wrapped so a hardening failure logs a warning and never stops the run. +- **New payload fields:** the report's `health` object gains `state_dir_repaired` (the folder was found user-owned and rebuilt) and `state_dir_files_removed` (count of untrusted `config.json` / `run.ps1` deleted, 0/1/2), so Battlefield can flag an endpoint that showed signs of a local tampering attempt. Battlefield stores the whole report (ADR 0002), so it accepts the fields unchanged; nothing displays them yet. +- **Regression test:** new `tests/Test-StateDirGuard.ps1` runs the extracted guard verbatim under `Set-StrictMode -Version 2`, with `Get-Acl` and `icacls` mocked and real temp directories for the filesystem operations. It covers a fresh box (folder created), the steady state (trusted owner, folder and its contents kept), a user-owned folder (removed and recreated), an unreadable owner (treated as untrusted), and trusted vs. user-owned vs. unknown-owner `config.json` and `run.ps1`. It asserts the `icacls` arguments carry the three SIDs with the `*` prefix and no localized principal, that `/inheritance:r` and `/setowner` are issued, and - from the script's own source order - that the guard runs before the `config.json` read. It does not replace a real Windows run: `Get-Acl`, `icacls` and NTFS inheritance are Windows behaviours this test mocks. +- **Not yet run on real Windows.** The ACL, the ownership reclaim and the owner checks need one real SYSTEM run on a Windows 10/11 endpoint before this reaches `main`, per the repo rule. + ## [v2026.09.25.004] - 2026-09-25 - **The Intel Engine loads threat intel for the first time (critical):** since v1.002 the engine's `Invoke-SafeBlock` read `$Script:Config.IntelEngine_PrimarySource`, which `$Script:Config` did not have; only `$SK_IntelEngine_PrimarySource` existed. Under `Set-StrictMode -Version 2` that threw in the `$consolidated` literal, before any download, cache write or `IntelSource`, and with no cache written the next run took the same path. **Every device on every run reported `intel_source: "Hardcoded fallback"` and 0 hash, filename and C2 IOCs** (Battlefield backtest, 2026-07-03 to 2026-09-25), so the detection engines ran on their hard-coded lists only. The only trace was one INFO line in the log: `Intel Engine skipped - The property 'IntelEngine_PrimarySource' cannot be found on this object. Verify that the property exists.` The property is now in `$Script:Config`. diff --git a/ShellKnight.ps1 b/ShellKnight.ps1 index ec25434..e19c6d2 100644 --- a/ShellKnight.ps1 +++ b/ShellKnight.ps1 @@ -2,7 +2,7 @@ #Requires -RunAsAdministrator <# .SYNOPSIS - ShellKnight v2026.09.25.004 - Enterprise Endpoint Security & Remediation Tool + ShellKnight v2026.09.26.001 - Enterprise Endpoint Security & Remediation Tool .DESCRIPTION Automated endpoint security remediation, threat detection, hardening, and @@ -18,9 +18,9 @@ C. David Burgess - PTech LLC .VERSION - Version : v2026.09.25.004 - Released : 2026-09-25 - Prior : v2026.09.25.003 + Version : v2026.09.26.001 + Released : 2026-09-26 + Prior : v2026.09.25.004 .ENGINES Phase 1 - Intel Engine : Threat intelligence download and cache @@ -33,6 +33,27 @@ Phase 8 - Reporting Engine : Reporting, trending, and extended checks .CHANGELOG + v2026.09.26.001 - Harden C:\ProgramData\ShellKnight against local privilege + escalation. By default ProgramData lets BUILTIN\Users create files + and folders in its subfolders, and CREATOR OWNER gets full control + of what they create. On a box where ShellKnight had never run, a + standard user could pre-create C:\ProgramData\ShellKnight (or + run.ps1 / config.json) and own it, then choose the code the SYSTEM + scheduled task runs, or redirect the run report and its API key + through config.json. Now, before config.json is read or the task is + trusted, the folder is created or repaired with an explicit ACL - + SYSTEM and Administrators full control, Users read only, inheritance + removed so the ProgramData Users-create ACEs are gone - and a folder + a user already owns is removed and recreated. Any config.json or + run.ps1 not owned by SYSTEM or Administrators is deleted, not read + or executed. Owner is checked with (Get-Acl).GetOwner, the same + model as the Intel cache check (v2026.09.25.004); the ACL is set + with icacls by SID (S-1-5-18 / S-1-5-32-544 / S-1-5-32-545), never + by localized name. The report's health object gains + state_dir_repaired and state_dir_files_removed so the dashboard can + flag a box that showed signs of tampering. New tests/Test- + StateDirGuard.ps1. NOT yet run on real Windows - the ACL and owner + checks need one real SYSTEM run before this reaches main. v2026.09.25.004 - The Intel Engine loads threat intel for the first time, and every intel match is REPORT-ONLY. Since v1.002 the engine read $Script:Config.IntelEngine_PrimarySource, which Config did @@ -346,7 +367,7 @@ architecture (all already in MachineInfo, just weren't exported). Populates the dashboard detail panel per host. v2026.07.03.009 - Event 7045 whitelist: ESET PROTECT agent updater path - (eset\RemoteAdministrator) - field FP across St. Michael site + (eset\RemoteAdministrator) - field FP across Customer B site 2026-07-03, first FP caught via Battlefield ingest. Pattern is regex-escaped (\\) since the path list is matched with -match. v2026.07.03.008 - Battlefield config via environment variables. The Datto @@ -362,7 +383,7 @@ component once the HTTPS endpoint is confirmed reachable. Push failure is logged as WARN and never affects the run/exit code; the on-disk JSON remains the source of truth. - v2026.07.03.006 - Field batch from PCH-DT-CJP2ZC3 test run. + v2026.07.03.006 - Field batch from HOST-E1 test run. BUG FIX: Get-ProfileScan sizes came back ~0 GB - AllDirectories enumeration throws on the first access-denied directory (every profile has denied junctions) and aborted the walk, also @@ -410,14 +431,14 @@ (aggregate), pending updates, CIS password length. JSON: new 'findings' array + 'failed_actions' count; ConvertTo-Json now -Depth 4. - v2026.07.03.001 - Field FP batch + SC removal safety (RAS1 2026-06-02, - HOPECENTER 2026-06-24 runs). + v2026.07.03.001 - Field FP batch + SC removal safety (HOST-D1 2026-06-02, + HOST-C1 2026-06-24 runs). Event 7045 whitelist: CentraStage svc/path (Datto RMM + bundled UltraVNC), HitmanPro, Silver Bullet Technology, PaniniUSB. New config: SK_Svc7045_ExtraNames / SK_Svc7045_ExtraPaths for per-deployment whitelist additions without editing engine code. SC auto-removal now DEFAULT OFF (detect+report only) after it - half-deleted our own managed instance on HOPECENTER; managed + half-deleted our own managed instance on HOST-C1; managed instance ID 32f7367870097776 now shipped in config. SC delete path guard: >=3 segments deep + leaf must contain 'screenconnect' before any Remove-Item -Recurse fires. @@ -535,7 +556,7 @@ param() # ============================================================================== -# SHELLKNIGHT v2026.09.25.004 CONFIGURATION +# SHELLKNIGHT v2026.09.26.001 CONFIGURATION # All settings are configured here. No external config files required. # Each engine can be independently enabled or disabled. # ============================================================================== @@ -625,7 +646,7 @@ $SK_RemoteAccessWarnUnknown = $true # WARN on remote tools not in Add/Re $SK_ScreenConnect_InstanceID = '32f7367870097776' # Your managed ScreenConnect instance ID $SK_RemoveRogueScreenConnect = $false # Auto-remove non-managed ScreenConnect instances # DEFAULT OFF: detect + report only. Field incident - # 2026-06-24 (HOPECENTER): auto-removal fired on our + # 2026-06-24 (HOST-C1): auto-removal fired on our # own SC instance and half-deleted it (locked DLL). $SK_Svc7045_ExtraNames = @() # Per-deployment additions to the Event 7045 service-name whitelist $SK_Svc7045_ExtraPaths = @() # Per-deployment additions to the Event 7045 service-path whitelist (substring match) @@ -689,6 +710,162 @@ $Script:ConfigPath = 'C:\ProgramData\ShellKnight\config.json' # engine can recognise (and never remove) our own persistence task. $Script:SelfLauncherPath = Join-Path (Split-Path $Script:ConfigPath -Parent) 'run.ps1' +# ============================================================================== +# STATE DIRECTORY HARDENING - runs before anything on disk is trusted +# ============================================================================== +# C:\ProgramData\ShellKnight holds config.json (read just below), run.ps1 (the +# native scheduled task executes it as SYSTEM every 8 h) and the Logs, JSON and +# Intel folders. By default ProgramData lets BUILTIN\Users create files and +# folders in its subfolders, and CREATOR OWNER gets full control of what they +# create. So on a box where ShellKnight has never run, a standard user can +# pre-create this folder (or run.ps1 / config.json) and own it. Owning run.ps1 +# lets them choose the code SYSTEM runs (local privilege escalation); owning +# config.json lets them point the run report and its API key at a URL of their +# own. Once SYSTEM has created a file a user cannot modify it, but if the user +# owns the folder they can still delete and replace its files. +# +# So, before the config below is read: create or repair the folder with an +# explicit ACL (SYSTEM and Administrators full control, Users read only, no +# inherited create rights), and delete any config.json or run.ps1 that SYSTEM +# or Administrators does not own. This mirrors the Intel cache owner check +# (v2026.09.25.004) and uses SIDs, never localized names, throughout - the +# groups are 'Administratoren' / 'Benutzer' on a German box but the SIDs are the +# same everywhere. It runs before Initialize-Logging, so it cannot Log-*; notes +# are buffered in $Script:StateGuardNotes and flushed to the log once it is open. +$Script:StateDir = Split-Path $Script:ConfigPath -Parent +$Script:StateGuardNotes = New-Object 'System.Collections.Generic.List[string]' +$Script:SID_System = 'S-1-5-18' # NT AUTHORITY\SYSTEM +$Script:SID_Administrators = 'S-1-5-32-544' # BUILTIN\Administrators +$Script:SID_Users = 'S-1-5-32-545' # BUILTIN\Users +$Script:TrustedOwnerSids = @($Script:SID_System, $Script:SID_Administrators) + +function Add-StateGuardNote { + param([string]$Message) + $Script:StateGuardNotes.Add($Message) + Write-Host " [state-guard] $Message" -ForegroundColor Yellow +} + +function Get-OwnerSid { + # The owning SID of a file or folder, or $null if the path does not exist or + # the owner cannot be read. A null owner is treated as untrusted by callers. + param([string]$Path) + if (-not (Test-Path -LiteralPath $Path)) { return $null } + try { + (Get-Acl -LiteralPath $Path -ErrorAction Stop).GetOwner( + [System.Security.Principal.SecurityIdentifier]).Value + } catch { $null } +} + +function Test-TrustedOwner { + # $true only if the path exists and SYSTEM or Administrators owns it. + # ProgramData lets any local user create and own a file here, so any other + # owner - or one that cannot be read - is untrusted. + param([string]$Path) + (Get-OwnerSid -Path $Path) -in $Script:TrustedOwnerSids +} + +function Invoke-Icacls { + # Thin wrapper so callers (and the test) drive icacls uniformly. Returns the + # exit code; 0 is success. Output is swallowed - the owner re-check and the + # run report are the record, not icacls' chatter. + param([Parameter(Mandatory = $true)][string[]]$Arguments) + $null = & icacls.exe @Arguments 2>&1 + $LASTEXITCODE +} + +function Set-StateDirAcl { + # Replace the folder's ACL with exactly SYSTEM and Administrators full + # control, Users read and execute, all inheritable to files and subfolders; + # owner Administrators; inheritance removed so ProgramData's Users-create + # ACEs are gone. Removing the parent's inherited ACEs and setting new + # inheritable ones propagates to existing Logs/JSON/Intel children that still + # inherit, so no /T sweep is needed on every run. SIDs take the '*' prefix + # icacls requires; -f formatting keeps the ':' out of string parsing. + param([string]$Path) + $sys = '*' + $Script:SID_System + $adm = '*' + $Script:SID_Administrators + $usr = '*' + $Script:SID_Users + $null = Invoke-Icacls @($Path, '/inheritance:r', '/Q', '/C') + $rc = Invoke-Icacls @($Path, '/grant:r', + ('{0}:(OI)(CI)F' -f $sys), + ('{0}:(OI)(CI)F' -f $adm), + ('{0}:(OI)(CI)RX' -f $usr), + '/Q', '/C') + $null = Invoke-Icacls @($Path, '/setowner', $adm, '/Q', '/C') + $rc -eq 0 +} + +function Protect-StateDirectory { + # Create the state folder if missing; if it exists but SYSTEM or + # Administrators does not own it (a user pre-created it), remove it and its + # contents and recreate it - a user-owned tree has no run history worth + # keeping and may hold ACEs we cannot enumerate. Then stamp the explicit ACL + # either way. Returns a result object; never throws. + param([string]$Path) + $result = [ordered]@{ + Existed = $false; OwnerSid = $null; OwnerTrusted = $false + Recreated = $false; Created = $false; AclApplied = $false + } + try { + $result.Existed = Test-Path -LiteralPath $Path + if ($result.Existed) { + $result.OwnerSid = Get-OwnerSid -Path $Path + $result.OwnerTrusted = $result.OwnerSid -in $Script:TrustedOwnerSids + if (-not $result.OwnerTrusted) { + $who = if ($result.OwnerSid) { $result.OwnerSid } else { 'an unknown account' } + Add-StateGuardNote "folder owned by $who, not SYSTEM or Administrators: removing and recreating it" + Remove-Item -LiteralPath $Path -Recurse -Force -ErrorAction Stop + $result.Existed = $false + $result.Recreated = $true + } + } + if (-not $result.Existed) { + $null = New-Item -ItemType Directory -Path $Path -Force -ErrorAction Stop + $result.Created = $true + } + $result.AclApplied = Set-StateDirAcl -Path $Path + if (-not $result.AclApplied) { + Add-StateGuardNote "could not fully apply the hardened ACL to $Path (icacls returned non-zero)" + } + } catch { + Add-StateGuardNote "hardening error on $Path : $($_.Exception.Message)" + } + [pscustomobject]$result +} + +function Remove-UntrustedStateFile { + # Delete a state file (config.json or run.ps1) unless SYSTEM or + # Administrators owns it. A user-owned config.json would redirect the report + # and its API key; a user-owned run.ps1 is code SYSTEM would execute. Once + # the folder ACL above is in place a user cannot create these, but on the + # first hardened run one may already be there. Returns $true if it removed + # one. The self-schedule block rewrites run.ps1; a removed config.json means + # built-in defaults and env vars stand in for this run. + param([string]$Path, [string]$Label) + if (-not (Test-Path -LiteralPath $Path)) { return $false } + if (Test-TrustedOwner -Path $Path) { return $false } + $owner = Get-OwnerSid -Path $Path + $who = if ($owner) { $owner } else { 'an unknown account' } + Add-StateGuardNote "$Label owned by $who, not SYSTEM or Administrators: deleting it" + Remove-Item -LiteralPath $Path -Force -ErrorAction SilentlyContinue + $true +} + +# Harden the folder, then drop any pre-planted config.json / run.ps1 - all +# before the config below is read or the scheduled task (which runs run.ps1) is +# trusted. Guarded so a hardening failure never stops the run. +$Script:StateDirGuard = [pscustomobject]@{ Existed = $false; OwnerSid = $null; OwnerTrusted = $false; Recreated = $false; Created = $false; AclApplied = $false } +$Script:StateGuardFilesRemoved = 0 +try { + $Script:StateDirGuard = Protect-StateDirectory -Path $Script:StateDir + $Script:StateGuardFilesRemoved = 0 + if (Remove-UntrustedStateFile -Path $Script:ConfigPath -Label 'config.json') { $Script:StateGuardFilesRemoved++ } + if (Remove-UntrustedStateFile -Path $Script:SelfLauncherPath -Label 'run.ps1') { $Script:StateGuardFilesRemoved++ } +} catch { + $Script:StateGuardFilesRemoved = 0 + Add-StateGuardNote "state hardening skipped: $($_.Exception.Message)" +} + # Load persisted config FIRST (scheduled runs rely on this); env overrides win after. if (Test-Path $Script:ConfigPath) { try { @@ -731,7 +908,7 @@ try { # Runtime Config Object - single source of truth for all engines $Script:Config = [PSCustomObject]@{ - Version = 'v2026.09.25.004' + Version = 'v2026.09.26.001' # Intel Engine IntelEngine_Enabled = $SK_IntelEngine_Enabled IntelEngine_CheckUpdates = $SK_IntelEngine_CheckForUpdates @@ -1134,7 +1311,7 @@ function Get-ProfileScan { # Manual stack-based walk. AllDirectories enumeration THROWS on the # first access-denied directory and aborts the remaining walk - and # every profile contains denied junctions (Application Data etc.), so - # sizes came back ~0 in the field (PCH-DT-CJP2ZC3 2026-07-03). This + # sizes came back ~0 in the field (HOST-E1 2026-07-03). This # walker skips denied dirs and reparse points but keeps walking. $dirStack = New-Object 'System.Collections.Generic.Stack[string]' $dirStack.Push($profileDir) @@ -1348,12 +1525,20 @@ function Add-IntelHit { # ============================================================================== Initialize-Logging +# Flush state-directory hardening notes captured before the log was open (the +# guard runs during config load, ahead of Initialize-Logging). +if ($Script:StateGuardNotes.Count) { + foreach ($n in $Script:StateGuardNotes) { Log-Warn "State directory - $n" } +} else { + Log-Info "State directory - $Script:StateDir owner and ACL verified" +} + # Detect PS version compatibility $Script:UseNewPSFeatures = $Script:PSVer -ge 5 # Banner $bannerWidth = 78 -$version = 'ShellKnight v2026.09.25.004' +$version = 'ShellKnight v2026.09.26.001' $hostname = $env:COMPUTERNAME $timestamp = Get-Date -Format 'yyyy-MM-dd HH:mm:ss' $psver = "PS $($PSVersionTable.PSVersion.Major).$($PSVersionTable.PSVersion.Minor)" @@ -1385,6 +1570,11 @@ $Script:Health = [ordered]@{ self_scheduled = [bool]$SK_SelfSchedule schedule_hours = $SK_ScheduleHours next_run = $null + # State-directory hardening outcome (captured at config load). Lets the + # dashboard flag a box where a user had pre-created the folder or planted a + # config.json / run.ps1 - a sign of an attempted local privilege escalation. + state_dir_repaired = [bool]$Script:StateDirGuard.Recreated + state_dir_files_removed = $Script:StateGuardFilesRemoved } try { $existingTask = Get-ScheduledTask -TaskName 'ShellKnight' -ErrorAction Stop @@ -2212,7 +2402,7 @@ if ($Script:Config.ProcessEngine_Enabled) { # an unanchored regex, so the 'play' family matched NVDisplay.Container and # Microsoft.Media.Player - and because the path check below was skipped for # pattern hits, both were force-killed. 170 terminations across 7 endpoints - # before this was caught (field FP 2026-09-08, Richardson Law). Same class of + # before this was caught (field FP 2026-09-08, Customer A). Same class of # bug as the bare 'miner' substring fixed earlier; anchoring is the general # fix, so short family names ('play', 'conti', 'clop') stay usable. # Boundary is alphanumeric-only, NOT \b: \b treats '_' as a word character, @@ -3389,12 +3579,12 @@ if ($Script:Config.ReportingEngine_Enabled) { # Sophos HitmanPro support driver - field FP 2026-06-24 'HitmanPro 3.7 Support Driver', # Windows Defender's own components. Defender re-registers these on every - # definition update, so they fire constantly - field FP 2026-09-08 RLG. + # definition update, so they fire constantly - field FP 2026-09-08 CUSTA. 'Microsoft Defender Core Service', 'Microsoft WdAiNisDrv Driver', # Bitdefender/Avira engine drivers. Installed to a bare system32\DRIVERS # path with no vendor directory, so a path rule cannot catch these - # - field FP 2026-09-08 RLG. + # - field FP 2026-09-08 CUSTA. 'Avira Network Filter', 'Avira Sentry Driver', 'netprotection_network_filter', @@ -3425,17 +3615,17 @@ if ($Script:Config.ReportingEngine_Enabled) { 'infocyte', # Datto EDR / Infocyte agent path 'centrastage', # Datto RMM install tree incl. bundled UltraVNC (uvnc_service) - field FP 2026-06-24 'hitmanpro', # Sophos HitmanPro driver - field FP 2026-06-24 - 'silver bullet technology', # SBT check-scanning suite (SBTKernel, Ranger) - field FP 2026-06-02 RAS1 - 'paniniusb', # Panini check scanner USB driver - field FP 2026-06-02 RAS1 - 'googleupdater', # Chrome updater re-registers services on every Chrome update - field FP 2026-07-03 PCH-DT - 'eset\\remoteadministrator', # ESET PROTECT agent updater re-registers its service - field FP 2026-07-03 St. Michael (regex: \\ = literal backslash) + 'silver bullet technology', # SBT check-scanning suite (SBTKernel, Ranger) - field FP 2026-06-02 HOST-D1 + 'paniniusb', # Panini check scanner USB driver - field FP 2026-06-02 HOST-D1 + 'googleupdater', # Chrome updater re-registers services on every Chrome update - field FP 2026-07-03 HOST-E1 + 'eset\\remoteadministrator', # ESET PROTECT agent updater re-registers its service - field FP 2026-07-03 Customer B (regex: \\ = literal backslash) # Windows Defender re-registers a randomly-named MpKsl driver on # every definition update, so the name can never be allowlisted - the - # path is the only stable handle - field FP 2026-09-08 RLG. + # path is the only stable handle - field FP 2026-09-08 CUSTA. 'windows defender', 'drivers\\wd\\', # Defender's driver directory (KslD.sys, WdAiNisDrv.sys) - 'dell\\saremediation', # Dell factory remediation plugin (BioNTDrv) - field FP 2026-09-08 RLG - 'datto rollback driver' # Our own RMM rollback driver - field FP 2026-09-08 RLG + 'dell\\saremediation', # Dell factory remediation plugin (BioNTDrv) - field FP 2026-09-08 CUSTA + 'datto rollback driver' # Our own RMM rollback driver - field FP 2026-09-08 CUSTA ) + @($Script:Config.Svc7045_ExtraPaths | Where-Object { $_ }) $svcGroups = @{} @@ -3840,7 +4030,7 @@ $freeAfterGB = if ($diskAfter) { [math]::Round($diskAfter.FreeSpace / 1GB, 1) } $sepLine = '=' * 80 Log-Info $sepLine -Log-Info " ShellKnight v2026.09.25.004 - Report" +Log-Info " ShellKnight v2026.09.26.001 - Report" Log-Info " Hostname : $($env:COMPUTERNAME)" Log-Info " Run Date : $(Get-Date -Format 'yyyy-MM-dd HH:mm:ss')" Log-Info " Runtime : $runtime seconds" @@ -3853,7 +4043,7 @@ Log-Info $sepLine $bannerWidth2 = 78 Write-Host '' Write-Host " $sepLine" -ForegroundColor Cyan -Write-Host " ShellKnight v2026.09.25.004 - Report" -ForegroundColor Cyan +Write-Host " ShellKnight v2026.09.26.001 - Report" -ForegroundColor Cyan Write-Host " Hostname : $($env:COMPUTERNAME)" -ForegroundColor White Write-Host " Run Date : $(Get-Date -Format 'yyyy-MM-dd HH:mm:ss')" -ForegroundColor White Write-Host " Runtime : $runtime seconds" -ForegroundColor White @@ -4129,7 +4319,7 @@ $jsonStamp= Get-Date -Format 'yyyy-MM-dd_HHmm' $jsonPath = "$jsonDir\ShellKnight_${jsonStamp}_$($env:COMPUTERNAME).json" $jsonData = [ordered]@{ - version = 'v2026.09.25.004' + version = 'v2026.09.26.001' device_id = $Script:DeviceId hardware_type = $Script:MachineInfo['Hardware Type'] site_name = $SK_SiteName diff --git a/docs/adr/0006-device-identity-and-site-assignment.md b/docs/adr/0006-device-identity-and-site-assignment.md index 6892d8b..f91d49f 100644 --- a/docs/adr/0006-device-identity-and-site-assignment.md +++ b/docs/adr/0006-device-identity-and-site-assignment.md @@ -31,7 +31,7 @@ We want a machine to have one identity for its whole life, and we want the flexi - New sites can be created on the fly during a move. **Bad:** -- Manual-wins means a device can display a site that disagrees with the key it reports under. That's intentional but can look odd ("why is this box reporting with St. Michael's key but shown under Clinic B?"). The device_id + key are both visible for auditing. +- Manual-wins means a device can display a site that disagrees with the key it reports under. That's intentional but can look odd ("why is this box reporting with Customer B's key but shown under Clinic B?"). The device_id + key are both visible for auditing. - Hardware UUID is blank or duplicated on some cheap/whitebox hardware — the MachineGuid fallback covers most of it, but a truly duplicated UUID would merge two machines. Rare; can be detected by hostname mismatch on a device_id. - Imaging/sysprep regenerates MachineGuid (not the hardware UUID) — for hardware-UUID machines this is a non-issue; for fallback machines a re-image looks like a new device. - Migration created interim `host:` device rows; the first report from ShellKnight ≥ v2026.07.03.014 (real hardware UUID) creates the canonical device, leaving the `host:` row stale until pruned. diff --git a/tests/Test-StateDirGuard.ps1 b/tests/Test-StateDirGuard.ps1 new file mode 100644 index 0000000..58c2b2d --- /dev/null +++ b/tests/Test-StateDirGuard.ps1 @@ -0,0 +1,321 @@ +<# +.SYNOPSIS + Regression test: the state directory is hardened before anything on disk is + trusted, and only SYSTEM- or Administrators-owned state is trusted. + +.DESCRIPTION + C:\ProgramData\ShellKnight holds config.json (read at startup), run.ps1 (the + native ShellKnight scheduled task executes it as SYSTEM every 8 h) and the + Logs, JSON and Intel folders. By default ProgramData lets BUILTIN\Users + create files and folders in its subfolders, and CREATOR OWNER gets full + control of what they create, so on a box where ShellKnight had never run a + standard user could pre-create the folder (or run.ps1 / config.json) and own + it - then choose the code SYSTEM runs, or redirect the run report and its API + key through config.json. + + v2026.09.26.001 adds a guard that runs during config load, ahead of + Initialize-Logging: it creates or repairs C:\ProgramData\ShellKnight with an + explicit ACL (SYSTEM and Administrators full control, Users read only, no + inherited create rights), rebuilds a folder a user already owns, and deletes + any config.json or run.ps1 not owned by SYSTEM or Administrators before it is + read or executed. + + This runs the extracted guard verbatim from ShellKnight.ps1 under + StrictMode 2, with Get-Acl and icacls mocked and real temp directories for + the filesystem operations. It asserts the create / repair / rebuild + decisions, the owner checks on config.json and run.ps1, and that the icacls + arguments carry the three SIDs with the '*' prefix and no localized + principal. From the script's own source it asserts the guard runs before the + config.json read and that the SID constants are the expected values. + + It does NOT replace a real Windows run: Get-Acl, icacls and NTFS inheritance + are Windows behaviours this test mocks. ShellKnight.ps1 is a monolith that + executes on load, so the code is extracted textually rather than dot-sourced. +#> +Set-StrictMode -Version 2 +# The test's own logic stops on any error. Only the extracted ShellKnight code +# runs under the script's own 'SilentlyContinue' (see Invoke-Verbatim). +$ErrorActionPreference = 'Stop' + +$scriptPath = Join-Path (Split-Path $PSScriptRoot -Parent) 'ShellKnight.ps1' +$source = Get-Content -LiteralPath $scriptPath -Raw + +function Get-Section { + param([string]$Pattern, [string]$What) + $m = [regex]::Match($source, $Pattern) + if (-not $m.Success) { throw "$What not found in ShellKnight.ps1 - did it get renamed or moved?" } + $m.Value +} + +$sidBlock = Get-Section '(?ms)^\$Script:SID_System\s+=.*?^\$Script:TrustedOwnerSids\s+=[^\r\n]*' 'the state-guard SID constants' +$icaclsFn = Get-Section '(?ms)^function Invoke-Icacls\s+\{.*?^\}' 'function Invoke-Icacls' +$functions = foreach ($fn in 'Add-StateGuardNote', 'Get-OwnerSid', 'Test-TrustedOwner', + 'Set-StateDirAcl', 'Protect-StateDirectory', 'Remove-UntrustedStateFile') { + Get-Section "(?ms)^function $fn\s+\{.*?^\}" "function $fn" +} + +# --- Mocks ------------------------------------------------------------------- +function Say { param([string]$m, [string]$c = 'Gray') Microsoft.PowerShell.Utility\Write-Host $m -ForegroundColor $c } +function Write-Host { } + +# The filesystem is real (a temp tree). Get-Acl and icacls are not: Get-Acl does +# not exist off Windows, and icacls changes nothing here. Owner is looked up per +# path; a path in $Script:AclThrows reads as an unreadable owner. +$Script:OwnerByPath = @{} +$Script:AclThrows = @{} +$Script:DefaultOwner = 'S-1-5-18' # SYSTEM, unless a path overrides it +function Key([string]$Path) { [System.IO.Path]::GetFullPath($Path) } +function Set-Owner([string]$Path, [string]$Sid) { $Script:OwnerByPath[(Key $Path)] = $Sid } +function Set-Unreadable([string]$Path) { $Script:AclThrows[(Key $Path)] = $true } + +function Get-Acl { + param($LiteralPath, $ErrorAction) + $key = Key $LiteralPath + if ($Script:AclThrows.ContainsKey($key) -and $Script:AclThrows[$key]) { + throw 'the owner could not be read (mock)' + } + $sid = if ($Script:OwnerByPath.ContainsKey($key)) { $Script:OwnerByPath[$key] } else { $Script:DefaultOwner } + $acl = [pscustomobject]@{} + $acl | Add-Member -MemberType ScriptMethod -Name GetOwner ` + -Value ({ param($Type) [pscustomobject]@{ Value = $sid } }).GetNewClosure() + $acl +} + +$Script:IcaclsCalls = New-Object 'System.Collections.Generic.List[object]' +function Invoke-Icacls { + param([Parameter(Mandatory = $true)][string[]]$Arguments) + $Script:IcaclsCalls.Add([pscustomobject]@{ Args = $Arguments }) + 0 +} + +# The guard's own note buffer, initialised as the script does. +$Script:StateGuardNotes = New-Object 'System.Collections.Generic.List[string]' + +# Bring the SID constants and the guard functions into scope, then check the +# SIDs are the ones the ACL depends on. +Invoke-Expression $sidBlock +foreach ($f in $functions) { Invoke-Expression $f } + +function Invoke-Verbatim([string]$Code) { + # Not used for the guard (its functions are already defined above), kept for + # symmetry with the other Test-*.ps1 harnesses. + $ErrorActionPreference = 'SilentlyContinue' + . ([scriptblock]::Create($Code)) +} + +$failures = 0 +function Fail([string]$Label, [string]$Why) { + Say " FAIL $Label - $Why" Red + $script:failures++ +} + +Say '' +Say ' State directory: hardened before anything on disk is trusted (StrictMode 2)' +Say ' ---------------------------------------------------------------------------' + +# --- SID constants ------------------------------------------------------------ +$before = $failures +if ($Script:SID_System -ne 'S-1-5-18') { Fail 'sids' "SID_System is '$($Script:SID_System)', expected S-1-5-18" } +if ($Script:SID_Administrators -ne 'S-1-5-32-544') { Fail 'sids' "SID_Administrators is '$($Script:SID_Administrators)', expected S-1-5-32-544" } +if ($Script:SID_Users -ne 'S-1-5-32-545') { Fail 'sids' "SID_Users is '$($Script:SID_Users)', expected S-1-5-32-545" } +if (@($Script:TrustedOwnerSids) -join ',' -ne 'S-1-5-18,S-1-5-32-544') { Fail 'sids' "TrustedOwnerSids is '$($Script:TrustedOwnerSids -join ',')'" } +if ($failures -eq $before) { Say ' ok sids - SYSTEM S-1-5-18, Administrators S-1-5-32-544, Users S-1-5-32-545; trusted = SYSTEM + Administrators' Green } + +$tmpRoot = Join-Path ([System.IO.Path]::GetTempPath()) ('sk-statedir-test-' + [guid]::NewGuid().ToString('N')) +$null = New-Item -ItemType Directory -Path $tmpRoot -Force +try { + $USER = 'S-1-5-21-100-200-300-1001' # a made-up local user + $n = 0 + function New-Case { $script:n++; Join-Path $tmpRoot "case$script:n" } + function Reset-Guard { + $Script:OwnerByPath = @{}; $Script:AclThrows = @{}; $Script:DefaultOwner = 'S-1-5-18' + $Script:StateGuardNotes.Clear(); $Script:IcaclsCalls.Clear() + } + function New-Sentinel([string]$Dir) { + $null = New-Item -ItemType Directory -Path $Dir -Force + $s = Join-Path $Dir 'sentinel.txt'; Set-Content -LiteralPath $s -Value 'keep' ; $s + } + function Had-Note([string]$Text) { @($Script:StateGuardNotes | Where-Object { $_ -like "*$Text*" }).Count -gt 0 } + + # --- Protect-StateDirectory -------------------------------------------------- + + # Fresh box: the folder does not exist and is created. + Reset-Guard + $before = $failures + $dir = New-Case + $g = Protect-StateDirectory -Path $dir + if (-not (Test-Path -LiteralPath $dir)) { Fail 'fresh box' 'the folder was not created' } + if (-not $g.Created) { Fail 'fresh box' 'Created should be true' } + if ($g.Recreated) { Fail 'fresh box' 'Recreated should be false (nothing was there)' } + if (-not $g.AclApplied){ Fail 'fresh box' 'AclApplied should be true' } + if ($Script:StateGuardNotes.Count) { Fail 'fresh box' "unexpected note(s): $($Script:StateGuardNotes -join ' | ')" } + if ($Script:IcaclsCalls.Count -lt 3) { Fail 'fresh box' 'expected icacls to be invoked to set the ACL' } + if ($failures -eq $before) { Say ' ok fresh box - folder created and ACL applied, no repair' Green } + + # Steady state: SYSTEM owns the folder, so it and its contents are kept. + Reset-Guard + $before = $failures + $dir = New-Case; $sentinel = New-Sentinel $dir; Set-Owner $dir 'S-1-5-18' + $g = Protect-StateDirectory -Path $dir + if (-not (Test-Path -LiteralPath $sentinel)) { Fail 'steady state' 'the folder contents were removed' } + if (-not $g.Existed) { Fail 'steady state' 'Existed should be true' } + if (-not $g.OwnerTrusted) { Fail 'steady state' 'OwnerTrusted should be true for SYSTEM' } + if ($g.Recreated) { Fail 'steady state' 'Recreated should be false' } + if ($g.Created) { Fail 'steady state' 'Created should be false' } + if (-not $g.AclApplied) { Fail 'steady state' 'AclApplied should be true (ACL re-asserted)' } + if ($Script:StateGuardNotes.Count) { Fail 'steady state' "unexpected note(s): $($Script:StateGuardNotes -join ' | ')" } + if ($failures -eq $before) { Say ' ok steady state - SYSTEM-owned folder kept, ACL re-asserted' Green } + + # Administrators-owned folder is trusted just the same. + Reset-Guard + $before = $failures + $dir = New-Case; $sentinel = New-Sentinel $dir; Set-Owner $dir 'S-1-5-32-544' + $g = Protect-StateDirectory -Path $dir + if (-not (Test-Path -LiteralPath $sentinel)) { Fail 'admins-owned' 'the folder contents were removed' } + if (-not $g.OwnerTrusted) { Fail 'admins-owned' 'OwnerTrusted should be true for Administrators' } + if ($g.Recreated) { Fail 'admins-owned' 'Recreated should be false' } + if ($failures -eq $before) { Say ' ok admins-owned - Administrators-owned folder kept' Green } + + # A user owns the folder: it and its contents are removed and recreated. + Reset-Guard + $before = $failures + $dir = New-Case; $sentinel = New-Sentinel $dir; Set-Owner $dir $USER + $g = Protect-StateDirectory -Path $dir + if (Test-Path -LiteralPath $sentinel) { Fail 'user-owned' 'the user-owned contents were NOT removed' } + if (-not (Test-Path -LiteralPath $dir)) { Fail 'user-owned' 'the folder was not recreated' } + if ($g.OwnerTrusted) { Fail 'user-owned' 'OwnerTrusted should be false for a user SID' } + if (-not $g.Recreated){ Fail 'user-owned' 'Recreated should be true' } + if (-not $g.Created) { Fail 'user-owned' 'Created should be true (recreated)' } + if (-not (Had-Note 'not SYSTEM or Administrators')) { Fail 'user-owned' 'expected a note that the owner is not SYSTEM or Administrators' } + if ($failures -eq $before) { Say ' ok user-owned - folder rebuilt, contents dropped, note logged' Green } + + # The owner cannot be read (Get-Acl throws): treated as untrusted, rebuilt. + Reset-Guard + $before = $failures + $dir = New-Case; $sentinel = New-Sentinel $dir; Set-Unreadable $dir + $g = Protect-StateDirectory -Path $dir + if (Test-Path -LiteralPath $sentinel) { Fail 'unreadable owner' 'the contents were NOT removed' } + if ($null -ne $g.OwnerSid) { Fail 'unreadable owner' "OwnerSid should be null, was '$($g.OwnerSid)'" } + if (-not $g.Recreated) { Fail 'unreadable owner' 'Recreated should be true' } + if (-not (Had-Note 'an unknown account')) { Fail 'unreadable owner' "expected a note naming 'an unknown account'" } + if ($failures -eq $before) { Say ' ok unreadable owner - treated as untrusted and rebuilt' Green } + + # --- Remove-UntrustedStateFile (config.json / run.ps1) ----------------------- + + foreach ($case in @( + @{ Label = 'config.json'; Name = 'config.json' }, + @{ Label = 'run.ps1'; Name = 'run.ps1' } + )) { + $label = $case.Label + + # Trusted owner: kept, not read. + Reset-Guard + $before = $failures + $dir = New-Case; $null = New-Item -ItemType Directory -Path $dir -Force + $file = Join-Path $dir $case.Name; Set-Content -LiteralPath $file -Value '{}' ; Set-Owner $file 'S-1-5-32-544' + $removed = Remove-UntrustedStateFile -Path $file -Label $label + if ($removed) { Fail "$label trusted" 'a trusted file should not be removed' } + if (-not (Test-Path -LiteralPath $file)) { Fail "$label trusted" 'the trusted file was deleted' } + if ($Script:StateGuardNotes.Count) { Fail "$label trusted" 'no note should be logged for a trusted file' } + if ($failures -eq $before) { Say " ok $label trusted - Administrators-owned, kept" Green } + + # User owner: deleted, note logged. + Reset-Guard + $before = $failures + $dir = New-Case; $null = New-Item -ItemType Directory -Path $dir -Force + $file = Join-Path $dir $case.Name; Set-Content -LiteralPath $file -Value 'evil' ; Set-Owner $file $USER + $removed = Remove-UntrustedStateFile -Path $file -Label $label + if (-not $removed) { Fail "$label user" 'a user-owned file should be removed' } + if (Test-Path -LiteralPath $file) { Fail "$label user" 'the user-owned file was NOT deleted' } + if (-not (Had-Note "$label owned by $USER")) { Fail "$label user" 'expected a note naming the file and owner' } + if ($failures -eq $before) { Say " ok $label user-owned - deleted, note logged" Green } + + # Unreadable owner: deleted as untrusted. + Reset-Guard + $before = $failures + $dir = New-Case; $null = New-Item -ItemType Directory -Path $dir -Force + $file = Join-Path $dir $case.Name; Set-Content -LiteralPath $file -Value 'evil' ; Set-Unreadable $file + $removed = Remove-UntrustedStateFile -Path $file -Label $label + if (-not $removed) { Fail "$label unknown" 'an unreadable-owner file should be removed' } + if (Test-Path -LiteralPath $file) { Fail "$label unknown" 'the file was NOT deleted' } + if (-not (Had-Note 'an unknown account')) { Fail "$label unknown" "expected a note naming 'an unknown account'" } + if ($failures -eq $before) { Say " ok $label unknown-owner - deleted, note logged" Green } + + # Absent file: nothing to do. + Reset-Guard + $before = $failures + $dir = New-Case; $null = New-Item -ItemType Directory -Path $dir -Force + $file = Join-Path $dir $case.Name + if (Remove-UntrustedStateFile -Path $file -Label $label) { Fail "$label absent" 'an absent file should return false' } + if ($failures -eq $before) { Say " ok $label absent - nothing removed" Green } + } + + # --- Test-TrustedOwner ------------------------------------------------------- + Reset-Guard + $before = $failures + $dir = New-Case; $null = New-Item -ItemType Directory -Path $dir -Force + $p = Join-Path $dir 'x'; Set-Content -LiteralPath $p -Value '.' + Set-Owner $p 'S-1-5-18'; if (-not (Test-TrustedOwner -Path $p)) { Fail 'trusted-owner' 'SYSTEM should be trusted' } + Set-Owner $p 'S-1-5-32-544'; if (-not (Test-TrustedOwner -Path $p)) { Fail 'trusted-owner' 'Administrators should be trusted' } + Set-Owner $p $USER; if (Test-TrustedOwner -Path $p) { Fail 'trusted-owner' 'a user should NOT be trusted' } + $missing = Join-Path $dir 'does-not-exist' + if (Test-TrustedOwner -Path $missing) { Fail 'trusted-owner' 'a missing path should NOT be trusted' } + if ($failures -eq $before) { Say ' ok trusted-owner - SYSTEM/Administrators trusted; user and missing not' Green } + + # --- icacls arguments: SIDs, not names -------------------------------------- + Reset-Guard + $before = $failures + $dir = New-Case + $null = Protect-StateDirectory -Path $dir + $allArgs = @($Script:IcaclsCalls | ForEach-Object { $_.Args }) + if (-not ($allArgs -contains '/inheritance:r')) { Fail 'icacls' 'expected /inheritance:r to drop inherited ACEs' } + if (-not ($allArgs -contains '/setowner')) { Fail 'icacls' 'expected /setowner to reclaim ownership' } + if (-not ($allArgs -contains '/grant:r')) { Fail 'icacls' 'expected /grant:r to replace the DACL' } + $grantArgs = @($allArgs | Where-Object { $_ -match ':\((OI|CI)' }) + if ($grantArgs.Count -ne 3) { Fail 'icacls' "expected 3 grant entries, saw $($grantArgs.Count): $($grantArgs -join ' ')" } + foreach ($ga in $grantArgs) { + if ($ga -notmatch '^\*S-1-') { Fail 'icacls' "grant principal is not a '*'-prefixed SID: '$ga'" } + } + foreach ($want in @('*S-1-5-18:(OI)(CI)F', '*S-1-5-32-544:(OI)(CI)F', '*S-1-5-32-545:(OI)(CI)RX')) { + if (-not ($grantArgs -contains $want)) { Fail 'icacls' "missing grant entry $want" } + } + # /setowner target is the Administrators SID, not a name. + $ownerCall = @($Script:IcaclsCalls | Where-Object { $_.Args -contains '/setowner' })[0] + if ($ownerCall) { + $oi = [array]::IndexOf([array]$ownerCall.Args, '/setowner') + if ($ownerCall.Args[$oi + 1] -ne '*S-1-5-32-544') { Fail 'icacls' "setowner target is '$($ownerCall.Args[$oi + 1])', expected *S-1-5-32-544" } + } + # No localized principal anywhere in the icacls arguments. + foreach ($bad in 'Administrators', 'Users', 'SYSTEM', 'BUILTIN', 'Everyone', 'Authenticated') { + if (@($allArgs | Where-Object { $_ -like "*$bad*" }).Count) { Fail 'icacls' "a localized name '$bad' was passed to icacls" } + } + if ($failures -eq $before) { Say ' ok icacls - ACL set by SID (*S-1-5-18/544/545), owner reclaimed, no localized name' Green } + + # --- Source order: the guard runs before config.json is read ----------------- + $before = $failures + $iGuard = $source.IndexOf('Protect-StateDirectory -Path $Script:StateDir') + $iCfgRun = $source.IndexOf('$Script:StateGuardFilesRemoved++') + $iRead = $source.IndexOf('if (Test-Path $Script:ConfigPath) {') + if ($iGuard -lt 0) { Fail 'source order' 'the Protect-StateDirectory call was not found' } + if ($iRead -lt 0) { Fail 'source order' 'the config.json read block was not found' } + if ($iGuard -ge 0 -and $iRead -ge 0 -and $iGuard -ge $iRead) { Fail 'source order' 'the folder guard must run before config.json is read' } + if ($iCfgRun -ge 0 -and $iRead -ge 0 -and $iCfgRun -ge $iRead) { Fail 'source order' 'the config.json / run.ps1 owner check must run before config.json is read' } + if ($failures -eq $before) { Say ' ok source order - folder guard and file checks run before config.json is read' Green } + + # --- Invoke-Icacls wrapper is sane (text) ------------------------------------ + $before = $failures + if ($icaclsFn -notmatch 'icacls') { Fail 'wrapper' 'Invoke-Icacls does not call icacls' } + if ($icaclsFn -notmatch 'LASTEXITCODE') { Fail 'wrapper' 'Invoke-Icacls does not return the exit code' } + if ($failures -eq $before) { Say ' ok wrapper - Invoke-Icacls runs icacls and returns its exit code' Green } +} +finally { + if (Test-Path -LiteralPath $tmpRoot) { Remove-Item -LiteralPath $tmpRoot -Recurse -Force -ErrorAction SilentlyContinue } +} + +Say '' +if ($failures -gt 0) { + Say " FAILED - $failures assertion(s)" Red + exit 1 +} +Say ' PASS - all assertions' Green +exit 0