std::process::Command resolve() to avoid security issues on Windows?

On Linux, using the std::process::Command API resolves commands using the $PATH environment variable. This represents a security decision, as by default only protected locations are included in this search path (/bin, /usr/bin, etc). It is possible for a user to change this behaviour and include additional search paths.

On Windows, at least with the current implementation, this security safeguard is overridden. Instead of only searching for the executable in @PATH@, the implementation looks for the binaries in the current path of the process first. This allows an attacker to hijack a Rust tool by placing a suitably named executable in the current working directory of a program that uses this API.

This has lead to CVE-2021-3013 (see cli: fix arbitrary execution of program bug · BurntSushi/ripgrep@229d1a8 · GitHub for the fix). In the fixing commit, basically the logic that UNIX (well, bash) uses to locate the binary is replicated, by parsing the PATH environment variable and looking for suitable matches in the directories, see the resolve_binary method.

Should some method be added to std::process::Command that resolves the name of an executable to an absolute path in a safe way by parsing the @PATH@ environment variable, in order to offer a simple way for code to be safe on Windows as well? Having some way to circumvent Window's defaults here and having this in the documentation will probably help everyone in writing safer, cross-platform code. Or maybe is there already something that does this and I was just not able to locate it?

4 Likes

Definitely there should be an option to add/remove cwd from the lookup path. With free reign, I'd say to "fix" Command to be consistent cross platform and not include cwd in the lookup path by default (assuming $env:PATH doesn't have . in it, because that's a bad idea, though legal), but given we're stable, matching the native shell environment is a decent enough choice to stick with.

I think the best API would be to provide a builder method that sets cwd lookup to one of { Native, Yes, No }, and a version of fs::canonicalize that only allows $PATH-relative or absolute names, not cwd-relative.


For a little context, cmd.exe does include cwd in the lookup chain for executables. I don't actually know whether MSDOS had a concept of $PATH, or if there were just shell builtins. For backwards compatibility, cmd.exe doesn't change, ever. MS even had difficulty finally splitting cmd.exe into the typical term / window architecture due to compatibility guarantees.

But the recommended Modern Windows approach is to use Powershell, which is shipped by default with the OS, or even install-requiring cross-platform Powershell inside Windows Terminal. No matter which version of PS you use, PS does not include cwd in executable path lookup, and requires ./ to execute a local executable. There's even a note printed when you fail to lookup a name in PATH but there is one locally, telling you to use ./ if you mean the local file.

So while including cwd in path lookup isn't default on Windows, it's still the behavior in the MS recommended shell environment, and as such something likely to be familiar to most Windows developers.

3 Likes

This is also how win32 CreateProcess works -- the PATH is sixth in the list of places that are searched.

3 Likes

To be fair, three of those are basically a living history of where Windows has stored system binaries over the decades.

But yeah, by using lpApplicationName with an absolute path, we can skip the legacy win32 rules and implement our own behaviour however we like. This also allows running executables whose path exceeds MAX_PATH.


EDIT: Looking at the implementation it already seems like we do some fun stuff: rust/process.rs at ff5522fc1ae2c1d66fb2a465f48e03732fa8570b · rust-lang/rust · GitHub

I was also tempted to replicate the PATH search to fix an issue with a bug in macOS's posix_spawnp implementation (see Don't use posix_spawn_file_actions_addchdir_np on macOS. by ehuss · Pull Request #80537 · rust-lang/rust · GitHub). I ended up doing a more conservative fix, though.

There may also be an interaction with https://github.com/rust-lang/rust/issues/37868, where it needs to decide how relative paths are resolved.

I'm surprised at this, as I thought CreateProcessW was limited to MAX_PATH no matter what (even with \\?\ paths). Or maybe it is lpCurrentDirectory that doesn't support MAX_PATH? Or maybe that needs a manifest?

Yes lpCurrentDirectory is limited, as is the path in lpCommandLine. But lpApplicationName isn't limited.