Get-ModifiablePath false positive result processing deny only SID

[Marsel-marsel]

Great tool! But I suppose I figure out some buggy testcase: if thread's token has deny only SID and file's ACE has deny only rule for that SID, Get-ModifiablePath() still shows that file could be modified, in spite of it actually couldn't.

PS C:\> icacls C:\test\file.txt
C:\test\file.txt BUILTIN\Administrators:(DENY)(W)  <--- deny rule for administrators 
                 BUILTIN\Users:(R,W)

PS C:\> whoami /groups
BUILTIN\Administrators  Alias            S-1-5-32-544 Group used for deny only  <--- current user is administrator
BUILTIN\Users           Alias            S-1-5-32-545 Mandatory group, Enabled by default, Enabled group

PS C:\> Get-ModifiablePath -Path C:\test\file.txt
ModifiablePath   IdentityReference                Permissions                                                           
--------------   -----------------                -----------                                                           
C:\test          NT AUTHORITY\Authenticated Users {Delete, WriteAttributes, Synchronize, ReadControl...}                
C:\test\file.txt BUILTIN\Users                    {WriteAttributes, Synchronize, ReadControl, ReadData/ListDirectory...} <--- Get-ModifiablePath() shows that file is modifiable

PS C:\> echo 123 > C:\test\file.txt
out-file : Access to the path 'C:\test\file.txt' is <--- actually it's not

Suppose that root cause of the issue that you are completely ignoring deny ACEs after Get-Acl() invocation

[itm4n]

Hello,

Thank you for taking the time to report this issue. You are completely right, deny ACEs are ignored by the check.

The Get-ModifiablePath function is from the original PowerUp project, I only slightly modified it to better handle some edge cases. Checking those deny ACEs would add to the complexity for a very little gain. Therefore, the choice was made not to implement it and I think it was an acceptable tradeoff.

That being said, that could be an opportunity for me to try and improve this check by adding this functionality. I will see what I can do.

In the meantime, could you simply tell me if this is a situation you encountered in a real environment or you just found out while testing in a lab environment?

[itm4n]

I tried to resolve this issue with commit b80e910.

There is now a dedicated and generic Get-AclModificationRights cmdlet that can check the DACL of a File/Directory/Registry Key.

I implemented the following solution:

1. I enumerate "deny" ACEs first. If any of them affects the current user's permissions, I save the "restricted" rights in a list.
2. I enumerate the "allow" ACEs. For each ACE, I remove the "restricted" rights (list from step 1) from the list of permissions.
3. I proceed as I did before. If the resulting list of permissions grants any modification right, I report it.

As a side note, I noticed that I forgot to include the "Delete" right in the list of potentially interesting rights so I took this opportunity to add it.

This solution is not perfect as I had to alter the content of ACE objects (by removing restricted rights) but that's an acceptable tradeoff IMO, especially given the fact that "deny" ACEs are not that common.

Further testing would obviously be appreciated. :)

[Marsel-marsel]

I've encountered this issue in the test environment. That's why agree with you that less complexity is better than this feature possible implementation.

I've reviewed b80e910 commit and I think there might be an issue:

$UserIdentity = [System.Security.Principal.WindowsIdentity]::GetCurrent()
$CurrentUserSids = $UserIdentity.Groups

$CurrentUserSids contains only allow token's SIDs, that's why if() statement below will always resolve to true. Thus, step 2 (remove the "restricted" rights from the list of permissions) will never be executed.

$IdentityReferenceSid = Convert-NameToSid -Name $DenyAce.IdentityReference
if ($CurrentUserSids -notcontains $IdentityReferenceSid) { continue }$Restrictions = 
# next never executes
$Restrictions = $TypeAccessMask.Keys | Where-Object { $DenyAce.$TypeAccessRights.value__ -band $_ } | ForEach-Object { $TypeAccessMask[$_] }

Here is the little POC:

# Current process' token contains 2 deny SIDS - admin and nt local
PS C:\Users\user> whoami /groups
NT AUTHORITY\Local account and member of Administrators group Well-known group S-1-5-114    Group used for deny only
BUILTIN\Administrators                                        Alias            S-1-5-32-544 Group used for deny only

# This is the way you fetch current process token SIDS
PS C:\Users\user> $UserIdentity = [System.Security.Principal.WindowsIdentity]::GetCurrent()
PS C:\Users\user> $CurrentUserSids = $UserIdentity.Groups
PS C:\Users\user> $CurrentUserSids.Value
S-1-5-21-4160622650-3572743448-2429229259-513
S-1-1-0
S-1-5-32-545
S-1-5-4
S-1-2-1
S-1-5-11
S-1-5-15
S-1-5-113
S-1-2-0
S-1-5-64-10
# notice that this list doesn't contain admin (S-1-5-32-544) and nt local (S-1-5-114)

I don't know how to get token's deny SIDs in powershell. I suppose AccessCheck() usage may resolve this issue.

[itm4n]

Thank you very much for testing and reviewing my solution.

I tested only deny ACES, not deny SIDs, hence why I missed this issue, nice catch!
You might be right, calling AccessCheck might be a smarter solution than rolling my own ACL check logic.
Damn it, I knew this would give me headaches. 🤯

[itm4n]

So, I realized that although AccessCheck would be the most reliable option, it would also result in an information loss as I would no longer be able to say which ACE grants which rights.

Instead, I chose to add a simple test case for restricted groups.

$CurrentUserDenySids = [string[]](Get-TokenInformationGroups -InformationClass RestrictedSids | Select-Object -ExpandProperty SID)
# ...
foreach ($DenyAce in $DenyAces) {
    # ...
    $IdentityReferenceSid = Convert-NameToSid -Name $DenyAce.IdentityReference
    if ($CurrentUserDenySids -notcontains $IdentityReferenceSid) { continue }
    if ($CurrentUserSids -notcontains $IdentityReferenceSid) { continue }
    # ...
}

I have to admit I did only basic testing, especially to ensure that there was no regression, so I don't know whether this solution actually works.

[Marsel-marsel]

Seems valid. Thank you!

[itm4n]

OK, thank you for your feedback. 👌

[Marsel-marsel]

Hello again,
$CurrentUserDenySids = [string[]](Get-TokenInformationGroups -InformationClass RestrictedSids ...
won't contain deny sids, because, according to MSDN RestrictedSids will get deny sids of restricted token, and also according to MSDN restricted token is the

access token that has been modified by the CreateRestrictedToken

Thus, if you don't invoke CreateRestrictedToken() you won't get restricted SIDs.
I suggest to use Groups instead of RestrictedSids. So this is the correct way to initialize $CurrentUserDenySids for example above:

PS> $CurrentUserDenySids = Get-TokenInformationGroups -InformationClass Groups | Where-Object {$_.Attributes.Equals("UseForDenyOnly")} | Select-Object -ExpandProperty SID
PS> $CurrentUserDenySids
S-1-5-114 <--- system (deny) 
S-1-5-32-544 <--- admin (deny)

[itm4n]

Hello!
It looks like I missed your message. I updated the script with your proposed solution.
Thank you again! 🙂