v2026.09.26.001: harden C:\ProgramData\ShellKnight ACL against local privilege escalation - #6
Open
cdburgess75 wants to merge 2 commits into
Open
cdburgess75 wants to merge 2 commits into
cdburgess75 wants to merge 2 commits into
Conversation
…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 (local privilege escalation) or redirect the run report and its API key through config.json. Before config.json is read or the scheduled task (which runs run.ps1) is trusted, a new guard: - creates or repairs the folder with an explicit ACL set by SID (SYSTEM / Administrators full control, Users read only), inheritance removed so ProgramData's Users-create ACEs are gone, owner reclaimed to Administrators; - removes and recreates a folder that SYSTEM or Administrators does not own; - deletes any config.json or run.ps1 not owned by SYSTEM or Administrators, so a planted config.json is never read and a planted run.ps1 is never executed. Owner is read with (Get-Acl).GetOwner, the model the Intel cache check uses (v2026.09.25.004); an unreadable owner is untrusted; icacls uses SIDs (S-1-5-18 / S-1-5-32-544 / S-1-5-32-545), never localized names. The health object gains state_dir_repaired and state_dir_files_removed so the dashboard can flag an endpoint that showed signs of tampering. New tests/Test-StateDirGuard.ps1 runs the extracted guard verbatim under StrictMode 2 with Get-Acl and icacls mocked and real temp directories. NOT yet run on real Windows: the ACL and owner checks need one real SYSTEM run before this reaches main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6 tasks
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suspected issue (not yet confirmed on real Windows)
C:\ProgramData\ShellKnight\holdsconfig.json(read at startup),run.ps1(the nativeShellKnightscheduled task executes it as SYSTEM every 8 h), and theLogs,JSONandIntelfolders. By default ProgramData letsBUILTIN\Userscreate files and folders in its subfolders, andCREATOR OWNERgets full control of what they create.So on an endpoint where ShellKnight has never run, a standard user can pre-create
C:\ProgramData\ShellKnight(orrun.ps1, orconfig.json) and own it. They could then:run.ps1-> local privilege escalation;config.json(BattlefieldURL/BattlefieldApiKey).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.
This is the same class of exposure PR #5 already guards for the Intel cache (owner must be
S-1-5-18orS-1-5-32-544); this PR applies that model to the whole state directory and the two sensitive files, and locks the folder ACL down so the create right does not exist in the first place.What this changes
A new guard runs during config load, before
config.jsonis read or the scheduled task is trusted (verified by a source-order assertion in the test):icacls, never localized names):SYSTEM(S-1-5-18) andAdministrators(S-1-5-32-544) full control,Users(S-1-5-32-545) read/execute, inheritance removed (/inheritance:r) so ProgramData'sUsers-create ACEs are gone, owner reclaimed to Administrators. Set on the base folder only; existingLogs/JSON/Intelchildren pick up the change through inheritance, so no/Tsweep runs on every check-in.config.jsonandrun.ps1are owner-checked: either one not owned by SYSTEM or Administrators is deleted, so a plantedconfig.jsonis never read and a plantedrun.ps1is never executed.run.ps1is rewritten later in the run; a removedconfig.jsonfalls back to built-in defaults and env vars.(Get-Acl).GetOwner([SecurityIdentifier]), the PR v2026.09.25.004: the Intel Engine loads threat intel; every intel match is report-only #5 model; an unreadable owner is treated as untrusted. The whole guard is wrapped so a hardening failure logs a warning and never stops the run.health.state_dir_repairedandhealth.state_dir_files_removedso Battlefield can flag an endpoint that showed signs of a local tampering attempt (report stored whole per ADR 0002; nothing displays them yet).Verification status
[System.Management.Automation.Language.Parser]::ParseFilecleantests/Test-*.ps1pass under pwsh (incl.Test-IntelEngine's whole-script$Script:Config.<Name>AST check)Write-Host/ShouldProcess/ singular-noun classes the existing script already triggers)tests/Test-StateDirGuard.ps1(18 assertions): fresh box, steady state, SYSTEM- and Administrators-owned, user-owned (rebuilt), unreadable owner, trusted/user/unknown/absentconfig.jsonandrun.ps1,Test-TrustedOwner,icaclsargs are*-prefixed SIDs with no localized principal, and the source-order guaranteeGet-Acl,icaclsand NTFS inheritance are Windows behaviours the test mocks. Per the repo rule, this needs one real SYSTEM run before it reachesmain— please do not merge until then.Suggested real-Windows checks
icacls C:\ProgramDataon a stock box to confirm the inheritedUserscreate ACEs ((CI)(IO)(WD)on subfolders,(CI)(WD), plusCREATOR OWNER:(OI)(CI)(IO)(F)).mkdir C:\ProgramData\ShellKnight(or drop arun.ps1) before ShellKnight's first run; confirm the user owns it (icacls/Get-Acl).SYSTEM:(OI)(CI)(F) Administrators:(OI)(CI)(F) Users:(OI)(CI)(RX)with inheritance off, the planted file is gone, and the run report showsstate_dir_repaired: true.State directory ... owner and ACL verifiedand reportsstate_dir_repaired: false,state_dir_files_removed: 0.Stacked on #5 (base
fix/intel-engine-load).🤖 Generated with Claude Code