os/exec: return error when PATH lookup would use current directory

[rsc]

We recently published a new package golang.org/x/sys/execabs, which is a forwarding wrapper around os/exec that makes three changes:

• execabs.LookPath changes the result of exec.LookPath in the case where a PATH search returns an executable in the current directory (dot). In that case, execabs.LookPath returns an error instead. The error text is “prog resolves to executable in current directory (./prog)”

• execabs.Command changes the result of exec.Command in the same case. It arranges that the subsequent cmd.Run or cmd.Start will return that same error.

• execabs.CommandContext does the same.

As yet another possible answer in the “what to do about PATH lookups on Windows” saga, perhaps we should change os/exec to do these things by default. I am filing this proposal to help us discuss and decide whether to take this path. (For more background, see the blog post https://blog.golang.org/path-security.)

The proposal can be summarized as “replace os/exec with golang.org/x/sys/execabs”.

To recap how we got here, the fundamental problem is that lots of Go programs do things like exec.Command("prog") and expect that prog will come from a system directory listed in the PATH. It is a surprise and in some cases a security problem that on Windows prog will be taken from dot instead when prog.exe (or prog.bat, prog.com, ...) exists. The same is true on Unix for users who have an explicit dot or empty-string element in their PATH.

We already have three issues with possible approaches to solving this problem. They are:

• proposal: os/exec: make LookPath not look in dot implicitly on Windows #38736, which removes the implicit dot lookup from LookPath on Windows
• proposal: os/exec: add LookPathAbs that refuses to return relative paths #42420, to add LookPathAbs that doesn't return relative results.
• proposal: os/exec: use LookPathAbs by default  #42950, to make exec.Command use LookPathAbs by default (assuming it is added)

The problem with #38736 is that it silently changes the behavior of programs on Windows. Where exec.Command("prog") previously found and ran .\prog.exe, it would now either silently switch to a prog.exe from the PATH (surprise!) or return an error that prog could not be found (even though it used to be; confusion!). The same is true of exec.LookPath.

#42420 avoids the problem of changing existing behavior by introducing a new function exec.LookPathAbs that never looks in dot. Clearly that doesn’t surprise or confuse anyone. But it also doesn’t fix any security problems.

#42950 extends #42420 by changing exec.Command to use exec.LookPathAbs by default. That brings back the surprise and confusion of #38736, but only for exec.Command and not exec.LookPath. And compared to #38736, #42420+#42950 has the added complexity of adding new API (LookPathAbs).

None of these are great.

The proposal in this issue, to adopt execabs semantics as the os/exec semantics, fixes the problems. Execabs doesn’t remove dot from the PATH lookup. Instead it reports use of dot as an error. This avoids the surprise of running a different program. And the reported error is very clear about what happened. Instead of a generic “program not found” it gives an error that avoids the confusion:

prog resolves to executable in current directory (./prog)

And of course because programs from the current directory are not being executed, that fixes the security problem too.

The specific changes this issue proposes in os/exec are:

• Add a new var ErrDot = errors.New("executable in current directory")
• Change LookPath: if it would have chosen to return path, nil where path is an executable in the current directory found by a PATH lookup, it now does return path, err where err satisfies errors.Is(err, ErrDot).
• Change Cmd: add a new field Err error which is returned by Start or Run if not set. This field replaces the current unexported field lookPathErr.

Consider this client code:

path, err := exec.LookPath("prog")
if err != nil {
	log.Fatal(err)
}
use(path)

cmd := exec.Command("prog")
if err := cmd.Run(); err != nil {
	log.Fatal(err)
}

With the proposed changes, the code would no longer find and run ./prog on Unix (when dot is in the PATH), nor .\prog.exe on Windows (regardless of PATH), addressing the security issue.

Programs that want to require the current directory to be used (ignoring PATH) can change "prog" to "./prog" (that works on both Unix and Windows systems). This change ("prog" to "./prog") is compatible with older versions of os/exec.

Programs that want to allow the use of the current directory in conjunction with PATH can add a few new lines of code, marked with <<<:

path, err := exec.LookPath("prog")
if errors.Is(err, exec.ErrDot) {        // <<<
	err = nil                       // <<<
}                                       // <<<
if err != nil {
	log.Fatal(err)
}
use(path)

cmd := exec.Command("prog")
if errors.Is(cmd.Err, exec.ErrDot) {    // <<<
	cmd.Err = nil                   // <<<
}                                       // <<<
if err := cmd.Run(); err != nil {
	log.Fatal(err)
}

This should give programs the flexibility to opt back into the old behavior when necessary.
Of course, this change (adding the errors.Is checks) is not compatible with older versions of os/exec, but we expect this need to be rare. We expect most programs that intentionally run programs from the current directory to update to the ./prog form.

Windows users, would this proposal break your programs?

You can check today by replacing

import "os/exec"

with

import exec "golang.org/x/sys/execabs"

in your source code. No other changes are needed to get the effect. (Of course, golang.org/x/sys/execabs does not provide ErrDot, so the errors.Is stanzas cannot be written in this simulation of the proposal.)

Thoughts? Comments? Concerns? Thanks very much.

[DmitriyMV]

As a part time Windows user, I don't think I expect exec.Command("prog") to work the same way as I expect launching stuff from cmd.exe. The cmd.exe behavior is surprising, but it's already set in stone several decades ago.

[bcmills]

I don't understand why the Cmd.Err field is necessary. Couldn't the workaround program instead be written as

	const short = "prog"
	actual := short
	if abs, err := exec.LookPath(short); errors.Is(err, exec.ErrDot) {
		actual = abs
	}
	cmd := exec.Command(actual)
	cmd.Args[0] = short

?

[mislav]

Thank you for the detailed description, @rsc!

If this proposal would be adopted, how should we write the following code so it would be safe to run inside an untrusted directory?

c := exec.Command("git", "status")
s, err := c.Output()

With this proposal, if the current directory has a .\git.exe or a .\git.bat (or a similar file matching any of the PATHEXTs), running this command would result in a failure. Since there might be legitimate reasons why the current directory could contain a git.bat, would we have any option to instead execute git as found in the system PATH instead of in the current directory?

I understand that this proposal is meant to prevent silent failures in programs that relied on the Windows behavior. I'm just wondering, if this proposal gets accepted, what are our options for running commands in untrusted directories (i.e. so that the contents of the current directory is entirely ignored)? Might this proposal ship in tandem with #42420?

[dylan-bourque]

For some additional context, I recently updated to Go 1.15.7 (which includes the security patch for this issue) and it broke build-time scripts that run on Mac and/or Linux systems. We have a build process where we intentionally go install build tools to <project-dir>/bin (in order to avoid installing local tools to "global" directories that happen to appear in $PATH) and prepend <project-dir>/bin to $PATH. We then have //go:generate custom-tool .... directives in .go source files within the project, with the obvious expectation that it will find the custom-tool binary that we just "installed" into <project-dir>/bin.

Unfortunately for me, running go generate ./... from the project root now fails with an error that says:

custom-tool resolves to executable in current directory (./bin/custom-tool)

The suggested workaround is to prepend ./ (or ./bin/ in my case) would work but it now makes those go:generate directives sensitive to which directory go generate is run from, i.e. a go:generate directive in a sub-package would work when go generate is run from the project root but fail if the command was run from the sub-package directory.

While I fully understand the security concern here, I take issue with the premise that all situations that involve executing a binary that resides within the current directory are invalid. With this change to Go, I now have approximately 800 go:generate directives that I need to investigate to see whether or not they are broken.

[seankhliao]

@dylan-bourque wouldn't using the absolute path to the bin directory be a more proper fix for your situation?

# assuming this is set from tooling in the root of the project
export PATH=$(pwd)/bin:$PATH

[dylan-bourque]

@seankhliao your assumption is correct. We do export PATH=$(pwd)/bin:$PATH in the project-level Makefile with the expectation that go generate ./... will find the various tools in that location.

The issue with injecting an absolute path into those //go:generate directives is that that absolute path is not the same on every machine.

As I said, it seems like updating those directives to use //go:generate ../../custom-tool ..... would technically work but that introduces the $PWD sensitivity I mentioned.

[bcmills]

@dylan-bourque, go:generate directions should not be sensitive to $PWD. Per the go:generate design doc, emphasis added:

command is the generator (such as yacc) to be run, corresponding to an executable file that can be run locally; it must either be in the shell path (gofmt) or fully qualified (/usr/you/bin/mytool) and is run in the package directory.

[bcmills]

(And note that we currently use exactly that approach in the Go standard library: see https://golang.org/cl/261499.)

[dylan-bourque]

All of mine weren't sensitive to $PWD before. The commands being run were always located via the shell path, and we went out of our way to ensure that those would be the correct commands.

That is no longer the case with Go 1.15.7, though. The same //go:generate custom-tool .... commands that worked yesterday fail today.

[dylan-bourque]

and is run in the package directory

I see where my confusion was now.

As a test, I added //go:generate echo PWD is $PWD in a sub-package and ran go generate ./... from the project root. The output is the project root directory, not the sub-package. However //go:generate ls -l shows the contents of the sub-package folder.

Even with this new information, though, I still have hundreds of go:generate directives that were working perfectly before and now need to be checked and potentially updated to include a relative path to the command binary. ☹️

I think my point stands that not every invocation of a command within the current directory should be treated as malicious, though.

[dolmen]

@rsc This proposal helps for the case where you want to catch the case of a local executable file. But it doesn't help for the case where you want to LookPath ignoring a local executable and looking just in %PATH%: this proposal doesn't provide a LookPathIgnoreDot and still forces to use an alternate version of the whole os/exec package.

Concrete example: In my goeval project I want to run go run to compile a Go source file. golang.org/x/sys/execabs catches a security issue if there is a go.bat in the current directory, but that's not what is helpful for users. I need instead an exec.Command that lookup go only from %PATH%. I just want the Unix behavior (which Windows user can opt-in on non-Go programs because of NeedCurrentDirectoryForExePathW).

Update: It looks like my only option is to vendor both os/exec and golang.org/x/sys/execabs in my project (with the patched version of LookPath I want) to get the behavior I need.

Update 2:

$ GOOS=windows GOARCH=amd64 go list -deps . | grep exec
internal/syscall/execenv
os/exec
golang.org/x/sys/execabs
internal/execabs

Do I also have to vendor that internal/execabs?

[rsc]

@dolmen, yes, there is no "do a non-standard PATH lookup". There is only "do the standard PATH lookup and refuse an insecure result". As noted above, the problem with "do a non-standard PATH lookup" is that it silently differs from the standard system behavior, which will cause mystery and confusion.

golang.org/x/sys/execabs catches a security issue if there is a go.bat in the current directory, but that's not what is helpful for users.

Those same users really probably should delete go.bat. If your goeval doesn't trip over it, something else will.

I think it would be fine, as an independent change, to respect the presence of the NoDefaultCurrentDirectoryInExePath environment variable. If that variable were found in the environment, then LookPath would never look in dot and the security check would never trigger.

[rsc]

@bcmills, cmd.Err is not strictly necessary - users can call LookPath themselves - but it is error-prone to assume the result of LookPath will be valid input to exec.Command. In general that's not true, since LookPath returns a clean path, so go not ./go. That is, abs in your code snippet is not always an absolute path. It's far easier and less error-prone to apply the check after the exec.Command lookup instead of keeping two lookups in sync.

Also, there are now two packages (execabs and safeexec) that function as exec.Command-workalikes and are hampered in that by not being able to set Err themselves. Safeexec uses a not-as-nice API and execabs uses ugly reflection. Better to expose Err and let any future wrappers do what they need to do. That field (lookPathErr) is the only setup field in exec.Command that's not exported.

[rsc]

@dylan-bourque It sounds like somehow you have

//go:generate PATH=./bin:$PATH foo ...

Why not instead write

//go:generate ./bin/foo ...

?

[dylan-bourque]

@rsc what we have are a bunch of //go:generate foo ... (no ./ prefix). foo is explicitly installed in the project-level bin/ folder and the Makefile that's invoking go generate ./... is pre-pending $PWD/bin to $PATH so that those directives will find the "right" foo when doing the path search.

For a more concrete example, one of those tools is protoc-gen-go, where the generated code is coupled to the library code that corresponds to the version of the generator used. We have lots of projects such that it's not feasible to guarantee that every one of them is always using the exact same version of google.golang.org/protobuf. Go modules is actually addressing that issue for us, but it means that we can't install the protoc-gen-go binary to $GOPATH/bin because different projects are bound to different versions. In order to solve that, we have our build process configured to install protoc-gen-go into <projectdir>/bin and do export PATH=$PWD/bin:$PATH in each project's Makefile so that each one is invoking the exact version that it's bound to via go.mod.

With the security fix in Go 1.14.4 and 1.15.7, we're now looking at having to update a significant number of our //go:generate directives to include explicit relative path prefixes (./bin/, ../bin/, ../../bin/, etc.) for the tool(s) being invoked.

My point here is that we are intentionally placing an executable within the current directory and explicitly configuring the environment such that that binary will be invoked (just like anyone would do in a shell script, etc.). This proposed change to treat any binary within $PWD as malicious will, in my opinion at least, break valid use cases.

[rsc]

the Makefile that's invoking go generate ./... is pre-pending $PWD/bin to $PATH so that those directives will find the "right" foo when doing the path search.

If $PWD is not an absolute path due to some mistake in go generate, we should fix that.

[dylan-bourque]

@rsc That (making $PWD correct for go generate) is the issue @bcmills submitted above. That doesn't fully cover my scenario, though. We're invoking our custom generation tools with no explicit path (relative or absolute) and expecting them to be resolved via os.LookPath. Since we're intentionally installing those tools into $PWD/bin (for the reasons I mentioned before), the change in 1.14.4/1.15.7 has introduced a new failure case because the resolved path to the tool ends up being $PWD/bin/foo

[gopherbot]

Change https://go.dev/cl/414054 mentions this issue: os/exec: on Windows, suppress ErrDot if the implicit path matches the explicit one

[gopherbot]

Change https://go.dev/cl/419794 mentions this issue: os/exec: add GODEBUG setting to opt out of ErrDot changes

[clarkmcc]

Is there a way to bypass this error at runtime in Go 1.18? I was under the impression that this was going to be released in Go 1.19 per the release notes but it looks like it also affects programs compiled with Go 1.18. We've got a few Go services deployed remotely at customer locations that can't self-upgrade due to this change, and we're looking for a way to bypass it temporarily.

[ianlancetaylor]

@clarkmcc This change is not in Go 1.18. I suggest that you open a new issue with more details.

That said, if your Go 1.18 somehow has the Go 1.19 changes, perhaps it also has the documented GODEBUG change that disables it. See https://pkg.go.dev/os/exec.

[Manbeardo]

This appears to cause substantial problems for go binaries that are run by Bazel and run subprocesses of their own because the directories in PATH are always relative to the cwd when running a command in Bazel. Bazel avoids using absolute paths by design in order to make builds more hermetic.

This appears to be blocking the use of gqlgen in Bazel because gqlgen depends upon golang.org/x/tools/go/packages, which uses golang.org/x/sys/execabs instead of os/exec, so I can't use the GODEBUG setting to opt out. :/

[Manbeardo]

AFAICT, resolution of executables in nested directories doesn't seem like it's within the scope of the security vulnerability unless there's some other commonly-exploitable pattern of people putting relative paths on their PATH.

[ianlancetaylor]

@Manbeardo Please open a new issue about these concerns. Thanks.