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.

[the8472]

It looks like searching the current directory is historical baggage on windows since CreateProcess does that by default.
And there's no recommendation from microsoft to only pass absolute paths to it? That seems strange.

The 32-bit Windows system directory.
The Windows directory.

Aren't those two on the PATH by default?

I mostly like the unix approach. If it's relative and not in your PATH it fails. Except for the case where you don't have PATH set at all (absent, not empty), then execvp has some questionable fallbacks too. But that can be solved by using execve instead. Then one has full control over the relative path resolution.

[ChrisDenton]

I'd love for us to simplify this to just checking PATH but my concern is possible breakage.

Aren't those two on the PATH by default?

They are indeed by default. But they can be removed or reordered. So removing the special handling would be an observable change (albeit hopefully a low impact one).

[dpaoliello]

Providing a Windows developer perspective on this:

Existing Behaviors:

1. CreateProcess (Win32)
   1. If lpApplicationName is provided, then the current directory is searched.
   2. If lpApplicationName is NULL, then there is a long set of paths that are search that is essentially: the path to the current executable, the current directory, various Windows directories, the PATH env var.
2. ShellExecute (Win32):
   1. If lpDirectory is set, then that path is searched.
   2. Otherwise the current directory is searched.
3. Process.Start (.NET)
   1. If ProcessStartInfo.ShellExecute is true (default for .NET Framework), then the ShellExecute behavior is used and ProcessStartInfo.WorkingDirectory can be used to control lpDirectory.
   2. Otherwise (default for .NET Core and .NET 5+), the executable and its arguments are passed to CreateProcess via lpCommandLine, so the lpApplicationName=NULL behavior is used (i.e., behavior 1.i: search the list of paths).
4. Launcher.LaunchFileAsync (WinRT): the IStorageFile object contains an absolute path, thus avoiding this issue altogether.

Security Concerns:

The question to ask here is: "what can an attacker do that they couldn't do before?".

With loading from the current directory, this is an issue: when opening an item via Explorer the current directory is the item's directory, if that is writable by an unprivileged user and it is a privileged user that is opening the item then the unprivileged user may be able to influence what code is running beyond the document being opened. (e.g., Imagine if "*.dog" is opened by "Doggo.exe" which internally invokes "Scritches.exe" expecting it to be on the PATH - an attacker can then place "Rover.dog" and a malicious "Scritches.exe" in a globally writable directory and so cause the malicious exe to be run even though the victim didn't click on it and Rover.dog is completely legitimate).

With loading from the same directory as the executable, this is not an issue: if the executable's directory (and, thus, the executable) is writable by an unprivileged user, then they can control what code the privileged user is running by replacing the original executable itself - no need to mess with search paths, etc. (Given the previous example, if "Doggo.exe" and "Scritches.exe" are both in a globally writable directory, then the attacker can replace Doggo.exe with a malicious version rather than trying to exploit the legitimate Doggo.exe's use of Scritches.exe). I know that there are a lot of "user-local installation" applications that place their binaries into a user-writable location, but these all rely on placing the files within a user's directory thus a different (unprivileged) user cannot write to those files: only the owner (which requires the attacker to already control that account) or privileged users (who can already write to the system files) can write to these files, neither of which gains anything by doing this.

Recommendation:

I would leave Command as it currently is (i.e., with the search order specified in #87704): this provides the closest behavior to CreateProcess and modern versions of .NET while also removing the possible attack vector of searching in the current directory.

[bk2204]

There are a couple of problems with the current approach. First, it works differently on different systems. Simply using PATH everywhere, and only PATH, makes it very easy to reason about how the code works. If the user wants the Windows directory or the executable directory in PATH, then they set that; otherwise, they don't. (In my experience, the Windows directory is almost always there anyway.) Adding other directories by default means that users who want to control that can't. For example, if I want a specific binary (e.g. ssh) which shares a name with a binary in the Windows directory but I don't want to use the Windows binary ever, then I have to manually implement my own PATH walking.

The one relevant different is that on Unix, an empty component is considered the current directory, but because many Windows systems end up with an empty PATH component because of how PATH gets set, that would be a mistake on Windows, and I don't recommend it.

The additional problem with loading files from the directory that the executable is in is temporary directories or download directories. If I download files with a browser, they won't be overwritten unless I specifically ask to do that. If I have a legitimate Rust binary that is supposed to invoke, say, cmd.exe, and it's invoked from the download directory, then I have a problem if someone has downloaded cmd.exe in the download directory and it's malicious. (You can substitute this with any other binary you like.) A malicious website cannot rename files, but it can cause malicious binaries to be automatically downloaded into the download directory without user interaction in at least some cases. This same approach has been used to exploit things like self-extracting Zip files in temporary directories.

While I admit I'm not a Windows developer (although I play one in my role as maintainer of Git LFS), my experience is the default behaviour of CreateProcess is a giant security disaster. It does a lot of very questionable things which are highly insecure and would be cause for multiple CVEs today (and have caused countless CVEs to be issued in other software), and its behaviour should not be a consideration here.

[ChrisDenton]

While I admit I'm not a Windows developer (although I play one in my role as maintainer of Git LFS), my experience is the default behaviour of CreateProcess is a giant security disaster. It does a lot of very questionable things which are highly insecure and would be cause for multiple CVEs today (and have caused countless CVEs to be issued in other software), and its behaviour should not be a consideration here.

The (by default) insecure behaviour of CreateProcess has yet again been shown with recent Git for Windows vulnerability where the current directory was used in the search path. I think everybody agrees that Rust removing this was the right call.

Has there been similar CVEs or other documented exploits that took advantage of the executable's directory being in the search path? The absence of such CVEs would not necessarily undermine the argument for removal but their presence would certainly help bolster the case.

[bk2204]

The most common instance of this that gets CVEs is DLL hijacking, where a program in a temporary directory loads DLLs that are beside it. However, there's also this article:

Quite some driver packages available on Windows Update contain besides their primary (kernel) drivers, which are typically installed via .inf scripts, also so-called "satellites", i.e. additional programs and/or DLLs, which provide interfaces to configure and control the driver and its hardware. These satellites are typically installed by separate setup programs that are run during driver installation.
[…]
These setup programs are typically self-extractors which use the %TEMP%\ directory to unpack and run their payload. Instead to create a properly secured subdirectory there, some self-extractors place their payload in the %TEMP%\ directory itself. Many, if not most payloads are but susceptible to CAPEC-471: Search Order Hijacking and execute files planted by unprivileged users in the %TEMP%\ alias %SystemRoot%\Temp\ directory with administrative privileges and access rights.

It goes on to describe a particular NVIDIA driver that had this vulnerability.

This is not the only instance; it was just the easiest one to find. DLL hijacking vulnerabilities are more common, so it's actually a little difficult to find vulnerabilities where an executable was the problem, but I assure you that this has been a problem with self-extracting executables from time to time when they extract to the temporary directory.