Fix Get-ModifiablePath returning early and missing other modifiable paths in the same entry

[SAERXCIT]

Hi!

I have observed what I think is a defect in Get-ModifiablePath. This PR aims to fix this issue.

Issue

Get-ModifiablePath returns early when finding a modifiable path within an entry, and will miss other modifiable paths in the same entry (for instance, the directory containing a binary).

Test setup is as follows:

• Service running a writable binary in a writable folder:

PS Z:\PrivescCheck> sc.exe qc vuln
[SC] QueryServiceConfig SUCCESS

SERVICE_NAME: vuln
        TYPE               : 10  WIN32_OWN_PROCESS
        START_TYPE         : 3   DEMAND_START
        ERROR_CONTROL      : 1   NORMAL
        BINARY_PATH_NAME   : C:\dir\bin.exe
        LOAD_ORDER_GROUP   :
        TAG                : 0
        DISPLAY_NAME       : vuln
        DEPENDENCIES       :
        SERVICE_START_NAME : LocalSystem

PS Z:\PrivescCheck> icacls C:\dir\bin.exe
C:\dir\bin.exe BUILTIN\Administrators:(I)(F)
               NT AUTHORITY\SYSTEM:(I)(F)
               BUILTIN\Users:(I)(RX)
               NT AUTHORITY\Authenticated Users:(I)(M)

Successfully processed 1 files; Failed processing 0 files

PS Z:\PrivescCheck> icacls C:\dir\
C:\dir\ BUILTIN\Administrators:(I)(OI)(CI)(F)
        NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
        BUILTIN\Users:(I)(OI)(CI)(RX)
        NT AUTHORITY\Authenticated Users:(I)(M)
        NT AUTHORITY\Authenticated Users:(I)(OI)(CI)(IO)(M)

Successfully processed 1 files; Failed processing 0 files

• Output of Invoke-ServiceImagePermissionCheck:

PS Z:\PrivescCheck> (Invoke-ServiceImagePermissionCheck).Result |fl


Name              : vuln
ImagePath         : C:\dir\bin.exe
User              : LocalSystem
ModifiablePath    : C:\dir\bin.exe
IdentityReference : NT AUTHORITY\Authenticated Users
Permissions       : Delete, WriteAttributes, Synchronize, ReadControl, ReadData, AppendData, WriteExtendedAttributes,
                    ReadAttributes, WriteData, ReadExtendedAttributes, Execute
Status            : Stopped
UserCanStart      : False
UserCanStop       : False

--> Modifiable path C:\dir\ is missing.

Cause

The code of Get-ModifiablePath breaks out of the loop when a first modifiable path is found, preventing it from finding any other issue:

PrivescCheck/src/check/Helpers.ps1

Line 548 in eb8a0de

if ($ModifiablePath) { $ModifiablePath; break }

Note: This has been the case since commit 3a6afaa, which completely refactored this function. If this was intentional, feel free to reject this PR :)

Fix

Simple remove the break clause. After this, the modifiable directory will appear in the output:

PS Z:\PrivescCheck> (Invoke-ServiceImagePermissionCheck).Result |fl


Name              : vuln
ImagePath         : C:\dir\bin.exe
User              : LocalSystem
ModifiablePath    : C:\dir\bin.exe
IdentityReference : NT AUTHORITY\Authenticated Users
Permissions       : Delete, WriteAttributes, Synchronize, ReadControl, ReadData, AppendData, WriteExtendedAttributes,
                    ReadAttributes, WriteData, ReadExtendedAttributes, Execute
Status            : Stopped
UserCanStart      : False
UserCanStop       : False

Name              : vuln
ImagePath         : C:\dir\bin.exe
User              : LocalSystem
ModifiablePath    : C:\dir
IdentityReference : NT AUTHORITY\Authenticated Users
Permissions       : Delete, WriteAttributes, Synchronize, ReadControl, ListDirectory, AddSubdirectory,
                    WriteExtendedAttributes, ReadAttributes, AddFile, ReadExtendedAttributes, Traverse
Status            : Stopped
UserCanStart      : False
UserCanStop       : False

Cheers, and thanks for this great tool, which I've been using for many years now!

[itm4n]

Thanks for the kind words, and also for being a regular user (and contributor). I appreciate. :)

At first glance, I would tend to say that it's the expected behavior, at least from my point of view. Nevertheless, I get your point, and it might be worth reconsidering this feature.

I'll have to take a closer look, and do some extensive testing, because it's a core function used in many checks, so there is chance it could break something else. I refactored this function entirely not so long ago, so there is probably a reason why I chose to implement it this way, I can't remember for sure.