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

Update filesystem library to use GetTempPath2 on Windows 11 #2302

Merged
merged 8 commits into from
Jul 20, 2022

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
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesys.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/awint.hpp Outdated Show resolved Hide resolved
stl/src/awint.hpp Outdated Show resolved Hide resolved
stl/src/awint.hpp Outdated Show resolved Hide resolved
stl/src/awint.hpp Outdated Show resolved Hide resolved
stl/src/filesys.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/winapisupp.cpp Outdated Show resolved Hide resolved
@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.

stl/src/winapisupp.cpp Show resolved Hide resolved
stl/src/filesys.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesys.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesys.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
@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 :)

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