Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
rsc opened this issue Jan 15, 2021 · 53 comments
Closed

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

rsc opened this issue Jan 15, 2021 · 53 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Jan 15, 2021

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:

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.

@golang golang locked and limited conversation to collaborators Jan 15, 2021
@rsc rsc added the Proposal label Jan 19, 2021
@rsc rsc added this to Incoming in Proposals (old) Jan 19, 2021
@rsc rsc added this to the Proposal milestone Jan 19, 2021
@rsc rsc changed the title placeholder proposal: os/exec: return error when PATH lookup would use current directory Jan 19, 2021
@golang golang unlocked this conversation Jan 19, 2021
@DmitriyMV
Copy link

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
Copy link
Contributor

bcmills commented Jan 20, 2021

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
Copy link

mislav commented Jan 20, 2021

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
Copy link

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
Copy link
Member

@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
Copy link

@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
Copy link
Contributor

bcmills commented Jan 22, 2021

@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
Copy link
Contributor

bcmills commented Jan 22, 2021

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

@dylan-bourque
Copy link

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
Copy link

dylan-bourque commented Jan 22, 2021

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
Copy link
Contributor

dolmen commented Jan 26, 2021

@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
Copy link
Contributor Author

rsc commented Jan 27, 2021

@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
Copy link
Contributor Author

rsc commented Jan 27, 2021

@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
Copy link
Contributor Author

rsc commented Jan 27, 2021

@dylan-bourque It sounds like somehow you have

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

Why not instead write

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

?

@dylan-bourque
Copy link

@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
Copy link
Contributor Author

rsc commented Jan 27, 2021

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
Copy link

@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
Copy link

Change https://go.dev/cl/408577 mentions this issue: [release-branch.go1.18] os/exec: return clear error for missing cmd.Path

@gopherbot
Copy link

Change https://go.dev/cl/408578 mentions this issue: [release-branch.go1.17] os/exec: return clear error for missing cmd.Path

gopherbot pushed a commit that referenced this issue May 27, 2022
Following up on CL 403694, there is a bit of confusion about
when Path is and isn't set, along with now the exported Err field.
Catch the case where Path and Err (and lookPathErr) are all unset
and give a helpful error.

Updates #52574
Followup after #43724.

Fixes #53057
Fixes CVE-2022-30580

Change-Id: I03205172aef3801c3194f5098bdb93290c02b1b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/403759
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit 960ffa9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408577
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
gopherbot pushed a commit that referenced this issue May 27, 2022
Following up on CL 403694, there is a bit of confusion about
when Path is and isn't set, along with now the exported Err field.
Catch the case where Path and Err (and lookPathErr) are all unset
and give a helpful error.

Updates #52574
Followup after #43724.

Fixes #53056
Fixes CVE-2022-30580

Change-Id: I03205172aef3801c3194f5098bdb93290c02b1b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/403759
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit 960ffa9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408578
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@rsc rsc removed their assignment Jun 23, 2022
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Jun 28, 2022
… explicit one

If the current directory is also listed explicitly in %PATH%,
this changes the behavior of LookPath to prefer the explicit name for it
(and thereby avoid ErrDot).

However, in order to avoid running a different executable from what
would have been run by previous Go versions, we still return the
implicit path (and ErrDot) if it refers to a different file entirely.

Fixes #53536.
Updates #43724.

Change-Id: I7ab01074e21a0e8b07a176e3bc6d3b8cf0c873cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/414054
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Jul 28, 2022
The changes are likely to break users, and we need
to make it easy to unbreak without code changes.

For #43724.
Fixes #53962.

Change-Id: I105c5d6c801d354467e0cefd268189c18846858e
Reviewed-on: https://go-review.googlesource.com/c/go/+/419794
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
… explicit one

If the current directory is also listed explicitly in %PATH%,
this changes the behavior of LookPath to prefer the explicit name for it
(and thereby avoid ErrDot).

However, in order to avoid running a different executable from what
would have been run by previous Go versions, we still return the
implicit path (and ErrDot) if it refers to a different file entirely.

Fixes golang#53536.
Updates golang#43724.

Change-Id: I7ab01074e21a0e8b07a176e3bc6d3b8cf0c873cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/414054
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
The changes are likely to break users, and we need
to make it easy to unbreak without code changes.

For golang#43724.
Fixes golang#53962.

Change-Id: I105c5d6c801d354467e0cefd268189c18846858e
Reviewed-on: https://go-review.googlesource.com/c/go/+/419794
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
mholt pushed a commit to caddyserver/xcaddy that referenced this issue Sep 12, 2022
Fix xcaddy run on windows with go 1.19

See golang/go#43724
@clarkmcc
Copy link

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
Copy link
Contributor

@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
Copy link

Manbeardo commented Dec 13, 2022

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
Copy link

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.

sad log

@ianlancetaylor
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod release-blocker
Projects
No open projects
Development

No branches or pull requests