Security: firewall command injection, darling.json ACL, and MCP Host-header guard (#1646, #1647, #1648)

[erikdarlingdata]

Closes #1646, #1647, #1648. Four findings from the 2026-07 maintenance security review (#1643), all in the same blast radius: what a local user, or a browser running on the monitoring host, can reach.

#1646 (HIGH) - PowerShell command injection via allowFrom

ReconcileEndpointFirewallAsync read web.network.allowFrom (or the MCP equivalent) out of darling.json and interpolated it unquoted into the PowerShell -Command string that creates the scoped firewall rule. The only gate was string.IsNullOrWhiteSpace. It was the one BuildFirewallEnableCommand caller that never parsed the value first - every other call site passes a canonicalized IPNetwork.ToString(), and --configure-network validates before writing - and ToggleEndpointAsync loads config with DarlingConfig.Load, which deserializes and never calls Validate(), so nothing upstream caught it either.

Failure scenario closed: an allowFrom of 10.0.0.0/8; <command> executed that command. Elevated (the documented way to run these verbs) it ran directly; non-elevated, the code printed the fully-injected command and told the operator to paste it into an elevated PowerShell - same outcome, via the human. Whoever can write darling.json, or set DARLING_CONFIG (honored with no path restriction), got code execution in the elevating operator's context.

Fix, two layers - either alone leaves a sharp edge:

1. The verb parses allowFrom as a CIDR (ClassifyAllowFrom) and passes the parser's canonical output, so nothing unvalidated is carried through even on the valid path. On a parse failure it refuses: no firewall change, and deliberately no printed command, because a handoff executes the injection just as surely as running it.
2. BuildFirewallEnableCommand / BuildFirewallDisableCommand now single-quote and escape every value they interpolate (PowerShell escapes ' by doubling), so the builders are safe regardless of what any present or future caller hands them.

Real rule names contain no quote, so the emitted commands are byte-for-byte unchanged. Worth noting: IPNetwork.TryParse masks host bits rather than rejecting them (192.168.1.5/24 becomes 192.168.1.0/24), verified empirically, which is exactly why the canonical form and not the caller's string is what reaches -RemoteAddress.

#1647 (HIGH) - darling.json was never ACL-hardened

It holds every monitored server's encryptedPassword, the MCP bearer token, the web dashboard access token, and in BYO mode the store connection string - all DPAPI LocalMachine scope with an entropy constant published in this open-source repo. Anything that can read the file can unprotect the lot. That is by design; DarlingFileSecurity says so itself ("the file ACL is therefore the real access boundary"). The config just never got that ACL: every harden call site targeted the credential files under the data directory, while the config sat beside the binary.

Failure scenario closed: the README's recommended install extracts the zip to C:\PerformanceMonitorDarling, and a folder created directly under C:\ inherits BUILTIN\Users: Read & Execute from the root DACL. Any local unprivileged user could read it, decrypt every SQL Server password, and lift the tokens that unlock the MCP write surface and the web dashboard - and, where the install directory was user-writable, plant the payload for #1646.

Fix: the service hardens the resolved config path at startup with the same posture the admin/viewer credentials get (allowInteractiveRead: true - the Viewer and the CLI verbs run as the interactive operator and must still read it); install-darling.ps1 applies the identical ACL right after service creation, which is when the NT SERVICE virtual account first has a SID to grant; and a file that is still readable by Users/Authenticated Users/Everyone after the attempt logs Critical rather than crashing a monitoring service over a permissions problem. The new IsReadableByOrdinaryUsers check keys on ReadData specifically, not the composite Read mask, so a metadata-only ACE does not cry wolf.

#1648 (MEDIUM, plus Lite parity) - MCP hosts had no Host-header allowlist

The web dashboard got this guard in #1576, with the reasoning written into the code: a loopback surface is tokenless, so a browser on the host that loads attacker content can be DNS-rebound to 127.0.0.1 and read/write the whole surface same-origin. That applies identically to the MCP host at :5152 in its default loopback configuration, which installs no auth middleware at all - and that surface is no longer read-only, having gained DarlingMcpCustomViewTools (view CRUD), DarlingMcpServerAdminTools (add_servers / remove_server), and alert-config writes.

Failure scenario closed: an operator browsing from the Darling host loads an attacker page; the attacker's domain re-resolves to 127.0.0.1 and their JavaScript POSTs to the MCP endpoint, reading the monitoring store and calling the write tools. No token needed. The application/json content type does not save it - under a rebind the browser treats the request as same-origin, so no CORS preflight applies, and ModelContextProtocol.AspNetCore 1.4.0 does not add the check itself (per the MCP spec it is the application's job).

Fix: the decision moves into a shared HostHeaderGuard in PerformanceMonitor.Common - the library both apps already reference, so there is one implementation, not a per-app copy - reachable from DarlingHostBinding where the rest of the hosts' bind/auth helpers live. It is installed as the first middleware, in both bind modes, on Darling's MCP host and on Lite's, mirroring DarlingWebHostService.cs:399-408. The web host's behavior is byte-for-byte unchanged: its IsAllowedHost is now a forwarder and its existing DarlingWebAuthTests matrix is untouched, which is the proof.

Lite is loopback-only, single-user, and holds no service-account privilege, so its exposure is informational. Fixed in the same change anyway - a guard that exists in one app's copy and not the other's is the drift this codebase keeps paying for.

Lite (LOW) - unescaped export path

DataImportService.FlushOldDatabase built COPY (...) TO '{path}' by interpolating the import folder raw, while every sibling COPY (ArchiveService, ParquetCompaction) routes the same value through DuckDbInitializer.EscapeSqlPath.

Failure scenario closed: an apostrophe is legal in a Windows path (C:\Users\O'Brien\...), so it closed the SQL literal, the COPY failed to parse, and the per-table catch swallowed it as a warning - the import reported success having silently flushed nothing. Verified against a real DuckDB: the unescaped form raises Parser Error: syntax error at or near "Brien" where the escaped form writes the file.

Tests

Added to the existing suites, and they pin the things that actually regress:

• ClassifyAllowFrom - the accept/refuse matrix, and a property test that whatever the verdict accepts re-parses as a CIDR and carries no shell metacharacter. That makes "a non-CIDR allowFrom never reaches a firewall command" true by construction rather than by enumerating the payloads someone thought of.
• The builders - any hostile -RemoteAddress stays inside one balanced single-quoted literal with its own quotes doubled, plus an exact-string pin that real inputs produce the unchanged command.
• HostHeaderGuard - the full accept/reject matrix in both modes, in both suites, plus a forwarders-agree check so the web host cannot diverge.
• The wiring - Security: MCP hosts lack a Host-header allowlist (DNS rebinding) - Darling + Lite #1648 was a wiring omission, not a logic bug, so a pure-function test would have passed on the vulnerable build. New HostHeaderGuardTests in both suites parse the shipped host sources (copied beside the test binary by the csproj, the same Link+copy delivery ProvisionRolesAclDriftTests uses) and assert the guard is the first _app.Use after builder.Build(), sits ahead of the if (networkMode) gates, and precedes MapMcp. Negative-checked: removing Lite's middleware makes the test fail as intended.
• The ACL - IsReadableByOrdinaryUsers against real Windows DACLs: an explicit Users read ACE is caught, hardening clears it, Authenticated Users and Everyone are caught too, and a metadata-only grant or a deny ACE is correctly ignored.
• The export - end-to-end against a real DuckDB under a directory whose name contains a quote, not a string assertion about the generated SQL, because the failure being pinned is a silent zero rather than an exception.

Results: Darling.Tests 3057 passed / 0 failed / 147 skipped (the DARLING_TEST_PG-gated live tests). Lite.Tests 1465 passed / 0 failed / 0 skipped. Installer.Tests deliberately not run (it touches live SQL Servers).

Build: 0 warnings, 0 errors on a clean --no-incremental build of the whole solution.

origin/dev moved while this was in progress - the maintenance PR (#1645, from the same #1643 review that filed these issues) merged in, and it is what took the build to zero warnings. This branch was cut from the older dev, so dev has been merged in here and everything re-verified on top of it: the zero-warning figure above, and both test runs below, are against the merged result, not the older base. Merge was conflict-free, including the CHANGELOG (both entries are present under [Unreleased] -> Fixed).

Notes

• Not done, deliberately: ParquetCompaction.EscapeSqlPath stays a private local copy rather than forwarding to DuckDbInitializer. That file's header states it is compiled into tools/CompactionRepro as well and must stay free of project dependencies - confirmed by CompactionRepro.csproj line 23, which <Compile Include>s it. Consolidating would break the reproducer. A comment now records why, and the source pin covers all three call sites regardless.
• Not done, out of scope: Security: darling.json holds recoverable secrets and is never ACL-hardened #1647 suggests eventually moving the tokens and encryptedPassword values into the already-hardened credential directory, which would remove the dependency on install-location ACLs entirely. That is a config-format migration, not a hardening pass.
• Pre-existing flake observed (unrelated to this change): ViewerWave2Tests.BlockedRow_EventTimeLocal_FormatsTheStoredUtcInLocalTime failed once across four full Darling runs and passes in isolation. Cause is shared mutable global state - ViewerTimeHelper exposes a settable static UTC offset and display mode that ForDisplay reads, so a test class flipping either can race this one under xUnit's parallel collections. Left alone here; fixing it means choosing a collection-serialization strategy for the existing Viewer suite, which does not belong in a security PR.

🤖 Generated with Claude Code