fix(security): pin icacls to its absolute System32 path at both ACL sites (BACKLOG #1769)

[wshallwshall]

DO NOT MERGE BEFORE PR #1132 LANDS. This PR requires _system_exe() from
#1132 (branch
fix/service-absolute-elevated-paths-1680, BACKLOG #1680). That symbol is not on main --
git grep -n _system_exe origin/main -- messagefoundry/ returns nothing.

Nothing automated catches this. This PR does not conflict with #1132, it depends on a symbol
#1132 introduces. That shape is invisible to the merge queue, to git merge-tree, and to branch
protection: if this lands first the failure is an ImportError at collection, not a merge conflict.
A human reading this paragraph is the only gate. It is kept as a draft for the same reason.

To unblock, after _system_exe is on main: mark this PR ready for review. Do not re-run the
existing checks -- a re-run replays the recorded merge SHA, which predates #1132. Marking ready
fires a fresh pull_request event, and ci.yml triggers on a bare pull_request: with no ref:
on any checkout, so the new run gets a recomputed refs/pull/N/merge containing #1132.

Do not rebase this branch. Verified with git merge-tree against a synthetic squash-merge of
#1132 onto main: this branch into squashed-#1132 is exit 0, and squashed-this-branch into #1132 is
exit 0, both producing the identical tree. The two PRs touch disjoint files. Order does not matter
once #1132 is in.

The required set is the merge bar, not the correctness bar. The two new tests here are
Windows-gated (os.name == "nt" must be real), so test (windows-2022, py3.14) and test (windows-2025, py3.14) are the only legs that execute them -- 15/15 required green without
those two legs read does not cover this change. Also read repo-scan (bandit, semgrep,
crypto-inventory, forbidden-content) for the reworded nosec sites.

---

What

Two subprocess.run calls invoked icacls by unqualified name. Windows resolves an unqualified
program name through a search path that reaches the caller's working directory, so an icacls.exe
planted in the directory an operator happened to launch from would be run instead. Neither call
elevates, but both are security-relevant, and both carried a comment asserting "icacls is a fixed
system tool" -- true only once the path is pinned.

Site	What it does	What a planted icacls.exe would do
messagefoundry/store/store.py _secure_file	Writes the ACL restricting the store DB and the DPAPI key file	Exit 0 and the hardening is a silent no-op: the code logs only on a non-zero exit, so the PHI-adjacent file keeps its inherited (possibly broad) ACL and nothing reports a problem
messagefoundry/auth/trust_anchors.py dacl_is_owner_only	Parses icacls output to decide whether a TLS trust anchor is owner-only	Print a clean DACL and a group-writable anchor becomes an accepted one

Both now call _system_exe("icacls.exe") from service_status, the stdlib-only neutral leaf, rather
than growing a third resolver. Both comments now state the pin instead of asserting a property the
code did not yet have.

BACKLOG #1769, allocated and claimed in the maintainer-internal vault. docs/BACKLOG.md is a stub
here and is deliberately untouched.

Layering, verified rather than assumed

service_status.py imports only stdlib (asyncio, ctypes, os, re, subprocess, sys), so
neither new import can create a cycle. store/store.py already reached it transitively through
config.settings. tests/test_dependency_boundaries.py is the only import-graph gate in the repo;
it forbids the engine packages only fastapi, pyside6, messagefoundry.api and
messagefoundry.console, and auth/ is not in its engine-package list at all. It passes here.

Importing a private name across packages already ships in this codebase -- api/app.py imports
_secure_file from store.store, and transports/file.py imports _content_matches_declared from
parsing.sniff -- so _system_exe is left private.

Tests

One per site. Each asserts argv[0] is absolute, is still icacls.exe, and sits in the directory
_system_dir() reports.

The third assertion compares against _system_dir() rather than a literal System32 because
GetSystemDirectoryW answers SysWOW64 to a 32-bit process, and that is the correct system
directory there; a literal would red while the pin was perfectly correct. _system_dir's own
behaviour is tested beside it in tests/test_service_control.py (#1132), so this does not duplicate
it.

Fail-first was observed, not derived. With only the two source hunks reverted to origin/main
and the tests unchanged, both fail:

E       AssertionError: icacls must be pinned to an absolute path, got 'icacls'
tests\test_store.py:135: AssertionError
E       AssertionError: icacls must be pinned to an absolute path, got 'icacls'
tests\test_trust_anchors.py:187: AssertionError
2 failed, 85 deselected

Two further details:

1. Each test asserts the call was recorded at all before asserting anything about it. Without
   that guard, a run where subprocess.run is never reached passes over nothing, which is
   indistinguishable from passing.
2. They run where os.name == "nt" is real, not monkeypatched, matching the existing note in
   tests/test_store.py (forcing it makes pathlib instantiate WindowsPath and crash pytest on
   Linux). So they are exercised on the windows-2022 / windows-2025 legs and skip on ubuntu.
   os.path.isabs is correct only because of that gate:
   posixpath.isabs("C:\\Windows\\System32\\icacls.exe") is False, so a version of this test that
   faked the platform on Linux would fail after the fix. Measured.

The two pre-existing assertions that pinned the bare name (argv[0] == "icacls" and the exact-argv
list) now check the basename, leaving the pin to its own dedicated test.

Checks run

ruff check (passed), ruff format --check (passed), mypy messagefoundry strict (no issues, 274
files), pytest tests/test_store.py tests/test_trust_anchors.py (86 passed, 1 skipped -- the
POSIX-only test, correct on Windows), and tests/test_dependency_boundaries.py tests/test_threat_model_doc_drift.py tests/test_phi_at_rest_inventory.py tests/test_tooling_partition.py (62 passed, 89 skipped). All pre-commit hooks passed at commit,
bandit included.

Because _system_exe is not on main, the local run needed
messagefoundry/service_status.py temporarily overlaid from #1132's branch. That overlay is not
part of this commit -- the committed diff is exactly the four files above. It is self-contained:
main's _sc_path has no consumer outside service_status.py itself, and main's service.py does
not import service_status.

Legs a local run cannot report, which must be read here after the fresh run: test (windows-2022, py3.14) and test (windows-2025, py3.14) -- the new tests are Windows-gated and skip
on ubuntu, so those two legs are the only ones that execute them. Also repo-scan (bandit, semgrep,
crypto-inventory, forbidden-content), CodeQL, and diff-coverage.

A full-suite run was attempted and is not being reported as a result: the tree was rebased
underneath it mid-run, which produced one unrelated ImportError failure in
tests/test_audit_offbox_tee.py. That is an artifact of the rebase, not a finding. The CI legs are
the authority.

Three deliberate non-changes

1. # nosec B603 B607 is left intact at both sites, though B607 is now redundant. Measured
   with a positive control: with both codes removed, bandit reports exactly B603
   (subprocess_without_shell_equals_true) and no B607, so pinning the path does eliminate the
   partial-path finding. It is left because the vault's docs/security/THREAT-MODEL.md:263 states
   the annotation verbatim as # nosec B603 B607, and the doc-drift test is inert in public
   checkouts (docs/security/** is withheld), so nothing in CI would catch that doc going stale.
   Trading dead text for silent drift in the security record is the worse outcome.

2. service_status.py is not touched at all, including its module docstring, which enumerates
   its importers as messagefoundry.config and messagefoundry.api. There are now six, and fix(service): pin every elevated program to an absolute system path (BACKLOG #1680) #1132's
   _system_exe docstring likewise enumerates its callers as "this module and
   messagefoundry.service" when this change adds two more. Both are completeness claims that
   SDS-3.6 would rather see as "at least". They are left alone because any edit to those lines
   conflicts with fix(service): pin every elevated program to an absolute system path (BACKLOG #1680) #1132 (measured), and because a four-file diff cannot accidentally land another
   session's work. Widening them belongs to fix(service): pin every elevated program to an absolute system path (BACKLOG #1680) #1132's author or a follow-up.

3. The vault THREAT-MODEL.md row is not updated here -- separate repository. Its subprocess
   inventory already lists both icacls sites, and its control column ("list-form argv with
   shell=False and fixed/validated arguments") stays true; this change makes it understated, not
   false. That update and the B607 removal belong together as one vault change covering these two
   sites and fix(service): pin every elevated program to an absolute system path (BACKLOG #1680) #1132's sc/cmd/net sites, which carry the same now-redundant annotation.

[wshallwshall]

LANDER: THE DEPENDENCY IS CLEARED. PR 1132 is merged, so _system_exe is on main and this PR can be marked ready.

7cb9969cb  fix(service): pin every elevated program to an absolute system path (BACKLOG #1680) (#1132)
engine main is now 448b0ab14

Recording this here rather than in a message, because the session that authored this PR has ended and a cross-session note dies with the session that received it. That was this PR author's own point and it turned out to apply to them.

MARK IT READY. DO NOT RE-RUN ITS CHECKS. This is the author's instruction and it is correct: a re-run replays the recorded merge SHA, which predates 1132, so the red would come straight back. Marking the PR ready fires a fresh pull_request event, and ci.yml triggers on a bare pull_request: with no ref: on any checkout, so the new run gets a recomputed refs/pull/1144/merge that contains 1132.

DO NOT REBASE EITHER. The author measured it with git merge-tree against a synthetic squash of 1132 onto main: 1144 into squashed-1132 is exit 0, and squashed-1144 into 1132 is exit 0, both producing the identical tree. The two PRs touch disjoint files, so order does not matter.

TWO THINGS FROM MY READ OF 1132 THAT BEAR ON THIS PR, since it imports from the same module:

_system_dir() asks the OS for the system directory rather than reading %SystemRoot%. The reason matters here: an environment block is inherited from whoever launched the process, and that is the same party the pin defends against. A resolver that went through the environment would look complete and reopen the hole. Its fallback still yields an absolute path, never a bare name. So importing _system_exe is the right call and re-implementing it would not be.

The pin deliberately lives in service_status.py, the stdlib-only leaf, with service.py importing it -- one pin, not two. Three of four review agents on 1132 flagged a second resolver as re-creating the exact drift the item blames, which is the sharper objection: the bug being fixed IS two code paths disagreeing about how to name a program. This PR importing rather than duplicating keeps that property.

ON windows-service-smoke, the leg that would actually exercise both PRs: it CANNOT run on a pull request. ci.yml:2567 gates it on schedule, workflow_dispatch and merge_group only, so on a PR it reports skipping. Do not wait for it and do not read the skip as a coverage gap. Enqueuing is its test. That bounds how much pre-merge evidence this PR can carry, and it is a property of the pipeline rather than a deficiency in the build.

I have not touched this PR. Whoever picks it up: mark ready, and I will read and enqueue it in turn.