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

Except for the discussion around go generate, the reaction here seems overwhelmingly in favor, and this is the kind of change we should land early in the cycle. It sounds like the consensus is to accept this proposal.

Do I have that right?

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 https://github.com/golang/go/issues/42420?

@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

Right. That snippet did feel off. I’ve never been a fan of altering global state and it doesn’t get much more global than the environment. Somehow I thought it would make sense to “put things back the way they were”, but, yeah, it’s not like anyone else really cares what a process does to its own environment, apart from the children that process may spawn.

My primary concern here is that it’s possible to interfere with an application that depends on stuff off the PATH just by altering it’s working directory: trick it into launching “fun” surprises or make it error out (respectively, before and after the implementation of this proposal). I wouldn’t want either of those happening.

I used to put a script at the project root sometimes, in my PHP days, naming it phpunit, just like the real executable, to invoke PHPUnit simply as ./phpunit instead of ./vendor/bin/phpunit. Yes, I’m that lazy. Sometimes I added some CLI parameters to that script, if a common set could be distilled, to have less to type. The problem now is that such a script in a current working directory would prevent a tool written in Go from intentionally starting the PHPUnit (in this example) from the PATH, unless NoDefaultCurrentDirectoryInExePath is set. Or it could be Git. Or Docker. Or whatever other executable expected to be found on the PATH. Would that be a problem on Windows systems only? Would others be safe from such a denial of service condition, or does it require a more extensive workaround to be reliably mitigated cross-platform?

Asking because not only am I lazy, I’m also selfish, smug and opinionated — I don’t use Windows and I think nobody else ever should either. If it’s only on that OS that Go programs break this way, well, stuff breaks on that OS all the time, and those who still use it regardless ought to have had resigned to that fate by now. It’s not like there is a lack of choice of alternatives. So, yeah, if it’s only that OS that goes bork yet again now, then, meh, fine, whatever, they had it coming to them.

If I specifically wanted command execution to proceed exhibiting weird quirks characteristic of a single specific platform (https://github.com/golang/go/issues/43947#issuecomment-780877042 lists steps that should be taken to locate and start an executable precisely the way that OS does), I’d look for that in a specific platform compatibility layer. Not in a supposedly sane cross-platform standard library API.

This is done now.

RE: the break due to lookupPathErr, it’s likely the case that there’s code out there that’s going to be using old versions of execabs for a while (e.g. stable versions of gopls, other libraries that were bulk updated, most of all users like me who took the advice of the blog posts and released code with that dependency).

I think execabs should be changed to a no-op on new builds, yes, but will a fake lookupPathErr be left behind to ensure that code built with newer toolchains continues to work? Since newer versions of Go would not do the relative lookup, I would think that old execabs touching an unexported field (but doing nothing) would maintain compatibility.

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

My only outstanding concern would be the inability to ignore the current directory in the path search, as mentioned in previous comments. The primary security issue is addressed by avoiding a potentially malicious executable in the current directory, but possibly transformed into a less severe denial-of-service. Rather than causing execution of malicious code, a hypothetical attacker’s rogue file will now prevent the Go-implemented service from performing its function. However, your proposal to separately respect the NoDefaultCurrentDirectoryInExePath environment variable would overcome that.

It might be convenient to avoid the new error if dot is explicitly present in PATH, though one could certainly achieve that via some additional application code in conjunction with errors.Is(err, exec.ErrDot). I don’t think that I foresee having such a use case in my own work, and the new scenario will be that the normally discouraged/“bad” practice is what takes additional effort to enable.

@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.

@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 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?

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.