Documentation of ObjectSecurity.SetAccessRuleProtection() somewhat vague/misleading/questionable or wrong(?)

[exoosh]

The documentation ends with the following remark:

When you call the method with isProtected=true and preserveInheritance=true, you need to walk the new ACL of the object and check for DENY type ACEs. For a canonically sorted DACL, the DENY ACEs must appear in the front of the DACL. For more information on the canonical ordering of ACLs, see Order of ACEs in a DACL.

The remark is correct regarding the ordering of ACEs in a DACL and that a well-formed (i.e. canonically sorted) DACL has the Deny ACEs before the Allow ACEs. It does matter.

Also, since the base class implementation of NativeObjectSecurity.Persist() uses Win32.SetSecurityInfo():

The SetSecurityInfo function does not reorder access-allowed or access-denied ACEs based on the preferred order. When propagating inheritable ACEs to existing child objects, SetSecurityInfo puts inherited ACEs in order after all of the noninherited ACEs in the DACLs of the child objects.

... one might expect that the outcome is some wildly ill-formed (i.e. non-canonical) ACL.

And the remark is borderline FUD due to its suggestive wording, as if one would potentially need to fix up the ACL's order of ACES as follows, quote:

The following steps describe the preferred order:

1. All explicit ACEs are placed in a group before any inherited ACEs.
2. Within the group of explicit ACEs, access-denied ACEs are placed before access-allowed ACEs.
3. Inherited ACEs are placed in the order in which they are inherited. ACEs inherited from the child object's parent come first, then ACEs inherited from the grandparent, and so on up the tree of objects.
4. For each level of inherited ACEs, access-denied ACEs are placed before access-allowed ACEs.

Back to the remark. Walking the DACL isn't the same as having to fix it. But since the explicit ACEs come first and within that group the Deny ACEs come first, it's totally unclear what may need fixing, let alone how to fix it.

But the suggestive, yet vague, remark becomes questionable once you notice that DACLs always always seem to get canonicalized by the .NET Framework, via this code (and references to this method).

So what's the purpose of the remark?

With best regards,

Oliver

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchForks See info in area-owners.md if you want to be subscribed.

Issue Details

---

The documentation ends with the following remark: > When you call the method with `isProtected=true` and `preserveInheritance=true`, you need to walk the new ACL of the object and check for DENY type ACEs. For a canonically sorted DACL, the DENY ACEs must appear in the front of the DACL. For more information on the canonical ordering of ACLs, see [Order of ACEs in a DACL](https://docs.microsoft.com/en-us/windows/win32/secauthz/order-of-aces-in-a-dacl). The remark is correct regarding the ordering of ACEs in a DACL and that a well-formed (i.e. canonically sorted) DACL has the Deny ACEs before the Allow ACEs. It _does_ matter. Also, since the base class implementation of `NativeObjectSecurity.Persist()` uses [`Win32.SetSecurityInfo()`](https://docs.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setsecurityinfo): > The `SetSecurityInfo` function **does not reorder access-allowed or access-denied ACEs based on the preferred order**. When propagating inheritable ACEs to existing child objects, `SetSecurityInfo` puts inherited ACEs in order after all of the noninherited ACEs in the DACLs of the child objects. ... one might expect that the outcome is some wildly ill-formed (i.e. non-canonical) ACL. And the remark is borderline FUD due to its suggestive wording, as if one **would potentially need to fix up the ACL's order of ACES** [as follows](https://docs.microsoft.com/en-us/windows/win32/secauthz/order-of-aces-in-a-dacl), quote: > The following steps describe the preferred order: > > 1. All explicit ACEs are placed in a group before any inherited ACEs. > 2. Within the group of explicit ACEs, access-denied ACEs are placed before access-allowed ACEs. > 3. Inherited ACEs are placed in the order in which they are inherited. ACEs inherited from the child object's parent come first, then ACEs inherited from the grandparent, and so on up the tree of objects. > 4. For each level of inherited ACEs, access-denied ACEs are placed before access-allowed ACEs. Back to the remark. _Walking_ the DACL isn't the same as _having to fix it_. But since the explicit ACEs come first and within that group the Deny ACEs come first, it's totally unclear _what may need fixing_, let alone _how to fix it_. But the _suggestive, yet vague, remark_ becomes questionable once you notice that DACLs always _always_ seem to get canonicalized by the .NET Framework, [via this code](https://referencesource.microsoft.com/#mscorlib/system/security/accesscontrol/acl.cs,855) (and references to this method). So what's the purpose of the remark? With best regards, Oliver

Author:	exoosh
Assignees:	-
Labels:	`area-System.Security`, `:watch: Not Triaged`, `Pri3`
Milestone:	-