Title
#windows
zwass

zwass

10/06/2022, 9:59 PM
Hey @thor (and anyone else), @Marcos Oviedo is starting on some work to bring ETW events to osquery (hello
etw_process_events
table!) and we could use some code review from folks with Windows experience. Any chance you have some time to do a bit of review as he pushes PRs starting in the next couple of weeks?
Mike Myers

Mike Myers

10/06/2022, 11:32 PM
I can review but I can't approve
thor

thor

10/07/2022, 2:38 AM
Yeah I can make time for sure, but my C++ is rusty so I’d love if other folks can also look. If I don’t get to then quick just ping me directly and I’ll make sure to take a look, stoked to see this coming!!
defensivedepth

defensivedepth

10/07/2022, 3:00 PM
Definitely cant do code review 🙂, but wanted to say that I am also super excited to see this & the
secedit
table work!!
s

seph

10/08/2022, 6:00 PM
👂 secedit? That would be a cleaner replacement for the exec one I have in launcher
defensivedepth

defensivedepth

10/10/2022, 1:13 PM
s

seph

10/10/2022, 1:32 PM
It’s very weird reading about the Launcher in the third person. (But that issue is totally reasonable, and I’m looking forward to whatever y’all PR)
4:39 PM
I’m not a fan of exec, but I think y’all have misdiagnosed whether or not there’s a security issue.
zwass

zwass

10/10/2022, 4:57 PM
I discussed that concern further with Marcos -- There's apparently a general issue with using the temp directory as a
SYSTEM
user that can lead to exploitable vulnerabilities, but we were not able to identify a way that this could be exploited.
m

Marcos Oviedo

10/10/2022, 5:02 PM
Hey guys, thanks for offering to help with reviewing these windows related changes! I've the secedit-related changes ready here, and I hope to create a PR soon after I finish with my local testing. @seph I'm quite new to Fleet and to osquery ecosystem in general. I jumped on the referenced PR on my second day after joining Fleet, so apologies if components were not properly referenced in what I commented on the PR 😄. Regarding the potential LPE I mentioned, I brought up that potential problem because that privileged directory creation operation on windows could be abused if done in a user-controlled directory such as
%windir%\temp
. There is a good doc that describes this exploitation technique here. I've also included examples of public exploits that abused these techniques when a privileged process wrote into temp directory here. Having said that, I have not created a PoC to exploit this specific scenario, so the general recommendation was around avoiding user-controlled directories from privileged processes as much as possible (here)
s

seph

10/10/2022, 11:13 PM
The Launcher call is not
os.TempDir()
, which as you identify would create a known path and a potential racey exploit. Instead, it uses
ioutil.TempDir
, which is
os.MkdirTemp
. This is notably different, in that it’s creating a random directory in whatever the system considers the temp directory. Mostly I trust the go authors to have gotten this use case correct, But…1. It’s creating a random path. Yes, I suppose, one could create 2^32 directories. I’m not sure if the underlying library is checking 2. Testing, it is created with reasonable permissions 3. It seems to use a per-user directory. As myself, this creates stuff in
~/AppData\Local\Temp
, no idea what it does for services running as admin. 4. Ultimately this is read by
<http://github.com/go-ini/ini|github.com/go-ini/ini>
. I don’t think there should be execution there. Maybe there’s a route to getting secedit to overwrite some file. Or just feeding launcher bad data. But (1) and (2) are generally considered sufficient for those cases.
11:15 PM
All that said, I’m not a fan of the exec pattern, and especially not bouncing through a temp file. I find it more error prone, and can create weird resource utilization. So I’m really psyched to see better API access and stuff like that more into core. It is such a better pattern
m

Marcos Oviedo

