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?

I can review but I can't approve

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!!

Definitely cant do code review 🙂, but wanted to say that I am also super excited to see this & the

secedit

table work!!

👂 secedit? That would be a cleaner replacement for the exec one I have in launcher

REF: https://github.com/fleetdm/fleet/issues/7425

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)

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.

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)

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.

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.

ooo,

os.UserCacheDir

sounds really valuable. I’ll have to dig in when I’m not jumping into office hours 😉

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!

@Marcos Oviedo Is there an easy way for me to test the secedit table?

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

If it’s in a PR, CI will build artifacts for you

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

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 ❤️

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.

@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 :)

Thanks @thor! I've just created PR 7794 with the latest changes. I'll ask maintainers to add you as a reviewer 😄

@thor I added you as a reviewer 🙂 please let us know if you have a chance to look in the next week or so.

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!

Thank you! Please enjoy your weekend, next week would be great!

@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 🙂

Thanks Nick! Next week sounds great to me. I hope you have a fun and spooky Halloween with the fam! 🎃😄

Im at Hushcon this weekend, so likely won’t get eyes on it till next week, but I’ve got a reminder to take a look!

you are the best, thanks @thor!