rsc commented 3 years ago

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:

38736, which removes the implicit dot lookup from LookPath on Windows
42420, to add LookPathAbs that doesn't return relative results.
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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

dylan-bourque commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@dylan-bourque It sounds like somehow you have

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

Why not instead write

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

?

dylan-bourque commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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

rsc commented 3 years ago

@dylan-borque, if $PWD is absolute then resolving to $PWD/bin/foo will be fine, right?

rsc commented 3 years ago

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?

dholmesdell commented 3 years ago

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.

dylan-bourque commented 3 years ago

@dylan-borque, if $PWD is absolute then resolving to $PWD/bin/foo will be fine, right?

@rsc Yes, with the caveat that any existing directives of the form //go:generate foo .... will need to be explicitly updated to be //go:generate $PWD/bin/foo ... instead.

My objection was more about the premise that any binary that happens to be in or below $PWD should be treated as malicious. My example about go generate was just a specific scenario where we have intentionally set up an environment where we want precisely the behavior (finding a binary under $PWD by doing a $PATH lookup) that is now being treated as malicious..

If you do move forward with this, I would suggest that, at a minimum, this potentially breaking change should be called out prominently in the release notes.

dolmen commented 3 years ago

//go:generate $PWD/bin/foo ... will not work on Windows.

dylan-bourque commented 3 years ago

Follow up question: Does this new error not occur when/if the resulting path is absolute but still within $PWD. I ask because we had one of our //go:generate directives fail and updating Makefile to do export PATH=$PWD/bin:$PATH instead of export PATH=./bin:$PATH "fixed" it. It's still finding the same binary in the same place, but the error doesn't occur if we pre-pend an absolute path rather than a relative one to $PATH.

rsc commented 3 years ago

@dylan-borque, yes, the error only happens for relative PATH entries. Sorry if that wasn't clear. Absolute PATH entries are always respected, even if they describe the current directory. So yes, we expect that you'd update the Makefile to use $PWD/bin:$PATH instead of ./bin:$PATH (the latter stops being usable when you cd into a different directory).

rsc commented 3 years ago

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

rsc commented 3 years ago

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

antichris commented 3 years ago

Just to be clear, it is only on Windows that the following must be done to securely* execute "prog" from the PATH:

const n = "NoDefaultCurrentDirectoryInExePath"
if v, ok := os.LookupEnv(n); ok {
    defer os.Setenv(n, v)
} else {
    defer os.Unsetenv(n)
}
os.Setenv(n, "1")

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

Is my understanding correct?

*) By "securely" meaning "mitigating the new denial of service vulnerability that, as of this proposal, replaces the previous arbitrary code execution one".

rsc commented 3 years ago

@antichris, I suppose you could do that, but I don't see why you would restore the old settings. That just introduces races if you have different goroutines executing programs. If you really need to make your Go program's os/exec behave silently differently from the standard Windows PATH lookup, then the way to do that would be to just do the os.Setenv once at startup in package main, with no attempt to revert it.

antichris commented 3 years ago

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.