Skip to content

fix(windows): avoid subtree propagation for object-only ACL guards - #523

Open
lie5860 wants to merge 1 commit into
anthropics:mainfrom
lie5860:codex/windows-acl-parent-propagation
Open

fix(windows): avoid subtree propagation for object-only ACL guards#523
lie5860 wants to merge 1 commit into
anthropics:mainfrom
lie5860:codex/windows-acl-parent-propagation

Conversation

@lie5860

@lie5860 lie5860 commented Sep 10, 2026

Copy link
Copy Markdown

Summary

Avoid recursive DACL propagation when updating object-only sandbox ACEs on a
denied target's parent. Related to #457: this fixes a reproducible parent-ACL
path, not every possible cause of Windows startup latency.

The change is confined to native ACL recomposition, regression tests and comments.
It does not change the CLI protocol, state schema, holder accounting, token model,
network policy, account provisioning or shipped binaries.

Cause

Locked::apply_aces adds DenyFdc to each denied target's immediate parent to
prevent deleting/renaming the target through FILE_DELETE_CHILD. Previously that
ACE was OI/CI-inheritable. Stamping a direct child of the user profile therefore
rewrote the profile DACL and propagated through unrelated descendants.

Making just the new ACE non-inheritable is insufficient: SetNamedSecurityInfoW
also propagates the DACL's other inheritable entries. The cost remains proportional
to the unrelated tree size even when the requested guard is parent-only.

Change and safety boundaries

  • Make parent DenyFdc object-only. Keep the parent guard; do not remove it.
  • Include FILE_DELETE_CHILD in each full deny's inheritable mask. A denied
    directory must retain internal deletion protection now that the parent guard
    no longer supplies it throughout the subtree.
  • Use GetSecurityInfo/SetSecurityInfo on the same no-follow handle opened with
    MAXIMUM_ALLOWED for object-only changes. Microsoft's documentation explicitly
    states that this handle access mode suppresses child propagation.
  • Preserve inherited ACEs, other principals and DACL protection on that path.
    Reject a reparse-point handle. An unavailable maximum-access open falls back
    to the existing narrower security writer; errors remain errors.
  • Keep propagation for full grants/denies and whenever an old explicit sandbox
    ACE has inheritance flags. This includes removal of legacy OI/CI parent-FDC
    rules, so upgrades do not strand inherited guards in descendants. A protected
    DACL with stray inherited entries also retains the existing fail-closed path.
  • Do not skip writes based on ledger presence or a matching root ACE: reapplying
    object-only guards repairs drift, and inheritable changes still converge the tree.

Reference: SetSecurityInfo documentation.

Legacy-rule cleanup and actual inheritable policy changes can still be expensive;
this patch intentionally does not trade correctness for a blanket constant-time claim.
The existing Windows additive-ACL isolation boundaries are unchanged.

Regression coverage

New owned-fixture native tests cover:

  • Parent-only ACE shape; unchanged existing siblings and future sibling files.
  • Full-deny FDC protection inside denied directories.
  • Actual kernel write/delete/rename PermissionDenied, positive controls,
    unchanged bytes, future denied children and successful access after release.
    The test-only restricted impersonation token exercises ACL evaluation without
    provisioning a sandbox account; it is not the production SRT token.
  • Protected DACLs and unrelated principals; inherited grants and denies.
  • Legacy inherited FDC cleanup, including when another parent hold remains.
  • Grant/deny removal propagation and repeated-apply drift repair.
  • In-memory ledger with real filesystem ACLs: siblings, multiple holders and
    partial-batch failure that must retain a pre-existing holder's protection.

An opt-in ACL-only CLI smoke test additionally uses real live holder PIDs and an
existing SRT installation to cover multiple holders, hardlink-triggered rollback,
dead-holder recovery, and optional nested/direct-profile-child timing. It does not
install/uninstall accounts, change WFP, launch a sandboxed command or alter global
PowerShell settings. Cleanup verifies ownership boundaries and ACL restoration.

The smoke test reports the specific SE_DACL_AUTO_INHERITED 0-to-1 normalization
Windows security writers can make on first use. It does not ignore owner/group,
protection, inheritance-request, ACE-order, flag or mask changes. The final profile
round trip was SDDL-identical. See control-bit semantics.

Validation

Locally on Windows 11 x64 / NTFS, Rust 1.94.1 and Node 22.21.1:

cargo build --release --manifest-path vendor/srt-win-src/Cargo.toml
cargo test --release --manifest-path vendor/srt-win-src/Cargo.toml
cargo clippy --release --manifest-path vendor/srt-win-src/Cargo.toml --all-targets -- -D warnings
cargo fmt --manifest-path vendor/srt-win-src/Cargo.toml --all -- --check
npm run typecheck
npm run build
node node_modules/eslint/bin/eslint.js .
node vendor/srt-win-src/ci/smoke-acl-parent.mjs vendor/srt-win-src/target/release/srt-win.exe --profile

All passed: 64 native tests, plus the explicitly run benchmark and all five
CLI smoke scenarios. Two initial regression assertions failed against unmodified
upstream before implementation. Existing native tests passed on the base revision.

Synthetic benchmark, identical benchmark code on base 66d35e5 and this patch;
only the parent ACE apply/release calls are timed (fixture creation and descriptor
verification excluded). Every descriptor and file sentinel is checked afterward:

Unrelated files Base apply / release (ms) Patched apply / release (ms)
0 0.932 / 0.679 0.407 / 0.302
1,000 101.610 / 95.586 0.419 / 0.304
10,000 1124.426 / 1086.648 0.469 / 0.433
cargo test --release --manifest-path vendor/srt-win-src/Cargo.toml --test acl_parent_scope benchmark_parent_acl_tree_size -- --ignored --nocapture

The benchmark is opt-in, not a timing-sensitive CI threshold. On the same test
host, a direct-profile-child CLI stamp/release was approximately 52/50 ms after
the change; the prior official-only control exceeded 60 seconds. This is native
ACL preparation evidence, not a claim about full application startup latency.

Local validation did not rerun the account/WFP installation smoke scripts or
the full cross-platform sandbox integration suite on the development machine.
The existing hosted-runner matrix remains necessary, especially Windows arm64
and production-token execution. No dependencies, lockfiles or binaries changed.

Keep parent FILE_DELETE_CHILD protection object-local, retain inheritable protection inside denied trees, and preserve propagation for legacy cleanup and grant/deny changes.

Add native ACL, holder lifecycle, rollback and recovery regressions plus an opt-in benchmark. Related to anthropics#457.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant