Update std::env::temp_dir to use GetTempPath2 on Windows when available.

[talagrand]

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

Compatibility risk: Software which relies on temporary files to communicate between SYSTEM and non-SYSTEM
processes may be affected by this change. In many cases, such patterns may be vulnerable to the very
attacks the new API was introduced to harden against.
I'm unclear on the Rust project's tolerance for such change-of-behavior in the standard library. If anything,
this PR is meant to raise awareness of the issue and hopefully start the conversation.

How tested: Taking the example code from the documentation and running it through psexec (from SysInternals) on
Win10 and Win11.
On Win10:
C:\test>psexec -s C:\test\main.exe
<...>
Temporary directory: C:\WINDOWS\TEMP\

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

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information.

[joshtriplett]

This seems reasonable to me. It's certainly theoretically possible that two processes could be communicating via temporary files by passing just filenames and assuming the same temporary directory (or worse, by using constant filenames), but that doesn't seem like a safe or portable assumption. It doesn't seem at all likely that this would break anything. Nonetheless, we should document the behavior change in the release notes.

[joshtriplett]

CC @rust-lang/libs-api for wider review. I think we can safely merge this.

[BurntSushi]

I think this should probably require an FCP. In particular, this is changing the documented behavior of a routine. I suppose it's questionable that we listed the precise platform call we're using in the first place. But if we don't consider changing it to be breaking the contract, then we should probably either remove it or make the wording more relaxed.

One concern I have here is the difference in behavior between Windows versions. I don't really know how to measure it, though, it seems like it could be a source of subtle bugs in a "poorly" written program.

Another thought here is: why didn't Windows just change the behavior of of GetTempPath instead of adding a new GetTempPath2? A guess would be that they were worried enough about compatibility to do it. Should we be similarly worried?

Finally, what are other language ecosystems doing here, if anything?

We should also ping the Windows folks. (I forget how.)

[ChrisDenton]

Purely from a security point of view, it would be great to have this. But I agree with the concern about the current documentation being a bit too explicit, which is (I think) not normally how the std handles implementation details. E.g. std::io says this: https://doc.rust-lang.org/std/io/#platform-specific-behavior

Many I/O functions throughout the standard library are documented to indicate what various library or syscalls they are delegated to. This is done to help applications both understand what’s happening under the hood as well as investigate any possibly unclear semantics. Note, however, that this is informative, not a binding contract. The implementation of many of these functions are subject to change over time and may call fewer or more syscalls/library functions.

---

We should also ping the Windows folks. (I forget how.)

@rustbot ping windows

[rustbot]

Error: Only Rust team members can ping teams.

Please let @rust-lang/release know if you're having trouble with this bot.

[BurntSushi]

@rustbot ping windows

[rustbot]

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

[wesleywiser]

This seems like a reasonable change to me as well. I also agree with the concerns regarding the current documentation. I think a note similar to the one @ChrisDenton referred to in the std::io docs would be appropriate here.

[talagrand]

Updated documentation as suggested - I consolidated the platform-specific behavior notice centrally for std. The previous commit simply adds a disclaimer to the "env" module if that's deemed preferrable.
@BurntSushi unclear how to initiate an FCP, do we have a documented process surrounding that, or is that something initiated by the libraries team?

[talagrand]

Updated documentation for temp_dir to fall more in line with the pattern established by std::io.

[joshtriplett]

@rfcbot merge

Let's see if we have consensus to make this change.

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[yaahc]

Another thought here is: why didn't Windows just change the behavior of of GetTempPath instead of adding a new GetTempPath2? A guess would be that they were worried enough about compatibility to do it. Should we be similarly worried?

I want to reiterate @BurntSushi's question here on why didn't Windows do this as a breaking change? If they did it because they're worried that makes me wonder if we should be mirroring their API choice and instead introduce a new method + deprecate the old one.

[sivadeilra]

Another thought here is: why didn't Windows just change the behavior of of GetTempPath instead of adding a new GetTempPath2? A guess would be that they were worried enough about compatibility to do it. Should we be similarly worried?

I want to reiterate @BurntSushi's question here on why didn't Windows do this as a breaking change? If they did it because they're worried that makes me wonder if we should be mirroring their API choice and instead introduce a new method + deprecate the old one.

Windows dev, here. Windows is under enormous pressure not to make any change in behavior, far beyond the threshold that most people would consider reasonable, because we can't predict the behavior of 10,000s of legacy apps, and neither can our customers. Somewhere out there, there's an app that calls GetTempPath as admin, writes a bunch of files, and then accidentally relies on them being visible to non-admin processes. Or, there's a scheduled task which cleans up %TEMP%, but doesn't clean up the admin version of %TEMP%, so some drive eventually fills up on some obscure machine, and it takes ages for someone to track it down. And then we get yelled at.

But I don't think the same is true for the Rust ecosystem. The distinction boils down to the size and age of the Windows app ecosystem -- it's huge, and much of it is literally decades old. By contrast, the Rust ecosystem is far younger, smaller, and is used to "evergreen" development, where apps track gradual updates to their tools.

I think a reasonable position here is to commit the change to switch to GetTempPath2, and then simply observe what happens. Perhaps it could be gated on whether you're running a nightly compiler or not.

I think erring on the side of safety (security) is best, in this particular situation. As a Windows dev, we would love to just push our customers into the safer behavior. And for 99.9% of our customers, that's probably the right call. But when the 0.1% might just happen to include, say, a hospital's IT department, it's harder for us to make that call for breaking behavior.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[talagrand]

I'll reiterate the previous feedback here on the importance for Windows to not change the externally visible behavior of Win32 APIs release-over-release to maximize app compatibility due to the size and age of the ecosystem.

Additionally, supposing Windows had changed the behavior of GetTempPath, there would be limited ability for apps to restore the old behavior if absolutely necessary, since it's the lowest-level API.
As for what language runtimes should do, this dips into the cultural expectations of what that runtime is providing - is it an abstract platform, implemented in terms of what the underlying system provides, or is it a direct binding of underlying OS primitives? Personally, I would expect a C++ std::thread to bind directly to an OS thread, but the story around a java.lang.Thread to be more nuanced.

In the case of a Rust, I'd imagine the standard library falls somewhere along this abstraction gradient. I believe that, despite std::fs::File::create existing, there is still a role to play for winapi::um::fileapi::CreateFileW existing for developers who want the precise semantics of the underlying OS. In a similar vein, developers who want the precise OS behavior here could always call into winapi::um::fileapi::GetTempPathW to get that behavior. But ideally, we wouldn't want folks to have to reach into winapi to get safe semantics.

My primary motivation for proposing this change is to expose safe defaults, and establish safety even when higher-order abstractions are built on top (as in the "tempfile" crate, to pick an example).

[m-ou-se]

@bors r+

[bors]

📌 Commit 1d26e41 has been approved by m-ou-se

[bors]

⌛ Testing commit 1d26e41 with merge faab3b7772961f5b31c11e39765257318d2eb6b4...

[bors]

💥 Test timed out

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[wesleywiser]

Seems spurious?

@bors retry