Darling install scripts refuse a writable-by-others install tree before the lock (#4043)

[erikdarlingdata]

Closes #4043.

Why

#4038's review left two residuals. The issue's last comment withdrew the Authenticode/hash-manifest
option for part 1 and replaced it: a check run BY install-darling.ps1 AFTER the lock only catches an
attacker who leaves the script itself untouched, and the same local user who can replace the service
exe can just as easily replace install-darling.ps1, which the admin then runs elevated. The trust
root is where the admin extracted the zip, not anything a script reads back from inside that tree.

What changes

Part 1 (implemented, matches the revised plan).

• install-darling.ps1 step 1a (new, before 1b2's lock) reads the install root's ACL and the
  service exe's own file ACL. Any Allow ACE granting write/append/delete/change-permissions/
  take-ownership to a principal outside SYSTEM, Administrators, TrustedInstaller and the admin running
  the script refuses the install, names the principals, says files may already have been replaced, and
  points at C:\Program Files\.... -AcceptWritableExtraction skips it (documented as a dev-loop
  escape hatch, not for a real install).
• upgrade-darling.ps1 gets the same check, and it has its own real pre-lock window: a zip
  -Source is covered by the existing SHA256 check regardless of who could write to its folder (the
  hash is of the zip's content), but a folder -Source has no content check at all - the script
  only confirms the service exe's name is present and says outright "this script cannot verify it." So
  the folder--Source branch now runs the same ACL check (same trusted set, same
  -AcceptWritableExtraction) before the overlay copies that folder over the already-locked
  -InstallRoot.
• The two new functions (Get-UntrustedWriteGrantees, Get-DarlingPreLockTrustedSids) are
  byte-identical in both scripts, same idiom as Lock-DarlingInstallTree - checked by a new xunit test
  the same way DarlingInstallLocationTests already checks the lock and
  Get-DarlingServiceLogonName.
• README: the install-location section now recommends C:\Program Files\PerformanceMonitorDarling
  over a bare C:\ folder and explains why (Program Files denies ordinary users write by default; a
  folder directly under C:\ inherits Authenticated Users: Modify the instant it's extracted), and
  documents the new refusal and switch. The scripted-install summary mentions the new step 1a and links
  the switch.

Part 2 (audit done; NOT narrowed - leaving Modify as #4038 shipped it).

Static audit of every file/directory write the service process makes, by grepping
AppContext.BaseDirectory and the runtime-root fields through Darling/PerformanceMonitor.Darling.Service:

Path (relative to install root)	Code	Confidence
pg-runtime\ (extract, rescue-copy to pg-runtime-prev\, pg-runtime.sha256/.stamp/.blocked stamp files)	DarlingManagedPostgres.cs (_runtimeRoot = AppContext.BaseDirectory + "pg-runtime"), DarlingStoreUpgrade.cs (stamp/blocked paths all Path.Combine(runtimeRoot, ...))	Confirmed write, confirmed scoped to pg-runtime/pg-runtime-prev
darling.json	DarlingConfig.ResolveConfigPath / Load	Read-only from the persistent service process - confirmed not written there. Written only by install-darling.ps1 step 4b and the --configure-network CLI verb, both running as the elevated admin, not the service account
Install root (diagnostic report)	DarlingInstallLocation.Report, called from DarlingWorker.cs	Read-only (logs findings, writes nothing)
wwwroot / web content root	Mcp/DarlingWebHostService.cs (ContentRootPath = AppContext.BaseDirectory)	Believed read-only (ASP.NET static file serving); not independently verified within this lane's time budget
A BringYourOwnDirectoryName folder beside darling.json, i.e. inside the install root by default	DarlingLogHashKeyFile.DirectoryFor - only when postgres.managed = false AND not running in a container (the non-default, bring-your-own-Postgres case; the default managed path resolves beside the data directory under %ProgramData%, outside the tree)	Uncertain / not closed: this is a real write site outside pg-runtime that the proposed narrowing (Modify on pg-runtime/pg-runtime-prev only) would break for anyone running BYO-Postgres on Windows outside a container

Given the BYO-Postgres log-hash-key-directory finding above, and that the issue itself requires
narrowing to be "verified with a real-service install on a test box" - which this lane's hard fences
(never install/start the real Darling service) rule out doing here regardless - Part 2 is left
unchanged. Narrowing needs: (a) resolving the BYO-Postgres write site (either fold it into the grant
or confirm nobody runs that combination on Windows), and (b) an actual install-and-run verification on
a disposable box. Recommend a follow-up lane sized for that, not folded into this one.

Test plan

• dotnet build Darling/Darling.Tests/Darling.Tests.csproj -c Debug - Build succeeded, 0 Warning(s), 0 Error(s).
• Both scripts parse clean ([System.Management.Automation.Language.Parser]::ParseFile, no syntax errors).
• Darling.Tests.exe -class "*DarlingInstallLocationTests*" - Total: 14, Errors: 0, Failed: 0 (13 pre-existing + 2 new: the byte-identity check and the red-watch check; one pre-existing test file, two new [Fact]s).
• Red-watch: temporarily neutered Get-UntrustedWriteGrantees to return @() in install-darling.ps1 (nothing else changed) and re-ran the new test - it failed (1 Failed) with the no-op in place, then reverted and re-ran the full class green (14/14). Confirms the test actually exercises the shipped function, not a tautology.
• Full Darling.Tests suite - not run; this lane hit the context/time budget after the targeted class passed. The change only touches two .ps1 scripts, one test file, and README prose, so the blast radius outside DarlingInstallLocationTests should be nil, but the coordinator should run the full suite once before arming.
• Real-service install/upgrade test on a disposable box - out of this lane's hard fences (no install/start of the real Darling service here); the coordinator or a follow-up should smoke-test both scripts' new step against a real extraction before shipping, per the issue's own "real-service install test" requirement for Part 2, and ideally exercising the Part 1 refusal path once on a throwaway VM too.

Round-1 fixes

The coordinator ran this PR's own Get-UntrustedWriteGrantees against real folders on this machine and
found two verified defects, both fixed here:

1. False refusal on the very location the refusal message recommends. CREATOR OWNER on
   C:\Program Files and C:\Program Files\dotnet was flagged as an untrusted grantee. CREATOR OWNER,
   CREATOR GROUP and their two SERVER twins (S-1-3-0..3) are inherit-only templates - they grant nothing
   on an object that already exists, only rights a CHILD inherits when someone later creates one, and
   creating that child needs write/create rights on the parent, which the check already tests for every
   other principal. Fixed by excluding those four SIDs by value inside Get-UntrustedWriteGrantees, not by
   adding them to $trusted (a caller must never be able to trust them for real by omission the way a name
   in $trusted would).

   OWNER RIGHTS (S-1-3-4) is different and is not in that exclusion: it redefines what the owner may
   do instead of naming a risk on its own, so what matters is WHO the owner is - an owner outside the
   trusted set holds WRITE_DAC/WRITE_OWNER implicitly and can grant itself anything regardless of the
   current DACL. Get-UntrustedWriteGrantees now also checks the owner of $path directly (the same fact
   Lock-DarlingInstallTree's own post-lock walk already acts on, "owned by ..."), which covers this case
   without trying to interpret the ACE.

2. False refusal on a re-run or upgrade over a tree Install and upgrade lock the Darling install folder, so an ordinary local user can no longer replace the service's binaries (#4034) #4038 already locked. Install and upgrade lock the Darling install folder, so an ordinary local user can no longer replace the service's binaries (#4034) #4038's lock grants the
   Darling service account Modify on the install root, and that account was not in
   Get-DarlingPreLockTrustedSids's static set - so the very first re-run, repair, or reconfigure over an
   already-locked install would have refused itself over the grant Install and upgrade lock the Darling install folder, so an ordinary local user can no longer replace the service's binaries (#4034) #4038 itself made.

   • install-darling.ps1 can run over an existing install: $existing/$isUpgrade already drive a
     real fresh-vs-upgrade branch later in the script. $existing is now resolved earlier, ahead of step
     1a instead of only at step 1b, so 1a's pre-lock check can tell a re-run apart from a fresh extraction.
   • upgrade-darling.ps1's check only ever looks at a folder -Source and its exe - never at
     -InstallRoot - so in the normal flow it does not see the locked installed tree at all. It can still
     see it if -Source itself is (or is derived from) a previously-locked Darling tree, so the same fix
     was applied there too for correctness, at negligible cost.
   • Get-DarlingPreLockTrustedSids now takes an optional $existingServiceAccount, normalized exactly the
     way Lock-DarlingInstallTree normalizes it (LocalSystem spelled out, a leading .\ read as this
     computer). Both call sites pass the current service's logon account (via the existing
     Get-DarlingServiceLogonName) only when a service by that name is already registered.

Tests (Darling/Darling.Tests/DarlingInstallLocationTests.cs, three new [Fact]s, realistic ACLs
instead of idealized ones):

• ThePreLockWritableExtractionCheck_IsSilentOnRealInheritOnlyTemplates_ButStillCatchesARealBroadGrant -
  (a) a temp folder carrying C:\Program Files' own captured DACL must PASS; a pre-round-1 copy of the
  function reproduced inline (not read from git history) is run over the same DACL and must still flag it -
  a red-watch built into the pin, not a one-off manual check; (b) a temp folder carrying C:\'s own DACL
  (Authenticated Users: Modify) must REFUSE; (c) all four inherit-only template SIDs, granted explicitly,
  must be silent regardless of what this box's Program Files happens to carry.
• ThePreLockWritableExtractionCheck_TrustsTheAccountTheLockItselfGranted_OnAnAlreadyLockedTree - a tree
  actually locked via Lock-DarlingInstallTree with a stand-in service account (NT AUTHORITY\LOCAL SERVICE, deliberately not one of the four base-trusted SIDs) is flagged without the account and silent
  with it.
• ThePreLockWritableExtractionCheck_FlagsAnUntrustedOwner_EvenWithAFullyTrustedDacl - Get-Acl is
  shadowed (the same idiom the existing WMI-unavailable test uses) to hand back a fully-trusted DACL with
  only the owner outside the trusted set, since Windows will not let this suite retarget a real object's
  owner to an arbitrary SID without elevation.

Manually red-watched beyond the automated case above: ran the shipped (pre-fix) Get-UntrustedWriteGrantees
against real C:\Program Files and C:\Program Files\dotnet on this machine and confirmed both were
flagged; confirmed the fixed version is silent on both while still flagging a real Authenticated Users
grant on C:\ itself.

• dotnet build Darling/Darling.Tests/Darling.Tests.csproj -c Debug - Build succeeded, 0 Warning(s), 0 Error(s).
• Both scripts parse clean ([System.Management.Automation.Language.Parser]::ParseFile).
• Get-UntrustedWriteGrantees/Get-DarlingPreLockTrustedSids confirmed byte-identical between the two scripts.
• Darling.Tests.exe -class "*DarlingInstallLocationTests*" - Total: 17, Errors: 0, Failed: 0 (14 prior + 3 new).
• Red-watch: temporarily stripped the CREATOR-OWNER exclusion, the owner check, and the
  $existingServiceAccount trust from the shipped install-darling.ps1 (upgrade script untouched) and
  re-ran the class - 4 failed (the 3 new Facts plus the byte-identity check, exactly as expected since
  only one script was neutered); reverted and re-ran green (17/17).
• Full Darling.Tests suite - Total: 13116, Errors: 0, Failed: 0, Skipped: 631 (unrelated live/DB
  tests with no PG rig running in this lane).
• Real-service install/upgrade test on a disposable box - still out of this lane's hard fences (no
  install/start of the real Darling service here); unchanged from round 1, still recommended before
  shipping.

Part 2 (the static write-site audit and the decision to leave the service account's Modify grant
unnarrowed) is unchanged from round 1 below - still handed off, not folded into this lane.

Round-1 security review (disposition)

Findings and comment: #4050 (comment).
Applied on top of the code above, same branch.

• H1 (High) - zip -Source not covered regardless of who could write to its folder. Fixed. Before
  hashing, the zip is copied into a fresh staging folder the script creates (New-DarlingProtectedStagingFolder
  • Protect-DarlingStagingFolder, upgrade-only) with an explicit protected ACL: SYSTEM and Administrators
    only, full control, inheritance removed via icacls /inheritance:r, owner set to Administrators. The
    staged copy is hashed and is the only thing Expand-Archive ever reads; $Source is reassigned to it
    right after the hash check. The folder is deleted in a finally that wraps the rest of the script (see
    below), on every failure path, not just success. Without -Sha256, SHA256SUMS.txt is trusted only when
    its own folder passes the identical recursive check a folder -Source gets (M2); otherwise the script
    refuses and points at -Sha256 with the hash from the release page. With -Sha256, the sidecar folder is
    never even looked at - the hash of the protected copy is the whole proof.
  • Implementation note: the copy-then-extract span (staging creation through the end of the script) is
    wrapped in try { ... } finally { remove $zipStagingFolder }, not re-indented, to keep the diff
    reviewable instead of touching every intervening line. Verified empirically that PowerShell's exit
    (which is how Fail() terminates) still runs an enclosing finally, while trap does not catch
    exit - so finally is the only mechanism that actually guarantees cleanup across Fail() calls between
    staging and extraction.
• M1 (Medium) - docs recommend the now-refused pattern. Fixed. Darling/README.md's upgrade section now
  extracts to C:\Program Files\PerformanceMonitorDarling-staging\<ver> from an elevated shell, with one
  sentence on why (the folder you run the script from is the trust root). The zip+-Sha256 form is still
  documented as an alternative. upgrade-darling.ps1's .PARAMETER Source help, the self-overwrite refusal
  message, and the .PARAMETER AcceptWritableExtraction help all point at the protected folder instead of
  C:\staging\<version>.
• M2 (Medium) - only the root and the exe were checked, so a re-permissioned root hid a swapped child.
  Fixed. Get-UntrustedWriteGrantees gained a -Recurse switch: the root itself is still checked for both
  explicit and inherited grants (that is how the C:\ hole shows up), but every descendant is checked for
  (a) an untrusted owner (never inherited, so a re-permissioned root can't hide who created it) and (b)
  an explicit (non-inherited) write grant to an untrusted principal - inherited grants on a child just
  repeat what the root already carries, which is checked once, not per file. A junction/link below the root
  is reported, not silently skipped (parity with Lock-DarlingInstallTree's own walk). The "for no benefit"
  comment is corrected. Trusted principals now also include any principal that is a direct member of
  BUILTIN\Administrators (Get-LocalAdministratorsDirectMemberSids, via WinNT ADSI rather than
  Get-LocalGroupMember, which throws on an orphaned SID) - nested/domain membership is not resolved; the
  refusal names the principal and -AcceptWritableExtraction is the accepted way past it. If the group
  itself can't be enumerated at all, nothing is added (fail closed).
  • Perf, measured on a real extracted release tree (this repo's Darling/artifacts/pg-runtime.zip,
    1,661 files): the recursive walk took 1.6 seconds. Well under the ~10s threshold, so it stays
    Get-ChildItem -Recurse | Get-Acl rather than moving to .NET enumeration.
• M3 (Medium) - upgrade-darling.ps1 never checked -InstallRoot itself. Fixed. Before its lock, it now
  runs the same recursive check on -InstallRoot (reusing the trust set computed once for both this and the
  folder--Source check) and refuses unless -AcceptWritableExtraction is passed. The message says: a tree
  installed before Install and upgrade lock the Darling install folder, so an ordinary local user can no longer replace the service's binaries (#4034) #4038 was writable by the named principals for its whole life so files may have been
  replaced; the safe path is a fresh install into a protected folder; the switch accepts the risk; this fires
  once, because the lock that follows closes the tree.
• L1 (Low) - mask comment claimed parity it didn't have. Fixed by adding the two missing bits
  (WriteAttributes, WriteExtendedAttributes) to Get-UntrustedWriteGrantees's mask, matching
  Lock-DarlingInstallTree's exactly - the comment is now literally true rather than reworded around the gap.
• L2 (Low) - "into Downloads" implied a general hole; Downloads is per-user. Fixed in both the
  -AcceptWritableExtraction help and the folder--Source inline comment: reworded to "a folder in your own
  profile ... writable by anything already running as you, elevated or not."
• L3 (Low) - the "ordinary users can already write" heading was wrong for an owner-only or unreadable-account
  finding. Fixed via Get-DarlingPreLockTrustedSidsForRerun: when the service is registered but its logon
  account can't be read at all, it fails with 1b2's own existing message (naming sc.exe qc) instead of
  reaching the generic writable-folder refusal.
• L4 (Low) - an untranslatable re-run account was dropped silently. Fixed in the same function: when the
  account name is readable but won't resolve to a SID (Resolve-DarlingServiceAccountSid returns $null -
  e.g. a gMSA an unreachable DC can't answer for), the script now fails naming the account and why, instead of
  later showing its own lock's grant as a stranger's raw SID.
• L5 (informational) - UAC's split token is not a security boundary. No code change, as specified. Added
  one sentence to the README next to the staging-folder guidance: trusting the installing admin's account also
  trusts that same admin's non-elevated session, since it's the same account. Listed here as accepted, per the
  ruling.

Test plan (round-1 fixes)

• Both scripts parse clean: [System.Management.Automation.Language.Parser]::ParseFile - no errors.
• Darling.Tests build - 0 Warning(s), 0 Error(s).
• DarlingInstallLocationTests - Total: 21, Failed: 0 (17 prior + 4 new: recursive walk catches a child
  grant/owner miss and stays silent on a clean recursive tree and a junction; a direct Administrators
  member enumerates; Get-DarlingPreLockTrustedSidsForRerun fails with a named reason for both L3 and L4
  instead of silently mis-trusting; the staging folder's ACL is protected).
• AST byte-identity test (ThePreLockWritableExtractionCheck_ShipsIdenticallyInTheInstallAndUpgradeScripts)
  extended to the three new shared functions (Get-LocalAdministratorsDirectMemberSids,
  Resolve-DarlingServiceAccountSid, Get-DarlingPreLockTrustedSidsForRerun) and stays green.
• Red-watch: neutered the recursive branch in Get-UntrustedWriteGrantees (if ($false -and $Recurse)) -
  the new recursion test went red (1 Failed); reverted, rebuilt, back to green (21/21).
• Performance measured on a real 1,661-file extracted pg-runtime tree: 1.6s (see M2 above).
• Full Darling.Tests suite - Total: 13120, Errors: 0, Failed: 0, Skipped: 631 (live/PG tests; no rig
  running in this lane - acceptable here since this PR touches scripts, one test class and docs, not the
  store).
• DARLING_TEST_PG rig was intentionally not stood up for this pass (script/docs/test-class-only change).
• Real install/upgrade cycle on a disposable box for the new -InstallRoot and staging-folder behavior -
  still out of this lane's hard fences (never register/start/stop a real Darling service); recommended
  before shipping, as round 1 already noted for the rest of this check.

Coverage gaps, said out loud rather than left silent:

• No dedicated test exercises the SHA256SUMS.txt-sidecar-folder-untrusted refusal path end to end (the
  function it calls, Get-UntrustedWriteGrantees -Recurse, IS covered directly). Time-boxed out of this pass;
  a good five-minute follow-up if anyone wants the belt-and-suspenders coverage.
• No dedicated end-to-end test drives upgrade-darling.ps1's M3 -InstallRoot block itself (it is inline
  top-level script flow, not an extractable function) - covered indirectly through
  Get-DarlingPreLockTrustedSidsForRerun and Get-UntrustedWriteGrantees -Recurse, which is the logic it
  composes.
• The top-of-file example path in install-darling.ps1 (C:\PerformanceMonitorDarling, which 1a itself refuses) is fixed in the coordinator pass below.

Coordinator pass (bf2f5bc, 8c00707, f2d14d6)

The staging lock left a third full-control grant on CI's elevated runner (f2d14d6). ProtectDarlingStagingFolder_LocksTheFolderToSystemAndAdministratorsOnly failed there with three explicit rules where two were expected. icacls /inheritance:r strips only inherited entries. A folder created under a parent that passes nothing on gets its creator's default DACL as explicit entries, and those survived. Protect-DarlingStagingFolder now runs /reset first. A new test builds such a parent so any machine reproduces the case; reproduced locally (3 rules), fixed (2), and red-watched (the new test fails with /reset removed).

The #2185 refusal sent people straight into the #4043 one. The service-account-profile refusal told users to
move the folder to C:\PerformanceMonitorDarling, and this PR's pre-lock check refuses exactly that folder: a
folder made directly under C:\ inherits write for every signed-in user. Every pointer to the install folder now
names C:\Program Files\PerformanceMonitorDarling:

• install-darling.ps1 (help, comments, and the [BUG] #2185 message, which now says to extract again from an elevated
  session and copy darling.json across);
• upgrade-darling.ps1's ImagePath comment;
• DarlingInstallPaths.DocumentedInstallDirectory (moved out of the Windows-only DarlingInstallLocation in
  36c6823), and the matching comment in DarlingWorker;
• Darling/README.md (the location section, upgrade-darling.ps1 examples, dotnet publish -o, sc create
  binPath with inner quotes, icacls);
• docs/retention-hold-runbook.md;
• docs/uat-onboarding.md (8c00707). 1.2 now verifies from an elevated PowerShell, 1.3 extracts into Program
  Files and says why a folder directly under C:\ is refused, 1.4 stays elevated so Notepad can save there, and
  every later command quotes the path (with & where a quoted exe is invoked).

install-darling.ps1 already registers the service with an inner-quoted binPath, so the space in the path is
safe. Test fixtures and changelog history that name the old folder are left alone: they model paths that real
installs used.

Verified: Darling.Tests builds with 0 warnings; 860 tests across the touched classes pass locally (94 after
8c00707: DarlingFileSecurityTests and DocCommentHygieneTests); both scripts parse with 0 errors. CI runs the
full suite.

Round-2 review (disposition, 36c6823 and 563d4a4)

The final review found no
High. This PR fixes all three of its items instead of filing them:

• M-a (Medium, fail-open): the walk skipped a folder it could not list. Get-UntrustedWriteGrantees (install
  and upgrade, still byte-identical) now passes -ErrorVariable to its recursive Get-ChildItem. Each folder
  that the walk cannot list becomes a finding, <path> (its contents could not be listed: UnauthorizedAccessException).
  The check refuses on it as it does on a write grant. The new theory
  ThePreLockWritableExtractionCheck_ReportsAFolderItCannotList_RatherThanSkippingIt covers both scripts. It
  denies the running account List Folder on one child and trusts that account everywhere else. With the fix
  reverted, it fails for both scripts.
• L-a (Low): the loader diagnosis still named C:\PerformanceMonitorDarling. DarlingToolExitCode now
  appends DarlingInstallPaths.DocumentedInstallDirectory. The constant moved out of the Windows-only
  DarlingInstallLocation into a new platform-neutral class, DarlingInstallPaths. The service also builds for
  Linux, and CA1416 flagged the new call site. Commit 8c00707 already fixed the UAT-doc half of L-a.
• y3 (coverage gap): the gates had no wiring tests.
  • TheSha256SumsSidecarIsReadOnlyAfterItsFolderPassesTheRecursiveCheck pins the order: the check, then its
    Fail refusal, then the read. It also pins that every use of $sums is the path, the existence test, or
    that one read.
  • TheWritableTreeCheck_RunsBeforeEitherScriptChangesAnything pins each script's tree check ahead of every step
    that changes state:
    • upgrade: the staging folder, the zip copy, the service stop, the lock, the backup, the extract or folder
      copy, and the service start
    • install: the 1b2 lock, the config copy, the pre-flight, the Event Log source, sc create, the config ACL,
      and the service start

The review's addendum on 8c00707 (comment)
found no High. Commit 563d4a4 fixes its nits in docs/uat-onboarding.md. The --export-viewer-config step now says to stay
elevated, because it writes into the install folder. The three commands that started with .\ and no cd of
their own now run the service exe by its full quoted path.

The addendum also noted a pre-existing Low. From UAT step 1.4 until install step 4b, BUILTIN\Users can read
darling.json, and under Program Files so can ALL APPLICATION PACKAGES. This PR does not change that. After step 4b, darling.json keeps its
NT AUTHORITY\INTERACTIVE read grant by design (#1792), because the viewer reads the file. #1792 accepted that
grant, and closing either window needs a design change that #1792 considered and did not make.

CI failure on f2d14d6

The failure was this PR's own new test. ProtectDarlingStagingFolder_StripsExplicitEntriesTheFolderWasCreatedWith
depended on the creator's default DACL to supply an explicit entry, and CI's runner supplies none
(explicitBefore=0). The test is now ProtectDarlingStagingFolder_StripsExplicitEntriesAlreadyOnTheFolder. It
plants BUILTIN\Users:(OI)(CI)M on the folder itself and asserts that the entry is there before the lock runs.
With /reset removed, the test fails with 3 rules left.

Verification

• Darling.Tests builds with 0 warnings.
• The full suite passes locally: 13,125 tests, 0 failed.
• Both scripts parse with 0 errors.
• With their fixes reverted, the M-a theory fails for both scripts, and the staging test fails.

CHANGELOG entry

SECTION: Fixed
ENTRY:

• Darling's install and upgrade scripts refuse a folder that ordinary users can already write to ([Darling install scripts refuse a writable-by-others install tree before the lock (#4043) #4050]) - When you extract the zip into a folder made directly under C:\, the folder inherits write access for every signed-in user. The Install and upgrade lock the Darling install folder, so an ordinary local user can no longer replace the service's binaries (#4034) #4038 lock stops writes only from the moment that it runs, so it could never catch a binary that was swapped before then. install-darling.ps1 and upgrade-darling.ps1 now check every file in the tree, and the existing install, before they change anything. They name who can write to the tree, and each folder that they cannot look inside. Then they refuse, unless you pass -AcceptWritableExtraction for a deliberate dev loop. An upgrade copies its zip to an administrators-only folder and verifies the zip there before it extracts anything. Every doc and message now names C:\Program Files\PerformanceMonitorDarling as the install folder. This includes the refusal for a folder inside a user profile, which used to send people to a folder that this check refuses.
  REF:
  [Darling install scripts refuse a writable-by-others install tree before the lock (#4043) #4050]: Darling install scripts refuse a writable-by-others install tree before the lock (#4043) #4050

Handoff

• Part 2 static audit is done and written above; narrowing itself is deliberately not attempted here.
  If a future lane picks it up: resolve the BYO-Postgres log-hash-key directory site first, and add
  pg-runtime, pg-runtime-prev, and (if kept) the BYO key directory to the grant, RX on everything
  else, verified with an actual install/upgrade/restart cycle.
• Mcp/DarlingWebHostService.cs's ContentRootPath write behavior was not independently verified
  (believed read-only, ASP.NET static content root) - worth a five-minute look before anyone relies on
  the audit table as exhaustive.
• The historical example paths in Darling/README.md are rewritten too; see the coordinator pass below.
• A leftover scratch file .git-commit-msg-4043.txt sits untracked in this lane's worktree (used to pass
  the commit message safely); it was never staged or committed and can be ignored/deleted with the
  worktree.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GdmA4ND1wLSqA91ax1m4xv

[erikdarlingdata]

Security review, round 1 (#4043 pre-lock ACL check)

Scope: install-darling.ps1 step 1a, the folder -Source check in upgrade-darling.ps1, the two new helpers, the README and the tests. Read-only on source. Verified by loading the PR's functions via the AST and running them read-only against real folders on a Windows 11 box (non-elevated, C: only, no Darling service registered). DarlingInstallLocationTests: Total: 17, Failed: 0.

High

H1. The zip -Source is not covered "regardless of who could write to the folder it sits in". See upgrade-darling.ps1:101-103 and :1364-1365.

• When -Sha256 is not passed, the expected hash comes from SHA256SUMS.txt in the zip's own folder (:1329). Anyone who can write that folder controls both the zip and the value it is checked against.
• Even with -Sha256, the zip is hashed at :1351 and read again by Expand-Archive at :1659, after the stop, the process checks and the backup. The file is not held open in between, so a zip that others can write is not proven to be the file that gets extracted.
• The new folder--Source refusal (:1390) tells the operator to "pass the zip itself as -Source". That routes users off the checked path onto this unchecked one, and often into the same writable folder.
• Fix, in this PR:
  • Run Get-UntrustedWriteGrantees on the zip file and its folder, under the same -AcceptWritableExtraction override.
  • Accept SHA256SUMS.txt only from a folder that passes the check. Otherwise require -Sha256.
  • Better still, copy the zip into a fresh private staging folder (admin-only ACL) first, then hash and extract that copy. That also closes the gap between hashing and extraction.
  • Correct the two comments.

Medium

M1. The documented upgrade procedure is now refused, and the docs still recommend the writable pattern this issue is about.

• README.md:186-190 gives Expand-Archive ... -DestinationPath C:\staging\3.5.1 and then -Source C:\staging\3.5.1. A folder under C:\ inherits Authenticated Users: Modify, so the new check refuses exactly this command.
• upgrade-darling.ps1:1316 (the self-overwrite refusal) tells operators to use C:\staging\<version>, and :55 and README :199 say the staging folder is "not optional".
• Every operator following the docs hits the new refusal, and the docs also have them run upgrade-darling.ps1 elevated from a folder others can write.
• The PR body's handoff called these examples "unrelated"; they are directly affected.
• Fix, in this PR: change the example and both messages to a staging location only administrators can write (for example under C:\Program Files\), or to the verified-zip path once H1 is fixed.

M2. Checking only the root and the exe misses the case where the root was re-permissioned after extraction, and the lock then erases the evidence.

• The comment at install-darling.ps1:105-107 (same at upgrade-darling.ps1:140-142) says a recursive pre-lock walk would re-do the lock's verification "for no benefit". That is not accurate.
• Lock-DarlingInstallTree runs icacls /setowner Administrators /T (install-darling.ps1:472) before its walk (:488) and resets explicit grants as it goes. So neither step ever reports a child file owned by a non-admin account. That is the most direct sign that someone other than the admin created or replaced it.
• The root and exe alone stop showing any finding once the root's permissions have been fixed by hand or re-inherited after a move, while the files under them keep their old owners.
• Holding only the inherited Modify grant gives neither WRITE_DAC nor WRITE_OWNER, so that holder cannot edit the root's or exe's DACL. They do own anything they create, and that ownership is only visible if it is read before the lock.
• Fix:
  • Make 1a a read-only recursive walk that reports any owner outside the trusted set and any explicit write grant outside it, before 1b2. The shipped tree is small, and the lock already walks it.
  • Add one line to the refusal: repairing this folder's permissions in place cannot undo a replaced file; delete it and extract the zip fresh.

M3. upgrade-darling.ps1 never checks -InstallRoot, but install-darling.ps1 refuses the same folder.

• An install made before Install and upgrade lock the Darling install folder, so an ordinary local user can no longer replace the service's binaries (#4034) #4038 has been writable by Authenticated Users for its whole life. A re-run of install-darling.ps1 over it is refused at 1a. upgrade-darling.ps1 locks it (:1533) and overlays it with no finding at all.
• The overlay replaces the files the new build ships. It does not vouch for anything else in the tree: the stale-file list only covers what an earlier manifest recorded, and pg-runtime is extracted in place.
• README :10 ("an older install is closed at its next upgrade") describes future writes only.
• Fix: before the lock, run the check on -InstallRoot. If it still carries broad write grants (never locked), warn loudly, or refuse without the override, and say what to verify: -RemoveStaleFiles, and a fresh pg-runtime.

Low

• L1. install-darling.ps1:109 and upgrade-darling.ps1:144 say the mask "mirrors Lock-DarlingInstallTree's". It omits WriteAttributes and WriteExtendedAttributes (compare :486). Neither can change file content, so there is no impact. Fix the comment, or add the bits for parity.
• L2. upgrade-darling.ps1:105 and :1367 say a folder extracted "into Downloads" inherited Authenticated Users: Modify. Downloads is per-user (SYSTEM, Administrators and the user only). The PR's own function passes it on this box. Reword.
• L3. The heading "Ordinary users can already write" (install-darling.ps1:603, upgrade-darling.ps1:1383) is misleading in two cases:
  • An owner-only finding, such as another admin's personal account.
  • A re-run where Get-DarlingServiceLogonName returns $null. The lock's own grant to the service account is then flagged, and the advice (re-extract under Program Files) is wrong. Before this PR, 1b2 failed here with a clear message (:750).
  • Fix: on $existing with an unreadable logon name, fail with 1b2's message. Word owner findings separately.
• L4. A re-run account that will not translate (a gMSA with the domain controller unreachable) is dropped silently (Get-DarlingPreLockTrustedSids, catch { }). The service's own grant is then refused and shown as a raw SID. Fix: say so in the refusal. The lock itself would fail on the same account at 1b2, so this changes only the message.
• L5 (informational). Trusting the installing admin's user SID also trusts that admin's non-elevated session. This is outside the stated non-admin threat model; noted for completeness.

Clean

• Parity:
  • Get-UntrustedWriteGrantees, Get-DarlingPreLockTrustedSids, Get-DarlingServiceLogonName and Lock-DarlingInstallTree are identical across both scripts (AST extent compare, one definition each).
  • Re-run trust is wired the same way in both: Get-Service, then Get-DarlingServiceLogonName, then Get-DarlingPreLockTrustedSids.
• CREATOR OWNER/GROUP (S-1-3-0..3) exclusion is sound.
  • These SIDs never appear in a token, so an ACE naming them grants nothing on an existing object.
  • Creating a child needs rights on the parent, and those are already checked.
  • The owner check stands in correctly for OWNER RIGHTS on the root and exe. A null owner or an unreadable ACL fails closed.
• SIDs: compared by value, and an untranslatable SID is still flagged (shown raw).
• Deny ACEs: ignored, so they can only cause a false refusal, never a pass.
• Groups: grants to any group other than the four exact trusted SIDs (plus the service account) are flagged, including groups the installer belongs to.
• False refusals, checked on real folders:
  • 79 of 79 folders under C:\Program Files and C:\Program Files (x86) passed, as did Downloads and Documents.
  • Only C:\ (Authenticated Users) and C:\ProgramData (Users) were flagged, both correctly.
  • An owner of BUILTIN\Administrators or the admin user passes.
  • Not testable here: a second volume (none on this box), a domain admin, a gMSA, and a real locked install (the re-run case is covered by the new test with LOCAL SERVICE).
  • By reasoning: a GPO-pushed Domain Admins ACE would be flagged. That is a Low false refusal and is not in the defaults.
• Messages: only admin-chosen paths ($PSScriptRoot, -Source), translated account names and exception text are echoed. No text a non-admin controls reaches the console.

[erikdarlingdata]

Security review, round 2 (final) -- #4043 pre-lock ACL check

Reviewed through git at bf2f5bc5 (delta 1835c30b..bf2f5bc5, whole PR origin/dev...bf2f5bc5). Functions were loaded from the extracted scripts via the AST and exercised read-only on a real Windows 11 box under both Windows PowerShell 5.1 and PowerShell 7. No service touched; no ACL changed outside my own scratch folders.

Round-1 findings -- all verified CLOSED

• H1 (zip staging). upgrade-darling.ps1:1521-1583: the zip is copied into New-DarlingProtectedStagingFolder (fresh GUID-named dir under %TEMP%, icacls /inheritance:r + SYSTEM/Administrators-only Full + Administrators owner, Protect-DarlingStagingFolder at :1394), and only the staged copy is hashed (Get-FileHash -LiteralPath $stagedZip at :1574) and extracted ($Source = $stagedZip at :1583, Expand-Archive -LiteralPath $Source at :1883). Cleanup is a finally (:2026-2033) wrapping staging-creation-to-end; I confirmed exit (how Fail terminates) runs an enclosing finally on both editions, so every Fail path cleans up. Without -Sha256, SHA256SUMS.txt is trusted only when Get-UntrustedWriteGrantees $sourceRoot -Recurse passes (:1535), else -Sha256 is required (:1566). The staged-copy hash closes the original hash-then-reread TOCTOU. Solid.
• M1 (docs). README.md:186-197,256 now stage under C:\Program Files\PerformanceMonitorDarling-staging; the self-overwrite refusal (upgrade:1505) and both .PARAMETER help blocks match.
• M2 (recursive walk). -Recurse (upgrade:181, install:140): root checked including inherited; every descendant checked for untrusted owner and explicit write grants; a reparse point is reported and not descended. Verified on both editions: Get-ChildItem -Recurse does not follow a junction into its target, and a self-referential loop junction terminates immediately (no runaway). Direct BUILTIN\Administrators members are trusted via WinNT ADSI (Get-LocalAdministratorsDirectMemberSids), which returns null (trust nobody, fail closed) when the group cannot be bound; I confirmed a localized/absent group name yields null.
• M3 (-InstallRoot). upgrade:1433-1461 runs the recursive check on -InstallRoot before the pre-copy Invoke-UpgradeTreeLock (:1769), gated by -AcceptWritableExtraction.
• L1 mask now includes WriteAttributes/WriteExtendedAttributes (upgrade:211). L2 Downloads wording reworded. L3/L4 Get-DarlingPreLockTrustedSidsForRerun (upgrade:316) fails with a named reason when the existing service account cannot be read (the 1b2 message) or will not resolve to a SID.

New -- introduced by the round-2 fixes

M-a (Medium) -- the recursive walk fails OPEN on a directory it cannot enumerate. Get-UntrustedWriteGrantees, upgrade-darling.ps1:212 (identical install-darling.ps1:171): foreach ($child in @(Get-ChildItem -LiteralPath $path -Recurse -Force -ErrorAction SilentlyContinue)). The SilentlyContinue with no -ErrorVariable swallows enumeration errors, so any subtree the elevated walker cannot list is skipped silently and the tree is reported clean. I reproduced this: a child directory whose ACL denies Administrators the list right causes a planted Everyone:Modify file beneath it to be dropped from the findings on both editions. This is a fail-open in a security gate, the same class the per-object Get-Acl catch (:195, the "permissions could not be read" line) already guards against, just not applied to the enumeration. Not rated High: to make a subtree unlistable to Administrators an attacker needs WRITE_DAC, i.e. ownership of that subtree, and a non-admin-owned directory is independently flagged by the owner check on the same walk. So the silent skip sits behind a finding that already fires, and I could not construct an independently-exploitable bypass in the non-admin threat model. It remains a real fail-open worth closing. Fix: add -ErrorVariable childErrors (or a per-directory try/catch around Get-ChildItem) and append one "contents could not be listed" finding per unreadable directory, mirroring the per-object catch. Same edit in both scripts (the byte-identity test holds it).

L-a (Low) -- sibling docs still steer operators into the folder 1a now refuses. M1 fixed README.md and both .ps1 headers (now C:\Program Files\PerformanceMonitorDarling), but docs/uat-onboarding.md still runs Expand-Archive -DestinationPath C:\PerformanceMonitorDarling (:104) then install-darling.ps1 (:208), 15 references, and DarlingToolExitCode.cs:204 tells operators to reinstall to a machine-scoped path such as C:\PerformanceMonitorDarling. Under this PR an operator following UAT hits the 1a refusal. Fail-SAFE (the script correctly refuses), so this is a usability/doc regression, not a vuln, but it is the exact M1 defect surviving in the sibling files the PR body scoped out. Low.

y3 coverage gap -- real but low, and cheaply closable

The SHA256SUMS-sidecar gate (upgrade:1527-1561) and the M3 inline -InstallRoot block (:1433-1461) are top-level script flow; only their composed functions (Get-UntrustedWriteGrantees -Recurse, Get-DarlingPreLockTrustedSidsForRerun) are tested. The wiring is not: a regression that dropped -Recurse, moved the block after the lock call, or inverted the -not $AcceptWritableExtraction gate would pass every existing test. Low probability, but it is the seam the fix lives in. Smallest closer: a source-order/string-index assertion in DarlingInstallLocationTests (same technique as LocationGuard_RunsBeforeAnythingIsInstalled) that, over upgrade-darling.ps1, asserts the Get-UntrustedWriteGrantees $InstallRoot ... -Recurse and Get-...SidsForRerun calls each appear, gated by the -not $AcceptWritableExtraction check, and BEFORE the pre-copy lock. Deterministic, no PG rig, ~5 lines.

Checked and clean

• Staging race / plant / junction at the staging path: New-DarlingProtectedStagingFolder uses an unpredictable per-run GUID under an elevation-scoped %TEMP% (admin+SYSTEM only); pre-creation or a planted reparse point at the path is infeasible, and the ~10 ms between New-Item and icacls /inheritance:r inherits an admin-only parent DACL, so nothing widens it. The finally cleanup Remove-Item -Recurse on the staging dir (which only ever holds the copied zip) is junction-safe (verified: -Recurse on a junction removes the link, not the target).
• Zip TOCTOU: a swap of the original zip before Copy-Item yields a staged copy that then fails the -Sha256/sidecar check. That is the H1 fix working as intended.
• -AcceptWritableExtraction scope: gates the two ACL writability checks (InstallRoot, folder--Source) only; the zip hash and the sidecar-folder writability check still fire. Fail-safe, not a bypass.
• Message echo: findings echo tree file names to the console only (Write-Host), no SQL/HTML/shell sink; no injection surface.
• ADSI membership read is the local Administrators group; nested/domain members are named, not expanded, and an orphaned-SID member cannot throw (objectSid read, no name lookup).
• Expand-Archive on the staged copy: the inbox module applies its own StartsWith-boundary zip-slip guard and rejects device-path prefixes; extraction is into the already-locked -InstallRoot.
• Byte-identity of the five shared functions across both scripts is asserted and green.

Net: functionally sound and a clear improvement over round 1. One fail-open (M-a) worth a follow-up; the rest is Low/doc/coverage. No independently-exploitable High found.

[erikdarlingdata]

Security review, round 2 -- addendum: bf2f5bc5..8c007072 (UAT doc sweep)

Scope as asked: git diff bf2f5bc5..8c007072 (docs/uat-onboarding.md + one test comment). I also looked at f2d14d62 (7 lines, now the branch head) because it changes the staging folder my round-2 comment called clean; see the end.

Verdict: nothing in the swept walkthrough sends a user into a refusal or runs an elevation-requiring command unelevated. No High. One pointer outside the doc still sends users into the #4043 refusal (below).

Refusals

• None in the walkthrough. 1.3 now extracts to C:\Program Files\PerformanceMonitorDarling, which passes both 1a (Darling install tree: verify shipped binaries after the lock, and narrow the service account's Modify to pg-runtime (#4038 review residuals) #4043: Program Files grants ordinary users read/execute only) and the [BUG] #2185 location check (machine-scoped, not a profile or UNC path). No C:\PerformanceMonitorDarling reference is left in the doc, and it has no upgrade or staging flow, so the upgrade self-overwrite and folder--Source refusals cannot be reached from it.
• Still open, outside the doc (the remaining half of my L-a, Low): DarlingToolExitCode.cs:204, the [BUG] Managed Postgres bootstrap reports a raw Win32 status and an empty Output on loader failures #2186 initdb loader-failure diagnosis, still says reinstall to a machine-scoped path such as C:\PerformanceMonitorDarling. That folder is the one 1a refuses. It is unchanged at 8c007072 and at f2d14d62. The fix is a one-line string change; no test pins that text (the C:\PerformanceMonitorDarling\pg-runtime\pgsql\bin in DarlingToolExitCodeTests.cs:114 is the input fixture path, not this advice).

Elevation

• 1.2, 1.3 and 1.4 are now explicitly elevated, and all three need it: the extract into Program Files, the Copy-Item of the sample, and Notepad saving darling.json. Starting elevated at 1.2 also keeps $zip in the same session for 1.3.
• 1.6 (install-darling.ps1), --configure-network, --configure-firewall (both places), --enable-web, --enable-mcp and --print-web-token are all labeled elevated.
• --encrypt-password (DPAPI LocalMachine) and --test-connection (read-only) do not need elevation. They run in the same elevated session anyway.
• Nit: --export-viewer-config (:339) writes viewer-config\ beside darling.json, which is inside Program Files, so it needs elevation. It works because the reader is still elevated from the preceding step (the one that says to stay elevated for --configure-firewall). The block itself is not labeled; one word fixes it.

Quoting

Every path in a command block is quoted, and & is used wherever a quoted exe runs (:171, :696, :813). Two unquoted paths remain, both outside command blocks: :241 is sample log output, and :282 is inline prose. :282 would need & "..." if someone pasted it into PowerShell (nit). The blocks at :329, :339 and :685 call .\PerformanceMonitor.Darling.Service.exe with no cd of their own. That works in one continuous session and fails with "not recognized" in a fresh shell. Not a security issue.

A security improvement this sweep makes, beyond doc hygiene

The old walkthrough ran PerformanceMonitor.Darling.Service.exe elevated at 1.4 (--encrypt-password) and 1.5 (--test-connection) from C:\PerformanceMonitorDarling, a folder ordinary users could write. That happened before install-darling.ps1 step 1a could check the folder, so on the UAT path a swapped exe ran with the admin token before the #4043 check ever ran. Under Program Files the folder is admin-only from the moment it is extracted, which closes that gap.

Pre-existing, unchanged by this commit (Low, informational)

From 1.4 until install step 4b hardens it, darling.json holds an encryptedPassword (DPAPI LocalMachine, entropy constant published in the repo), and every local user can read it: children of Program Files get BUILTIN\Users:(GR,GE). Under Program Files, ALL APPLICATION PACKAGES (AppContainer processes) can read it too. Users could read it at the old location as well (C:\ grants Users:(OI)(CI)(RX)), so this is not a regression. If you want to close the window: a protected ACL right after the 1.4 Copy-Item, or a doc note to finish 1.4 to 1.6 in one sitting.

The DarlingFileSecurityTests.cs change is comment-only, and what it says is correct (Program Files passes Users read to its children).

f2d14d62 (beyond the requested range): correct hardening, no new issue

It adds icacls /reset before /inheritance:r in Protect-DarlingStagingFolder. I verified it on a real folder. Under a parent that passes nothing on, a new child gets the creator's default DACL as explicit entries (here SYSTEM, Administrators and the creating user). /inheritance:r alone left the creator's entry in place. With /reset first, the result is exactly SYSTEM:Full + Administrators:Full. /setowner is still a separate step and succeeds elevated. On the fresh, empty folder, /reset cannot widen access: it either inherits from the admin-only parent or produces an empty (not null) DACL. This makes the SYSTEM-and-Administrators-only claim in my round-2 comment hold under any parent.

Open items after this addendum: M-a (Medium: the walk fails open on a directory it cannot enumerate), unchanged, and the DarlingToolExitCode.cs:204 string (Low).

[erikdarlingdata]

Round-2 disposition: commits 36c6823 and 563d4a4 fix every item from the review and its addendum, instead of filing them.

• M-a: both scripts now report a folder that the walk cannot list as a finding.
• L-a: the loader diagnosis names the documented install folder from the one constant.
• y3: new wiring tests pin the SHA256SUMS.txt gate and the order of the check in both scripts.
• Addendum nits: three UAT commands now run the service exe by full path, and the export step says to stay elevated.

The CI failure on f2d14d6 came from this PR's own staging test. The test relied on a default DACL that CI's runner does not supply. It now plants its entry directly. Each new test fails with its fix reverted, and the full Darling suite passes locally. The darling.json read window is pre-existing and falls under #1792. The PR body has the details under "Round-2 review".