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

Following discussion on #43724, change os/exec to take the
approach of golang.org/x/sys/execabs, refusing to respect
path entries mentioning relative paths by default.

Code that insists on being able to find executables in relative
directories in the path will need to add a couple lines to override the error.

See the updated package docs in exec.go for more details.

Fixes #43724.
Fixes #43947.

Change-Id: I73c1214f322b60b4167a23e956e933d50470fe13
Reviewed-on: https://go-review.googlesource.com/c/go/+/381374
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>

6 files changed, 217 insertions, 16 deletions

diff --git a/src/os/exec/dot_test.go b/src/os/exec/dot_test.go
new file mode 100644
index 0000000000..ca6b0950da
--- /dev/null
+++ b/src/os/exec/dot_test.go
@@ -0,0 +1,88 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package exec_test
+
+import (
+	"errors"
+	"internal/testenv"
+	"io/ioutil"
+	"os"
+	. "os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+func TestLookPath(t *testing.T) {
+	testenv.MustHaveExec(t)
+
+	tmpDir := filepath.Join(t.TempDir(), "testdir")
+	if err := os.Mkdir(tmpDir, 0777); err != nil {
+		t.Fatal(err)
+	}
+
+	executable := "execabs-test"
+	if runtime.GOOS == "windows" {
+		executable += ".exe"
+	}
+	if err := ioutil.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0777); err != nil {
+		t.Fatal(err)
+	}
+	cwd, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		if err := os.Chdir(cwd); err != nil {
+			panic(err)
+		}
+	}()
+	if err = os.Chdir(tmpDir); err != nil {
+		t.Fatal(err)
+	}
+	origPath := os.Getenv("PATH")
+	defer os.Setenv("PATH", origPath)
+
+	// Add "." to PATH so that exec.LookPath looks in the current directory on all systems.
+	// And try to trick it with "../testdir" too.
+	for _, dir := range []string{".", "../testdir"} {
+		os.Setenv("PATH", dir+string(filepath.ListSeparator)+origPath)
+		t.Run("PATH="+dir, func(t *testing.T) {
+			good := dir + "/execabs-test"
+			if found, err := LookPath(good); err != nil || !strings.HasPrefix(found, good) {
+				t.Fatalf("LookPath(%q) = %q, %v, want \"%s...\", nil", good, found, err, good)
+			}
+			if runtime.GOOS == "windows" {
+				good = dir + `\execabs-test`
+				if found, err := LookPath(good); err != nil || !strings.HasPrefix(found, good) {
+					t.Fatalf("LookPath(%q) = %q, %v, want \"%s...\", nil", good, found, err, good)
+				}
+			}
+
+			if _, err := LookPath("execabs-test"); err == nil {
+				t.Fatalf("LookPath didn't fail when finding a non-relative path")
+			} else if !errors.Is(err, ErrDot) {
+				t.Fatalf("LookPath returned unexpected error: want Is ErrDot, got %q", err)
+			}
+
+			cmd := Command("execabs-test")
+			if cmd.Err == nil {
+				t.Fatalf("Command didn't fail when finding a non-relative path")
+			} else if !errors.Is(cmd.Err, ErrDot) {
+				t.Fatalf("Command returned unexpected error: want Is ErrDot, got %q", cmd.Err)
+			}
+			cmd.Err = nil
+
+			// Clearing cmd.Err should let the execution proceed,
+			// and it should fail because it's not a valid binary.
+			if err := cmd.Run(); err == nil {
+				t.Fatalf("Run did not fail: expected exec error")
+			} else if errors.Is(err, ErrDot) {
+				t.Fatalf("Run returned unexpected error ErrDot: want error like ENOEXEC: %q", err)
+			}
+		})
+	}
+}
diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 91c2e003d8..7b72ffece4 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -18,6 +18,71 @@
 // Note that the examples in this package assume a Unix system.
 // They may not run on Windows, and they do not run in the Go Playground
 // used by golang.org and godoc.org.
