Merge pull request from GHSA-fqfh-778m-2v32

Ensure that only PATH is searched when shelling out to external commands

cmd/gh/main.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cli/cli/pkg/cmdutil"
 	"github.com/cli/cli/update"
 	"github.com/cli/cli/utils"
+	"github.com/cli/safeexec"
 	"github.com/mattn/go-colorable"
 	"github.com/mgutz/ansi"
 	"github.com/spf13/cobra"
@@ -107,7 +108,13 @@ func main() {
 		}
 
 		if isShell {
-			externalCmd := exec.Command(expandedArgs[0], expandedArgs[1:]...)
+			exe, err := safeexec.LookPath(expandedArgs[0])
+			if err != nil {
+				fmt.Fprintf(stderr, "failed to run external command: %s", err)
+				os.Exit(3)
+			}
+
+			externalCmd := exec.Command(exe, expandedArgs[1:]...)
 			externalCmd.Stderr = os.Stderr
 			externalCmd.Stdout = os.Stdout
 			externalCmd.Stdin = os.Stdin

git/git.go

@@ -10,9 +10,11 @@ import (
 	"os/exec"
 	"path"
 	"regexp"
+	"runtime"
 	"strings"
 
 	"github.com/cli/cli/internal/run"
+	"github.com/cli/safeexec"
 )
 
 // ErrNotOnAnyBranch indicates that the user is in detached HEAD state
@@ -37,7 +39,10 @@ func (r TrackingRef) String() string {
 // ShowRefs resolves fully-qualified refs to commit hashes
 func ShowRefs(ref ...string) ([]Ref, error) {
 	args := append([]string{"show-ref", "--verify", "--"}, ref...)
-	showRef := exec.Command("git", args...)
+	showRef, err := GitCommand(args...)
+	if err != nil {
+		return nil, err
+	}
 	output, err := run.PrepareCmd(showRef).Output()
 
 	var refs []Ref
@@ -57,7 +62,10 @@ func ShowRefs(ref ...string) ([]Ref, error) {
 
 // CurrentBranch reads the checked-out branch for the git repository
 func CurrentBranch() (string, error) {
-	refCmd := GitCommand("symbolic-ref", "--quiet", "HEAD")
+	refCmd, err := GitCommand("symbolic-ref", "--quiet", "HEAD")
+	if err != nil {
+		return "", err
+	}
 
 	output, err := run.PrepareCmd(refCmd).Output()
 	if err == nil {
@@ -78,13 +86,19 @@ func CurrentBranch() (string, error) {
 }
 
 func listRemotes() ([]string, error) {
-	remoteCmd := exec.Command("git", "remote", "-v")
+	remoteCmd, err := GitCommand("remote", "-v")
+	if err != nil {
+		return nil, err
+	}
 	output, err := run.PrepareCmd(remoteCmd).Output()
 	return outputLines(output), err
 }
 
 func Config(name string) (string, error) {
-	configCmd := exec.Command("git", "config", name)
+	configCmd, err := GitCommand("config", name)
+	if err != nil {
+		return "", err
+	}
 	output, err := run.PrepareCmd(configCmd).Output()
 	if err != nil {
 		return "", fmt.Errorf("unknown config key: %s", name)
@@ -94,12 +108,23 @@ func Config(name string) (string, error) {
 
 }
 
-var GitCommand = func(args ...string) *exec.Cmd {
-	return exec.Command("git", args...)
+var GitCommand = func(args ...string) (*exec.Cmd, error) {
+	gitExe, err := safeexec.LookPath("git")
+	if err != nil {
+		programName := "git"
+		if runtime.GOOS == "windows" {
+			programName = "Git for Windows"
+		}
+		return nil, fmt.Errorf("unable to find git executable in PATH; please install %s before retrying", programName)
+	}
+	return exec.Command(gitExe, args...), nil
 }
 
 func UncommittedChangeCount() (int, error) {
-	statusCmd := GitCommand("status", "--porcelain")
+	statusCmd, err := GitCommand("status", "--porcelain")
+	if err != nil {
+		return 0, err
+	}
 	output, err := run.PrepareCmd(statusCmd).Output()
 	if err != nil {
 		return 0, err
@@ -123,10 +148,13 @@ type Commit struct {
 }
 
 func Commits(baseRef, headRef string) ([]*Commit, error) {
-	logCmd := GitCommand(
+	logCmd, err := GitCommand(
 		"-c", "log.ShowSignature=false",
 		"log", "--pretty=format:%H,%s",
 		"--cherry", fmt.Sprintf("%s...%s", baseRef, headRef))
+	if err != nil {
+		return nil, err
+	}
 	output, err := run.PrepareCmd(logCmd).Output()
 	if err != nil {
 		return []*Commit{}, err
@@ -154,7 +182,10 @@ func Commits(baseRef, headRef string) ([]*Commit, error) {
 }
 
 func CommitBody(sha string) (string, error) {
-	showCmd := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
+	showCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
+	if err != nil {
+		return "", err
+	}
 	output, err := run.PrepareCmd(showCmd).Output()
 	if err != nil {
 		return "", err
@@ -164,7 +195,10 @@ func CommitBody(sha string) (string, error) {
 
 // Push publishes a git ref to a remote and sets up upstream configuration
 func Push(remote string, ref string, cmdOut, cmdErr io.Writer) error {
-	pushCmd := GitCommand("push", "--set-upstream", remote, ref)
+	pushCmd, err := GitCommand("push", "--set-upstream", remote, ref)
+	if err != nil {
+		return err
+	}
 	pushCmd.Stdout = cmdOut
 	pushCmd.Stderr = cmdErr
 	return run.PrepareCmd(pushCmd).Run()
@@ -179,7 +213,10 @@ type BranchConfig struct {
 // ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` part of git config
 func ReadBranchConfig(branch string) (cfg BranchConfig) {
 	prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch))
-	configCmd := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix))
+	configCmd, err := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix))
+	if err != nil {
+		return
+	}
 	output, err := run.PrepareCmd(configCmd).Output()
 	if err != nil {
 		return
@@ -209,21 +246,28 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) {
 }
 
 func DeleteLocalBranch(branch string) error {
-	branchCmd := GitCommand("branch", "-D", branch)
-	err := run.PrepareCmd(branchCmd).Run()
-	return err
+	branchCmd, err := GitCommand("branch", "-D", branch)
+	if err != nil {
+		return err
+	}
+	return run.PrepareCmd(branchCmd).Run()
 }
 
 func HasLocalBranch(branch string) bool {
-	configCmd := GitCommand("rev-parse", "--verify", "refs/heads/"+branch)
-	_, err := run.PrepareCmd(configCmd).Output()
+	configCmd, err := GitCommand("rev-parse", "--verify", "refs/heads/"+branch)
+	if err != nil {
+		return false
+	}
+	_, err = run.PrepareCmd(configCmd).Output()
 	return err == nil
 }
 
 func CheckoutBranch(branch string) error {
-	configCmd := GitCommand("checkout", branch)
-	err := run.PrepareCmd(configCmd).Run()
-	return err
+	configCmd, err := GitCommand("checkout", branch)
+	if err != nil {
+		return err
+	}
+	return run.PrepareCmd(configCmd).Run()
 }
 
 func parseCloneArgs(extraArgs []string) (args []string, target string) {
@@ -252,7 +296,10 @@ func RunClone(cloneURL string, args []string) (target string, err error) {
 
 	cloneArgs = append([]string{"clone"}, cloneArgs...)
 
-	cloneCmd := GitCommand(cloneArgs...)
+	cloneCmd, err := GitCommand(cloneArgs...)
+	if err != nil {
+		return "", err
+	}
 	cloneCmd.Stdin = os.Stdin
 	cloneCmd.Stdout = os.Stdout
 	cloneCmd.Stderr = os.Stderr
@@ -262,7 +309,10 @@ func RunClone(cloneURL string, args []string) (target string, err error) {
 }
 
 func AddUpstreamRemote(upstreamURL, cloneDir string) error {
-	cloneCmd := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL)
+	cloneCmd, err := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL)
+	if err != nil {
+		return err
+	}
 	cloneCmd.Stdout = os.Stdout
 	cloneCmd.Stderr = os.Stderr
 	return run.PrepareCmd(cloneCmd).Run()
@@ -274,7 +324,10 @@ func isFilesystemPath(p string) bool {
 
 // ToplevelDir returns the top-level directory path of the current repository
 func ToplevelDir() (string, error) {
-	showCmd := exec.Command("git", "rev-parse", "--show-toplevel")
+	showCmd, err := GitCommand("rev-parse", "--show-toplevel")
+	if err != nil {
+		return "", err
+	}
 	output, err := run.PrepareCmd(showCmd).Output()
 	return firstLine(output), err
 

git/remote.go

@@ -3,7 +3,6 @@ package git
 import (
 	"fmt"
 	"net/url"
-	"os/exec"
 	"regexp"
 	"strings"
 
@@ -45,7 +44,10 @@ func Remotes() (RemoteSet, error) {
 	remotes := parseRemotes(list)
 
 	// this is affected by SetRemoteResolution
-	remoteCmd := exec.Command("git", "config", "--get-regexp", `^remote\..*\.gh-resolved$`)
+	remoteCmd, err := GitCommand("config", "--get-regexp", `^remote\..*\.gh-resolved$`)
+	if err != nil {
+		return nil, err
+	}
 	output, _ := run.PrepareCmd(remoteCmd).Output()
 	for _, l := range outputLines(output) {
 		parts := strings.SplitN(l, " ", 2)
@@ -107,8 +109,11 @@ func parseRemotes(gitRemotes []string) (remotes RemoteSet) {
 
 // AddRemote adds a new git remote and auto-fetches objects from it
 func AddRemote(name, u string) (*Remote, error) {
-	addCmd := exec.Command("git", "remote", "add", "-f", name, u)
-	err := run.PrepareCmd(addCmd).Run()
+	addCmd, err := GitCommand("remote", "add", "-f", name, u)
+	if err != nil {
+		return nil, err
+	}
+	err = run.PrepareCmd(addCmd).Run()
 	if err != nil {
 		return nil, err
 	}
@@ -136,6 +141,9 @@ func AddRemote(name, u string) (*Remote, error) {
 }
 
 func SetRemoteResolution(name, resolution string) error {
-	addCmd := exec.Command("git", "config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
+	addCmd, err := GitCommand("config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
+	if err != nil {
+		return err
+	}
 	return run.PrepareCmd(addCmd).Run()
 }

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/MakeNowJust/heredoc v1.0.0
 	github.com/briandowns/spinner v1.11.1
 	github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684
+	github.com/cli/safeexec v1.0.0
 	github.com/cpuguy83/go-md2man/v2 v2.0.0
 	github.com/enescakir/emoji v1.0.0
 	github.com/google/go-cmp v0.5.2

go.sum

@@ -44,6 +44,8 @@ github.com/briandowns/spinner v1.11.1/go.mod h1:QOuQk7x+EaDASo80FEXwlwiA+j/PPIcX
 github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
 github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684 h1:YMyvXRstOQc7n6eneHfudVMbARSCmZ7EZGjtTkkeB3A=
 github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684/go.mod h1:UA27Kwj3QHialP74iU6C+Gpc8Y7IOAKupeKMLLBURWM=
+github.com/cli/safeexec v1.0.0 h1:0VngyaIyqACHdcMNWfo6+KdUYnqEr2Sg+bSP1pdF+dI=
+github.com/cli/safeexec v1.0.0/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q=
 github.com/cli/shurcooL-graphql v0.0.0-20200707151639-0f7232a2bf7e h1:aq/1jlmtZoS6nlSp3yLOTZQ50G+dzHdeRNENgE/iBew=
 github.com/cli/shurcooL-graphql v0.0.0-20200707151639-0f7232a2bf7e/go.mod h1:it23pLwxmz6OyM6I5O0ATIXQS1S190Nas26L5Kahp4U=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=

internal/run/run.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 )
 
@@ -23,7 +24,13 @@ var PrepareCmd = func(cmd *exec.Cmd) Runnable {
 // SetPrepareCmd overrides PrepareCmd and returns a func to revert it back
 func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() {
 	origPrepare := PrepareCmd
-	PrepareCmd = fn
+	PrepareCmd = func(cmd *exec.Cmd) Runnable {
+		// normalize git executable name for consistency in tests
+		if baseName := filepath.Base(cmd.Args[0]); baseName == "git" || baseName == "git.exe" {
+			cmd.Args[0] = baseName
+		}
+		return fn(cmd)
+	}
 	return func() {
 		PrepareCmd = origPrepare
 	}
@@ -36,7 +43,9 @@ type cmdWithStderr struct {
 
 func (c cmdWithStderr) Output() ([]byte, error) {
 	if os.Getenv("DEBUG") != "" {
-		fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args)
+		// print commands, but omit the full path to an executable
+		debugArgs := append([]string{filepath.Base(c.Cmd.Args[0])}, c.Cmd.Args[1:]...)
+		fmt.Fprintf(os.Stderr, "%v\n", debugArgs)
 	}
 	if c.Cmd.Stderr != nil {
 		return c.Cmd.Output()

pkg/browser/browser.go

@@ -6,6 +6,7 @@ import (
 	"runtime"
 	"strings"
 
+	"github.com/cli/safeexec"
 	"github.com/google/shlex"
 )
 
@@ -31,7 +32,7 @@ func ForOS(goos, url string) *exec.Cmd {
 	case "darwin":
 		args = append(args, url)
 	case "windows":
-		exe = "cmd"
+		exe, _ = lookPath("cmd")
 		r := strings.NewReplacer("&", "^&")
 		args = append(args, "/c", "start", r.Replace(url))
 	default:
@@ -51,8 +52,13 @@ func FromLauncher(launcher, url string) (*exec.Cmd, error) {
 		return nil, err
 	}
 
+	exe, err := lookPath(args[0])
+	if err != nil {
+		return nil, err
+	}
+
 	args = append(args, url)
-	cmd := exec.Command(args[0], args[1:]...)
+	cmd := exec.Command(exe, args[1:]...)
 	cmd.Stderr = os.Stderr
 	return cmd, nil
 }
@@ -71,4 +77,4 @@ func linuxExe() string {
 	return exe
 }
 
-var lookPath = exec.LookPath
+var lookPath = safeexec.LookPath

pkg/browser/browser_test.go

@@ -49,17 +49,22 @@ func TestForOS(t *testing.T) {
 				goos: "windows",
 				url:  "https://example.com/path?a=1&b=2&c=3",
 			},
+			exe:  "cmd",
 			want: []string{"cmd", "/c", "start", "https://example.com/path?a=1^&b=2^&c=3"},
 		},
 	}
 	for _, tt := range tests {
+		origLookPath := lookPath
 		lookPath = func(file string) (string, error) {
 			if file == tt.exe {
 				return file, nil
 			} else {
 				return "", errors.New("not found")
 			}
 		}
+		defer func() {
+			lookPath = origLookPath
+		}()
 
 		t.Run(tt.name, func(t *testing.T) {
 			if cmd := ForOS(tt.args.goos, tt.args.url); !reflect.DeepEqual(cmd.Args, tt.want) {