10/11/2022, 4:58 PM
Thanks @seph, for the detailed technical response! I'm a big fan of going deep on things to understand how they are implemented, so I truly enjoyed reading your comments on your tests with
ioutil.TempDir()
🙌🙌🙌 I remember quickly trying
ioutil.TempDir
back then to found that it also uses
%windir%\temp
when called from a process running as SYSTEM. Down below are the details on why this happens. This affects #3 as a temporary base directory that can be user-controlled might introduce risks. I ended up suggesting to use
os.UserCacheDir
across Orbit codebase as this helper always returns the path to the
%localappdata%
directory, which is a directory associated with the running user (location with ownership and permissions safe to use, as you mentioned in #2) Your point on #1 is very valid and mitigates exploitation attempts as it would make exploitation harder. I updated the original issue to reflect this and do some justice to the original comment, see here. Avoiding user-controlled tmp directory should be done if possible. Exploiting a bug like this will be complex, and AFAIK the only options an attacker will have are: A) hijack the privileged directory creation to create a directory on a privileged location (abuse examples around controlled privileged directory creation here). B) hijack the privileged .ini file creation/write to redirect the operations to an arbitrary file on a privileged location (abuse examples here). Doing #A is difficult as the attack will have to predict the directory name in advance, brute-forcing the directory name address space to place directory symlinks. This is not very practical and will produce an unreliable exploit (here is an example of people doing something similar for files). On the other hand, doing #B will require the exploit to continuously monitor the
%windir%\temp
folder to look for newly created directories with the expected pattern which I think should be
%windir%\temp\prefix{random}
, and then use this location to place file symlink with the expected ini filename. Winning this race condition will redirect that file operation somewhere else. Again, this is not very practical and will produce an unreliable exploit. Now regarding ioutil.TempDir returning
%windir%\temp
, I found that after
os.MkdirTemp
is called, a call to TempDir() will be made because of the first parameter being empty. The implementation of this function in windows ends up calling syscall.GetTempPath, which in turn ends up calling kernel32!GetTempPathW to obtain the temporary directory. According to MSDN, the GetTempPathW export returns the content of the
%TMP%
environment variable for the running user, which in the case of SYSTEM is
%windir%\temp
. The default value for this environment variable on windows for the SYSTEM user is at the following registry location
HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment
. In the case of standard users, the content of the TMP environment variable corresponds to the value you observed, and it is stored at
HKEY_CURRENT_USER\Environment
registry location.
s

seph

10/11/2022, 5:00 PM
ooo,
os.UserCacheDir
sounds really valuable. I’ll have to dig in when I’m not jumping into office hours 😉
m

Marcos Oviedo

10/11/2022, 5:00 PM
Regarding the upcoming secedit logic merge into core, I've done testing across different versions of windows, and the logic is working fine! No issues have been found so far. FYI, I created two tables to reflect the security configuration data better and mimic secedit output. I went this way to expose the security options stored in the registry through separate columns. Example output details are here. Any feedback is welcomed!
5:01 PM
jumping into office hour
doing the same 😄
defensivedepth

defensivedepth

10/11/2022, 6:38 PM
@Marcos Oviedo Is there an easy way for me to test the secedit table?
m

Marcos Oviedo

10/11/2022, 6:50 PM
thanks for offering to help with testing! I think the easiest way will be to share an osqueryd binary with the changes so you can use it to test it in your lab. Let me know if you want to go that way
s

seph

10/11/2022, 6:50 PM
If it’s in a PR, CI will build artifacts for you
m

Marcos Oviedo

10/11/2022, 6:52 PM
I'm afraid I've not been pushed a PR yet, it is only on a local branch here at this point. I plan to send a PR later today/tomorrow
thor

thor

10/11/2022, 6:53 PM
Oof-da, sorry for not being present on this thread y'all! I can hopefully help out with the testing as well this week. We have contractors out our house and I'm still spinning up at new $JOB, so my day-time has been really tight. I'll try and catch up with y'all tonight ❤️
4:59 AM
Alright, here's hoping your branch @Marcos Oviedo is up to date? I left a couple of comments ([here](https://github.com/osquery/osquery/commit/42e89213eaab4568b654d6c3eb3d3187de51441a) in case you've moved on), mostly just nits though. Overall I think it's good! I wasn't able to find any issues with the dynamic library loading. All I'll do is voice my general concern with using un-documented library calls (We've had this debate before, and I think there's general precedent for using them on Windows where it made sense. That said please be careful 🙏 . The only other thing I think we could benefit from would be wrapping a lot of your interfacing logic with the dynamically loaded DLL into a helper class, so we can get some RIAA benefits. One bit of advice I'd give -- this thread is pretty deep, and I think this whole conversation would be much more searchable if it were had on a GH issue or on a PR to the repo 🙂 Thanks a ton for the implementation work, I know lots of folks are super pumped to see this ship!
m

Marcos Oviedo

10/13/2022, 9:47 PM
Thanks @thor for having a look at the branch and providing comments! This is excellent feedback. I will go through it and incorporate some changes before sending a PR. The idea of having an RIAA helper class is worth doing, so I'm including that one on the upcoming change, good suggestion. As for using undocumented APIs, I agree that it is something that has to be treated very carefully. To mitigate risks around this, I've mostly checked that the scecli.dll binary with the export that I'm using kept the same prototype across windows versions (checked win7 and on) and also added sanity checks whenever possible on the returned data. Having said that, I'm going to update this thread with the PR number as soon as I have it.
thor

thor

10/16/2022, 2:20 AM
@Marcos Oviedo sounds great, thanks for the hard work! Feel free to add me as a reviewer when it’s up for PR, happy to allocate time, and I can also test if needed but these days I’ve got but a handful of Windows boxes 😃
m

Marcos Oviedo

10/17/2022, 6:40 PM
Thanks @thor! I've just created PR 7794 with the latest changes. I'll ask maintainers to add you as a reviewer 😄
zwass

zwass

10/21/2022, 5:15 PM
@thor I added you as a reviewer 🙂 please let us know if you have a chance to look in the next week or so.
thor

thor

10/21/2022, 5:47 PM
Will get it a review asap. I should have some time this weekend, but worst-case I’ll get eyes on it early next week!
zwass

zwass

10/21/2022, 8:22 PM
Thank you! Please enjoy your weekend, next week would be great!
thor

thor

10/29/2022, 3:15 AM
@Marcos Oviedo saw your update, will get it another review as soon as possible! It's all hallows eve weekend though, so lots of family plans, but will try and get you something no later than Tuesday 🙂
m

Marcos Oviedo

10/29/2022, 9:05 PM
Thanks Nick! Next week sounds great to me. I hope you have a fun and spooky Halloween with the fam! 🎃😄