Skip to content

Conversation

@talagrand
Copy link
Contributor

As a security measure, Windows 11 introduces a new temporary directory API, GetTempPath2.
When the calling process is running as SYSTEM, a separate temporary directory
will be returned inaccessible to non-SYSTEM processes. For non-SYSTEM processes
the behavior will be the same as before.

This can help mitigate against attacks such as this one:
https://medium.com/csis-techblog/cve-2020-1088-yet-another-arbitrary-delete-eop-a00b97d8c3e2

This PR updates the filesystem library to call into this new API when available.
Note that there is a small possible compatibility impact if software relies on temporary files to
communicate between SYSTEM and non-SYSTEM processes and one uses GetTempPath (prior to this change)
and the other GetTempPath2. In many cases, such patterns may be vulnerable to the very attacks the new
API was introduced to harden against. The standard itself requires only that this API should return
"an unspecified directory path suitable for temporary files," though GetTempPath is mentioned as an
example implementation.

How tested: Sample program printing out the value of std::filesystem::temp_directory_path()
run through psexec (from SysInternals) on Win10 and Win11.

On Win10:
C:\test>psexec -s C:\test\main.exe
<...>
Temp directory is "C:\WINDOWS\TEMP\"

On Win11:
C:\test>psexec -s C:\test\main.exe
<...>
Temp directory is "C:\Windows\SystemTemp\"

@talagrand talagrand requested a review from a team as a code owner October 25, 2021 19:22
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej added the affects redist Results in changes to separately compiled bits label Dec 15, 2021
@StephanTLavavej StephanTLavavej added the filesystem C++17 filesystem label May 6, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 8, 2022
@strega-nil-ms

This comment was marked as resolved.

As a security measure, Windows 11 introduces a new temporary directory API, GetTempPath2.
When the calling process is running as SYSTEM, a separate temporary directory
will be returned inaccessible to non-SYSTEM processes. For non-SYSTEM processes
the behavior will be the same as before.

This can help mitigate against attacks such as this one:
https://medium.com/csis-techblog/cve-2020-1088-yet-another-arbitrary-delete-eop-a00b97d8c3e2

This PR updates the filesystem library to call into this new API when available.
Note that there is a small possible compatibility impact if software relies on temporary files to
communicate between SYSTEM and non-SYSTEM processes and one uses GetTempPath (prior to this change)
and the other GetTempPath2. In many cases, such patterns may be vulnerable to the very attacks the new
API was introduced to harden against. The standard itself requires only that this API should return
"an unspecified directory path suitable for temporary files," though GetTempPath is mentioned as an
example implementation.

How tested: Sample program printing out the value of std::filesystem::temp_directory_path()
run through psexec (from SysInternals) on Win10 and Win11.

On Win10:
C:\test>psexec -s C:\test\main.exe
<...>
Temp directory is "C:\\WINDOWS\\TEMP\\"

On Win11:
C:\test>psexec -s C:\test\main.exe
<...>
Temp directory is "C:\\Windows\\SystemTemp\\"
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for porting this to the new function pointer table. 😸 I'll validate and push changes for the remaining minor issues I noticed.

@StephanTLavavej StephanTLavavej removed their assignment Jul 15, 2022
@StephanTLavavej
Copy link
Member

Validated and pushed changes. I was slightly daring in replacing the compare_exchange_weak with a store - please meow if anything looks wrong or weird.

I observe that __crtGetTempPath2W doesn't need to be extern "C" because it's used within the STL's DLL only, but imitating the Windows API is reasonable and the risk of parameter/return type mismatches is very low (not zero, as we saw recently, but low). It also grants us noexcept implicitly as we build (almost all of) the STL with /EHsc, so I think the PR is fine as-is.

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Jul 15, 2022

Changes look good to me! I usually prefer to use cmpexchg when possible, since I was under the impression that on some platforms, cmpexchg (weak) is the only atomic store instruction; however, this was just me misremembering that cmpexchg (weak) is the only operation available to implement interlocked operations (also we only run on ARM and 386 derived platforms anyways so shrug)

@StephanTLavavej StephanTLavavej self-assigned this Jul 19, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I've had to push an additional commit to fix OneCore (where we don't have the dynamic loading machinery) and /clr:pure (where filesys.cpp is compiled, but winapisupp.cpp is not).

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Jul 20, 2022

I've had to push an additional commit to fix OneCore (where we don't have the dynamic loading machinery) and /clr:pure (where filesys.cpp is compiled, but winapisupp.cpp is not).

thanks I hate it 😭

@StephanTLavavej StephanTLavavej merged commit d066aef into microsoft:main Jul 20, 2022
@StephanTLavavej
Copy link
Member

Thanks for this security improvement and congratulations on your first microsoft/STL commit, @talagrand! Also thanks to @strega-nil-ms for refactoring this PR for binary compatibility! 🛡️ 😻 🎉

@talagrand
Copy link
Contributor Author

Thank you for pushing this forward :)

fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
…t#2302)

Co-authored-by: Nicole Mazzuca <mazzucan@outlook.com>
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

affects redist Results in changes to separately compiled bits enhancement Something can be improved filesystem C++17 filesystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants