trailofbits/windows-acl

Adding an entry seems to lose the inherited flag on existing ACEs?

simonbuchan opened this issue · 1 comments

This one is a bit confusing to me, as I'm not that familiar with the windows security APIs, so please bear with me:

So I have some simple test code to create a directory and add full control to the builtin "Users" group (not a reasonable option, but doesn't seem to matter much exactly what I pick here):

    std::fs::create_dir_all("test-dir")?;
    let mut acl = acl::ACL::from_file_path("test-dir", /*get_sacl:*/ false).map_err(to_io_err)?;
    let users_sid = windows_acl::helper::string_to_sid("S-1-5-32-545").map_err(to_io_err)?;
    let inheritable = true;
    let mask = GENERIC_ALL;
    acl.allow(users_sid.as_ptr() as PSID, inheritable, mask)
        .map_err(to_io_err)?;

When I test this, instead of getting permissions like so:

image

where the new ACE is added separately to the inherited ACEs, instead it removes the inheritance (presumably INHERITED_ACE) and merges into any existing ACE for the SID (e.g. all entries are "Inherited from: None")

It's not exactly clear to me what's happening here, since the code in <AddEntryCallback as EntryCallback>::on_entry() seems to be trying to avoid this situation explicitly.

Am I just doing something wrong? Does this matter at all?

Thanks for the report!

It's been a while since I've looked at Windows ACLs/ACEs, but I remember the ordering rules for preserving inheritance being extremely obtuse. MSDN documents them here: https://docs.microsoft.com/en-us/windows/win32/secauthz/order-of-aces-in-a-dacl

I'll look into this more and try to repro, but I think your interpretation of INHERITED_ACE and AddEntryCallback as EntryCallback>::on_entry() is right.