Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check root folder ACLs #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Check root folder ACLs #53

wants to merge 1 commit into from

Conversation

v-p-b
Copy link

@v-p-b v-p-b commented Feb 10, 2024

A common pattern I recognized is admins placing applications in directories created in drive roots, e.g.:

C:\Install
C:\ORACLE
C:\SuperAccounting

Some installers also do this by default.

Since these folders inherit AddDirectory and AddFile permissions for the BUILTIN\Users group, any local user can place malicious DLL's inside these directories to hijack processes executed by other users of the same machine (think terminal servers):

C:\ORACLE
\__bin
    \__sqlplus.exe
    \__version.dll (totally legit *wink* *wink*)

For now, a new check only enumerates modifiable folders in root directories, and doesn't check whether there are any executables inside (this seems to be problematic elsewhere too). It also doesn't care if subdirectory ACL's are customized. Since there are other venues for attack (e.g. add a script to a conf.d-like folder) I'd rather just highlight the potential problem and let the tester assess the actual risk depending on the environment.

I only use PowerShell when I really have to, so the code is mostly frankensteined from other places. Also I don't expect this to be merged right away (not sure about base risk, are descriptions good enough, ...), any feedback is welcome!

@itm4n
Copy link
Owner

itm4n commented Feb 21, 2024

I totally get your point, but...

  • There is already a check for misconfigured DACLs on program folders - Invoke-ModifiableProgramsCheck - the one you pointed out.
  • There is also a check for misconfigured DACLs on PATH folders, which often reveals this kind of bad practice because, most of the time, they were created at the root.
  • Checking root folder seems a bit too arbitrary. I mean, although it's a common place for misconfiguration. if I add this kind of check, I might have to consider other locations as well.
  • Overall, the philosophy is to limit manual analysis. I want the reports to be as accurate as possible.

So, I'm not entirely sure about this change. I will have to think a bit more about it. 🤔

@v-p-b
Copy link
Author

v-p-b commented Feb 23, 2024

Thanks for considering this! Some notes:

Checking root folder seems a bit too arbitrary. I mean, although it's a common place for misconfiguration. if I add this kind of check, I might have to consider other locations as well.

I can hardly remember any case when I encountered similar misconfigs elsewhere, while installations in the drive root seem pretty common. Considering the standard top-level directory strucure in a fresh installation:

  • PerfLogs - I doubt anyone would install anything here
  • Program Files / Program Files (x86) - Strong default ACL, the preferred locations for system-wide installations
  • Users - User folders are accessible by their owner only (no cross-user contamination), Public may be interesting though
  • Windows - Strong default ACL

it seems to me that there are not many cases where one would install things. There are of course countless possible configurations and we can't cover everything (full recursive ACL checks would be overkill IMO).

Overall, the philosophy is to limit manual analysis. I want the reports to be as accurate as possible.

I created this patch because I had to go through a few dozen servers by hand (no network access) to collect this information, and it would've been nice if PrivescCheck did the work for me in a single run :)

Would it make a difference if we improved on executable detection too, so both Invoke-ModifiableProgramsCheck and this one would get more accurate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants