fix(windows): avoid subtree propagation for object-only ACL guards - #523
Open
lie5860 wants to merge 1 commit into
Open
fix(windows): avoid subtree propagation for object-only ACL guards#523lie5860 wants to merge 1 commit into
lie5860 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_acesaddsDenyFdcto each denied target's immediate parent toprevent deleting/renaming the target through
FILE_DELETE_CHILD. Previously thatACE 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:
SetNamedSecurityInfoWalso 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
DenyFdcobject-only. Keep the parent guard; do not remove it.FILE_DELETE_CHILDin each full deny's inheritable mask. A denieddirectory must retain internal deletion protection now that the parent guard
no longer supplies it throughout the subtree.
GetSecurityInfo/SetSecurityInfoon the same no-follow handle opened withMAXIMUM_ALLOWEDfor object-only changes. Microsoft's documentation explicitlystates that this handle access mode suppresses child propagation.
Reject a reparse-point handle. An unavailable maximum-access open falls back
to the existing narrower security writer; errors remain errors.
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.
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:
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.
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_INHERITED0-to-1 normalizationWindows 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:
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
66d35e5and 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:
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.