Insecure behavior in std::process::Command on Windows

[bk2204]

There appears to be a security vulnerability in std::process::Command, specifically on Windows. The documentation for the new function states this:

If program is not an absolute path, the PATH will be searched in an OS-defined way.

The search path to be used may be controlled by setting the PATH environment variable on the Command, but this has some implementation limitations on Windows (see issue #37519).

What this does not say is that these implementation limitations cause the program to be executed from the current directory, even if that is not in PATH. This is a vulnerability and this behavior has been known to be insecure on Unix for many years.

As a result, it is not possible to use Rust to invoke other than an absolute path when the current directory is untrusted, such as when a user is working in cloned Git repository. Moreover, this fact is not even documented, and as such, I would reasonably expect that directories outside of PATH are not searched.

I've attached a tarball of a cargo project which contains two trivial programs. To reproduce the problem, do the following:

1. Open PowerShell.
2. Extract the tarball.
3. Change into path-check.
4. Run cargo build.
5. Change into target\debug.
6. Run $env:Path = "C:\Users\User\.cargo\bin" to set a fixed PATH without the current directory.
7. Run & '.\path-check.exe'.
8. Notice that exploit.exe is also invoked and that its output is displayed.

I used a Windows 10 Development VM for this purpose. I don't habitually use Windows; I'm mostly a Linux user, so my apologies if the steps are hard to understand.

I realize that the current functionality exists because Rust looks only for .exe files and not other types of files, and it therefore it otherwise passes files which do not exist in PATH to CreateProcessW, which has the insecure behavior. However, just because Microsoft has designed an insecure interface does not mean Rust should permit the same behavior.

The approach that Go uses for searching for executables, other than the use of the current directory, is generally good. It uses PATHEXT (or, if absent, a hardcoded list) to look for extensions, and then considers each component in PATH (rejecting empty components), looking for each extension in turn. This algorithm (with the current directory) is used by CMD, and therefore doing the same thing without the current directory would be normal and expected for Windows users, and also secure.

Note that unlike on Unix, on Windows empty components in PATH should not be treated as the current directory. Unfortunately, many Windows machines contain a trailing semicolon in PATH and as such, that would preserve insecure semantics.

I originally reported this to the private security list, but it was determined that this behavior had been discussed publicly before, and thus, opening an issue was appropriate. It remains a vulnerability, and a CVE should still be issued, though.

This came to my attention because Go has the same insecure behavior and they have deliberately chosen to retain the vulnerability for compatibility, so every Go program that runs on Windows (including Git LFS, which I maintain) must contain special code to work around this. Since Rust has already documented secure behavior (using PATH), all that needs to be done is actually fixing the implementation, which is less of a problem.

Meta

rustc --version:

rustc 1.54.0 (a178d0322 2021-07-26)

I've verified that the vulnerable code exists in a recent HEAD.

[Urgau]

You should check out this PR #87704

[bk2204]

Yes, that would solve the security problem. It would also end up breaking invoking any batch files or other programs that people are running right now without using a suffix, which would be less desirable. I"ll make a comment on the PR.

[steveklabnik]

cc @rust-lang/libs

[yaahc]

Seems like @ChrisDenton's PR already solves this and doesn't introduce the breakage that @bk2204 was worried about. I'm assuming we can close this when #87704 merges.

[cuviper]

There's potential breakage if anyone depended on any of those implicit paths that #87704 will no longer search. That may still be the way to go -- I'm just saying that it's not entirely worry-free.

[ChrisDenton]

Indeed. I am happy to change my PR so it's a closer match to the existing behaviour (current directory aside). This would fix the immediate issue while having the lowest chance of breaking things. This would also allow for a separate decision on removing implicit paths at a later date. Hm, actually that does sound like a good idea.

[ChrisDenton]

#87704 fixed the specific issue raised here (looking for the exe in the current directory) but didn't close this. I guess partly because the PR was opened before this issue was filed and also because it was mentioned that the uses of the current exe directory is also an issue:

Unfortunately, the directory from which the application loaded isn't a secure option, either. If someone has a self-installing executable that's downloaded into a temporary directory or downloads directory, it isn't safe to execute programs from that directory. This behavior, except with DLLs, is the cause of a large number of security vulnerabilities.

Which @yaahc felt at the time was a compelling reason to remove:

I find the arguments about the directory the application was loaded from being insecure to be rather compelling

The impact of removing the current directory from the search path seems to have been minimal. Therefore I'd like to propose removing the exe's directory from the search path.

To spell out the changes to the search strategy, I've listed it here with the changed crossed out:

1. The directories that are listed in the child's PATH environment variable.
2. The directory from which the application loaded.
3. The 32-bit Windows system directory.
4. The Windows directory.
5. The directories that are listed in the PATH environment variable.