feat(config): make Windows cache-dir DACL hardening configurable (env var + CLI config key) - #1649
Conversation
Add an opt-out for the Windows cache-directory DACL hardening (upstream DeusData#1624): CBM_SKIP_DACL_HARDENING=1 as an env kill switch effective from the first run, plus a persisted windows-dacl-hardening config key (default true) effective from the second run, with env taking precedence. When disabled: the creation-time stamp keeps the exact-owner stamp but drops the owner-only protected DACL; the runtime-directory walk skips the re-protection and restores the parent-inherited DACL (SE_DACL_PROTECTED cleared) on a directory hardened by a previous run, supplying a real ACL rather than a NULL DACL; the validators accept the OS-default inherited DACL. Owner validation stays active in every path, and the default behavior is unchanged. Docs updated (CONFIGURATION.md runtime settings + environment variables, README env table) and three Windows-only unit tests added in tests/test_daemon_ipc.c.
|
Thank you for this, and especially for the two details that show you understood the constraint rather than working around it: the env kill switch working from the very first run (the config store lives inside the directory being hardened, so a persisted key alone cannot help someone who is locked out on run one), and keeping owner validation active in every path when hardening is off. Those are exactly the two things that would have made a naive version of this unsafe. Your #1620 report is also the best-evidenced thing in the tracker this week — the background I need to be straight with you about how this will be handled, because it is a policy question rather than a code review. Making security hardening opt-out is a maintainer decision, not something I will merge on technical merit alone. The protected-DACL behaviour was a deliberate choice, and an escape hatch changes what a default install guarantees — including for people who never read the flag. That belongs to the project owner, and I will put it in front of them with your evidence attached rather than quietly deciding it. There is also a live alternative worth weighing against yours: the re-stamp currently fires on every process start, whether or not anything is wrong. That is what produces the rewrite window your Two things I can do now: your work is queued behind a v0.10.5 that fixes a batch of install and startup blockers (CI is badly backed up tonight), and #1628 — which makes an atomic-publish failure report the actual Win32 error instead of I will come back with a decision rather than leaving this open indefinitely. |
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
2 participants
Summary
Closes #1624 (fix option #2 from #1620).
Makes the Windows cache-directory DACL hardening configurable via two opt-out surfaces:
CBM_SKIP_DACL_HARDENING=1— effective from the very first run, before any config store exists.windows-dacl-hardening(defaulttrue) — settable viaconfig set/reset, visible viaconfig list. Effective from the second run (the store_config.dblives inside the cache directory, so it does not exist when the first run creates that directory).Precedence: env > config. When hardening is disabled, owner validation of the cache directory stays active in every path; the default behavior is unchanged.
What changes when disabled
cbm_windows_stamp_dir_owner(creation stamp)DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATIONdropped → OS-default inherited DACLwin_runtime_directory_secure(per-start re-application)icaclswin_file_acl_secure(validator)Decisions and tradeoffs
D1 — Flag threading: process-global accessor, not a signature change. The stamp site lives in
foundation/(reached fromcbm_mkdir_p, ~25 call sites) while the harden site lives indaemon/; threading aboolthrough both would touch two definitions plus ~9 callers and still could not reach the stamp without going throughcbm_mkdir_p. Instead astatic atomic_int g_dacl_hardening = -1+ accessor, mirroring the existingcbm_mem_profile_enabledidiom.D2 — Config flows via an explicit setter, not the accessor. The accessor resolves the env var only, so it stays dependency-free inside the stamp path (which runs before any config store exists). A lazy config read inside the accessor would recurse:
cbm_config_open→mkdirp→ stamp → accessor.main_build_identityreads the config once (env unset) and stores it via the setter.D3 — Skip + unprotect (self-healing), not skip-only. The issue's literal proposal (skip the protection) leaves an already-hardened directory broken — the user would still need a manual
icacls /inheritance:e, contradicting "config set false→ index →indexed". The opt-out therefore also restores inheritance. Important implementation detail:SetSecurityInfowithDACL_SECURITY_INFORMATIONand aNULLpDacl creates a NULL DACL (full access to everyone) and would also make the relaxed validator fail closed. The unprotect therefore copies the parent directory's DACL (the ACE set a freshly created directory inherits) and clearsSE_DACL_PROTECTED— the same end state asicacls /inheritance:e.D4 — Global stamp skip, runtime-dir-only unprotect. The stamp skip is global (all dirs created via
cbm_mkdir_p); other directories do not use rename-replace, so skipping their DACL protection has no functional effect. The unprotect is naturally runtime-dir-only.D5 — Two-phase activation timing (documented, not worked around). Env from run 1; config from run 2. A single-run config opt-out is impossible by construction (the store lives inside the boundary being protected) and is documented in
docs/CONFIGURATION.md.D6 — Verification strategy. Wine (the only local Windows runtime) does not implement real ACL semantics:
SE_DACL_PROTECTEDis not honored, so the unprotect unit test asserts meaningfully only on real Windows (fork CI msys2 legs). Local gates: clang-format-20 + cppcheck, linux/amd64 test+build, mingw cross-compile, Wine test-runner and version check.D7 — Unit tests. Three Windows-only tests in
tests/test_daemon_ipc.c: env accessor contract, stamp-skip (inherited DACL + exact owner), and relaxed-validator + unprotect (previously hardened directory accepted,SE_DACL_PROTECTEDcleared, resulting DACL non-NULL and valid).Security tradeoff. The protected DACL defends the cache/IPC boundary against other local accounts. Disabling it is intended for single-user hosts where the protected DACL breaks
MoveFileExWrename-replace (EDR/minifilter conflicts); it is not recommended on multi-user/terminal-server hosts. Default remainstrue; owner validation is never disabled.Verification
./test-infrastructure/run.sh lint— pass (clang-format-20 + cppcheck 2.20.0)./test-infrastructure/run.sh amd64— build pass; 5 test failures are pre-existing and base-identical (container runs as root vs/srcowned by uid 1000 → activation-transaction ancestor check; vendored-integrity drift) — verified by running the same leg on the base commitdocker compose -f test-infrastructure/docker-compose.yml run --rm test-windows— mingw cross-compile pass; Winedaemon_ipcsuite shows no regressions vs base (2/3 new tests pass under Wine; the third requires real ACL semantics, per D6)./test-infrastructure/run.sh windows— production mingw build + Wine version check passTest plan for reviewers
config set windows-dacl-hardening false→ index any repo →status:"indexed"(on an affected host; regression-check on a normal host that default behavior is unchanged)CBM_SKIP_DACL_HARDENING=1overridestruein config