+//
+// Executables in the current directory
+//
+// The functions Command and LookPath look for a program
+// in the directories listed in the current path, following the
+// conventions of the host operating system.
+// Operating systems have for decades included the current
+// directory in this search, sometimes implicitly and sometimes
+// configured explicitly that way by default.
+// Modern practice is that including the current directory
+// is usually unexpected and often leads to security problems.
+//
+// To avoid those security problems, as of Go 1.19, this package will not resolve a program
+// using an implicit or explicit path entry relative to the current directory.
+// That is, if you run exec.LookPath("go"), it will not successfully return
+// ./go on Unix nor .\go.exe on Windows, no matter how the path is configured.
+// Instead, if the usual path algorithms would result in that answer,
+// these functions return an error err satisfying errors.Is(err, ErrDot).
+//
+// For example, consider these two program snippets:
+//
+//	path, err := exec.LookPath("prog")
+//	if err != nil {
+//		log.Fatal(err)
+//	}
+//	use(path)
+//
+// and
+//
+//	cmd := exec.Command("prog")
+//	if err := cmd.Run(); err != nil {
+//		log.Fatal(err)
+//	}
+//
+// These will not find and run ./prog or .\prog.exe,
+// no matter how the current path is configured.
+//
+// Code that always wants to run a program from the current directory
+// can be rewritten to say "./prog" instead of "prog".
+//
+// Code that insists on including results from relative path entries
+// can instead override the error using an errors.Is check:
+//
+//	path, err := exec.LookPath("prog")
+//	if errors.Is(err, exec.ErrDot) {
+//		err = nil
+//	}
+//	if err != nil {
+//		log.Fatal(err)
+//	}
+//	use(path)
+//
+// and
+//
+//	cmd := exec.Command("prog")
+//	if errors.Is(cmd.Err, exec.ErrDot) {
+//		cmd.Err = nil
+//	}
+//	if err := cmd.Run(); err != nil {
+//		log.Fatal(err)
+//	}
+//
+// Before adding such overrides, make sure you understand the
+// security implications of doing so.
+// See https://go.dev/blog/path-security for more information.
 package exec
 
 import (
@@ -134,7 +199,7 @@ type Cmd struct {
 	ProcessState *os.ProcessState
 
 	ctx             context.Context // nil means none
-	lookPathErr     error           // LookPath error, if any.
+	Err             error           // LookPath error, if any.
 	finished        bool            // when Wait was called
 	childFiles      []*os.File
 	closeAfterStart []io.Closer
@@ -173,7 +238,7 @@ func Command(name string, arg ...string) *Cmd {
 	}
 	if filepath.Base(name) == name {
 		if lp, err := LookPath(name); err != nil {
-			cmd.lookPathErr = err
+			cmd.Err = err
 		} else {
 			cmd.Path = lp
 		}
@@ -200,7 +265,7 @@ func CommandContext(ctx context.Context, name string, arg ...string) *Cmd {
 // In particular, it is not suitable for use as input to a shell.
 // The output of String may vary across Go releases.
 func (c *Cmd) String() string {
-	if c.lookPathErr != nil {
+	if c.Err != nil {
 		// failed to resolve path; report the original requested path (plus args)
 		return strings.Join(c.Args, " ")
 	}
@@ -335,7 +400,7 @@ func (c *Cmd) Run() error {
 // lookExtensions does not search PATH, instead it converts `prog` into `.\prog`.
 func lookExtensions(path, dir string) (string, error) {
 	if filepath.Base(path) == path {
-		path = filepath.Join(".", path)
+		path = "." + string(filepath.Separator) + path
 	}
 	if dir == "" {
 		return LookPath(path)
@@ -363,10 +428,10 @@ func lookExtensions(path, dir string) (string, error) {
 // The Wait method will return the exit code and release associated resources
 // once the command exits.
 func (c *Cmd) Start() error {
-	if c.lookPathErr != nil {
+	if c.Err != nil {
 		c.closeDescriptors(c.closeAfterStart)
 		c.closeDescriptors(c.closeAfterWait)
-		return c.lookPathErr
+		return c.Err
 	}
 	if runtime.GOOS == "windows" {
 		lp, err := lookExtensions(c.Path, c.Dir)
@@ -845,3 +910,12 @@ func addCriticalEnv(env []string) []string {
 	}
 	return append(env, "SYSTEMROOT="+os.Getenv("SYSTEMROOT"))
 }
+
+// ErrDot indicates that a path lookup resolved to an executable
+// in the current directory due to ‘.’ being in the path, either
+// implicitly or explicitly. See the package documentation for details.
+//
+// Note that functions in this package do not return ErrDot directly.
+// Code should use errors.Is(err, ErrDot), not err == ErrDot,
+// to test whether a returned error err is due to this condition.
+var ErrDot = errors.New("cannot run executable found relative to current directory")
diff --git a/src/os/exec/lp_plan9.go b/src/os/exec/lp_plan9.go
index e8826a5083..68224814d1 100644
--- a/src/os/exec/lp_plan9.go
+++ b/src/os/exec/lp_plan9.go
@@ -30,7 +30,11 @@ func findExecutable(file string) error {
 // directories named by the path environment variable.
 // If file begins with "/", "#", "./", or "../", it is tried
 // directly and the path is not consulted.
-// The result may be an absolute path or a path relative to the current directory.
+// On success, the result is an absolute path.
+//
+// In older versions of Go, LookPath could return a path relative to the current directory.
+// As of Go 1.19, LookPath will instead return that path along with an error satisfying
+// errors.Is(err, ErrDot). See the package documentation for more details.
 func LookPath(file string) (string, error) {
 	// skip the path lookup for these prefixes
 	skip := []string{"/", "#", "./", "../"}
@@ -49,6 +53,9 @@ func LookPath(file string) (string, error) {
 	for _, dir := range filepath.SplitList(path) {
 		path := filepath.Join(dir, file)
 		if err := findExecutable(path); err == nil {
+			if !filepath.IsAbs(path) {
+				return path, &Error{file, ErrDot}
+			}
 			return path, nil
 		}
 	}
diff --git a/src/os/exec/lp_unix.go b/src/os/exec/lp_unix.go
index 5db6c5e109..9833205663 100644
--- a/src/os/exec/lp_unix.go
+++ b/src/os/exec/lp_unix.go
@@ -31,7 +31,11 @@ func findExecutable(file string) error {
 // LookPath searches for an executable named file in the
 // directories named by the PATH environment variable.
 // If file contains a slash, it is tried directly and the PATH is not consulted.
-// The result may be an absolute path or a path relative to the current directory.
+// Otherwise, on success, the result is an absolute path.
+//
+// In older versions of Go, LookPath could return a path relative to the current directory.
+// As of Go 1.19, LookPath will instead return that path along with an error satisfying
+// errors.Is(err, ErrDot). See the package documentation for more details.
 func LookPath(file string) (string, error) {
 	// NOTE(rsc): I wish we could use the Plan 9 behavior here
 	// (only bypass the path if file begins with / or ./ or ../)
@@ -52,6 +56,9 @@ func LookPath(file string) (string, error) {
 		}
 		path := filepath.Join(dir, file)
 		if err := findExecutable(path); err == nil {
+			if !filepath.IsAbs(path) {
+				return path, &Error{file, ErrDot}
+			}
 			return path, nil
 		}
 	}
diff --git a/src/os/exec/lp_windows.go b/src/os/exec/lp_windows.go
index e7a2cdf142..dab5770298 100644
--- a/src/os/exec/lp_windows.go
+++ b/src/os/exec/lp_windows.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 )
 
 // ErrNotFound is the error resulting if a path search failed to find an executable file.
@@ -53,10 +54,14 @@ func findExecutable(file string, exts []string) (string, error) {
 
 // LookPath searches for an executable named file in the
 // directories named by the PATH environment variable.
-// If file contains a slash, it is tried directly and the PATH is not consulted.
 // LookPath also uses PATHEXT environment variable to match
 // a suitable candidate.
-// The result may be an absolute path or a path relative to the current directory.
+// If file contains a slash, it is tried directly and the PATH is not consulted.
+// Otherwise, on success, the result is an absolute path.
+//
+// In older versions of Go, LookPath could return a path relative to the current directory.
+// As of Go 1.19, LookPath will instead return that path along with an error satisfying
+// errors.Is(err, ErrDot). See the package documentation for more details.
 func LookPath(file string) (string, error) {
 	var exts []string
 	x := os.Getenv(`PATHEXT`)
@@ -75,18 +80,34 @@ func LookPath(file string) (string, error) {
 	}
 
 	if strings.ContainsAny(file, `:\/`) {
-		if f, err := findExecutable(file, exts); err == nil {
+		f, err := findExecutable(file, exts)
+		if err == nil {
 			return f, nil
-		} else {
-			return "", &Error{file, err}
 		}
+		return "", &Error{file, err}
 	}
-	if f, err := findExecutable(filepath.Join(".", file), exts); err == nil {
-		return f, nil
+
+	// On Windows, creating the NoDefaultCurrentDirectoryInExePath
+	// environment variable (with any value or no value!) signals that
+	// path lookups should skip the current directory.
+	// In theory we are supposed to call NeedCurrentDirectoryForExePathW
+	// "as the registry location of this environment variable can change"
+	// but that seems exceedingly unlikely: it would break all users who
+	// have configured their environment this way!
+	// https://docs.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-needcurrentdirectoryforexepathw
+	// See also go.dev/issue/43947.
+	if _, found := syscall.Getenv("NoDefaultCurrentDirectoryInExePath"); !found {
+		if f, err := findExecutable(filepath.Join(".", file), exts); err == nil {
+			return f, &Error{file, ErrDot}
+		}
 	}
+
 	path := os.Getenv("path")
 	for _, dir := range filepath.SplitList(path) {
 		if f, err := findExecutable(filepath.Join(dir, file), exts); err == nil {
+			if !filepath.IsAbs(f) {
+				return f, &Error{file, ErrDot}
+			}
 			return f, nil
 		}
 	}
diff --git a/src/os/exec/lp_windows_test.go b/src/os/exec/lp_windows_test.go
index 34abe09d04..1f609fffd0 100644
--- a/src/os/exec/lp_windows_test.go
+++ b/src/os/exec/lp_windows_test.go
@@ -8,6 +8,7 @@
 package exec_test
 
 import (
+	"errors"
 	"fmt"
 	"internal/testenv"
 	"io"
@@ -36,6 +37,9 @@ func cmdLookPath(args ...string) {
 func cmdExec(args ...string) {
 	cmd := exec.Command(args[1])
 	cmd.Dir = args[0]
+	if errors.Is(cmd.Err, exec.ErrDot) {
+		cmd.Err = nil
+	}
 	output, err := cmd.CombinedOutput()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Child: %s %s", err, string(output))
@@ -329,7 +333,7 @@ var lookPathTests = []lookPathTest{
 	},
 }
 
-func TestLookPath(t *testing.T) {
+func TestLookPathWindows(t *testing.T) {
 	tmp := t.TempDir()
 	printpathExe := buildPrintPathExe(t, tmp)