STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
Ensure that only PATH is searched when shelling out to external commands
This commit is contained in:
Mislav Marohnić 2020-11-11 16:36:50 +01:00 • committed by GitHub
commit fc3f517419
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 177 additions and 47 deletions
Download patch file Download diff file Expand all files Collapse all files

9
cmd/gh/main.go
View file

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

97
git/git.go
View file

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

18
git/remote.go
View file

@ -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()
}

1
go.mod
View file

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

2
go.sum
View file

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

13
internal/run/run.go
View file

@ -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()

12
pkg/browser/browser.go
View file

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

5
pkg/browser/browser_test.go
View file

@ -49,10 +49,12 @@ 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
@ -60,6 +62,9 @@ func TestForOS(t *testing.T) {
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) {

6
pkg/cmd/alias/expand/expand.go
View file

@ -4,13 +4,13 @@ import (
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"regexp"
"runtime"
"strings"
"github.com/cli/cli/internal/config"
"github.com/cli/safeexec"
"github.com/google/shlex"
)
@ -80,7 +80,7 @@ func ExpandAlias(cfg config.Config, args []string, findShFunc func() (string, er
}
func findSh() (string, error) {
shPath, err := exec.LookPath("sh")
shPath, err := safeexec.LookPath("sh")
if err == nil {
return shPath, nil
}
@ -88,7 +88,7 @@ func findSh() (string, error) {
if runtime.GOOS == "windows" {
winNotFoundErr := errors.New("unable to locate sh to execute the shell alias with. The sh.exe interpreter is typically distributed with Git for Windows.")
// We can try and find a sh executable in a Git for Windows install
gitPath, err := exec.LookPath("git")
gitPath, err := safeexec.LookPath("git")
if err != nil {
return "", winNotFoundErr
}

8
pkg/cmd/pr/checkout/checkout.go
View file

@ -16,6 +16,7 @@ import (
"github.com/cli/cli/pkg/cmd/pr/shared"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/cli/safeexec"
"github.com/spf13/cobra"
)
@ -165,7 +166,12 @@ func checkoutRun(opts *CheckoutOptions) error {
}
for _, args := range cmdQueue {
cmd := exec.Command(args[0], args[1:]...)
// TODO: reuse the result of this lookup across loop iteration
exe, err := safeexec.LookPath(args[0])
if err != nil {
return err
}
cmd := exec.Command(exe, args[1:]...)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := run.PrepareCmd(cmd).Run(); err != nil {

17
pkg/cmd/release/create/create.go
View file

@ -6,11 +6,11 @@ import (
"io/ioutil"
"net/http"
"os"
"os/exec"
"strings"
"github.com/AlecAivazis/survey/v2"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/git"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/internal/ghrepo"
"github.com/cli/cli/internal/run"
@ -307,13 +307,19 @@ func createRun(opts *CreateOptions) error {
}
func gitTagInfo(tagName string) (string, error) {
cmd := exec.Command("git", "tag", "--list", tagName, "--format=%(contents:subject)%0a%0a%(contents:body)")
cmd, err := git.GitCommand("tag", "--list", tagName, "--format=%(contents:subject)%0a%0a%(contents:body)")
if err != nil {
return "", err
}
b, err := run.PrepareCmd(cmd).Output()
return string(b), err
}
func detectPreviousTag(headRef string) (string, error) {
cmd := exec.Command("git", "describe", "--tags", "--abbrev=0", fmt.Sprintf("%s^", headRef))
cmd, err := git.GitCommand("describe", "--tags", "--abbrev=0", fmt.Sprintf("%s^", headRef))
if err != nil {
return "", err
}
b, err := run.PrepareCmd(cmd).Output()
return strings.TrimSpace(string(b)), err
}
@ -324,7 +330,10 @@ type logEntry struct {
}
func changelogForRange(refRange string) ([]logEntry, error) {
cmd := exec.Command("git", "-c", "log.ShowSignature=false", "log", "--first-parent", "--reverse", "--pretty=format:%B%x00", refRange)
cmd, err := git.GitCommand("-c", "log.ShowSignature=false", "log", "--first-parent", "--reverse", "--pretty=format:%B%x00", refRange)
if err != nil {
return nil, err
}
b, err := run.PrepareCmd(cmd).Output()
if err != nil {
return nil, err

10
pkg/cmd/repo/create/create.go
View file

@ -284,14 +284,20 @@ func createRun(opts *CreateOptions) error {
if doSetup {
path := repo.Name
gitInit := git.GitCommand("init", path)
gitInit, err := git.GitCommand("init", path)
if err != nil {
return err
}
gitInit.Stdout = stdout
gitInit.Stderr = stderr
err = run.PrepareCmd(gitInit).Run()
if err != nil {
return err
}
gitRemoteAdd := git.GitCommand("-C", path, "remote", "add", "origin", remoteURL)
gitRemoteAdd, err := git.GitCommand("-C", path, "remote", "add", "origin", remoteURL)
if err != nil {
return err
}
gitRemoteAdd.Stdout = stdout
gitRemoteAdd.Stderr = stderr
err = run.PrepareCmd(gitRemoteAdd).Run()

5
pkg/cmd/repo/fork/fork.go
View file

@ -218,7 +218,10 @@ func forkRun(opts *ForkOptions) error {
}
if _, err := remotes.FindByName(remoteName); err == nil {
renameTarget := "upstream"
renameCmd := git.GitCommand("remote", "rename", remoteName, renameTarget)
renameCmd, err := git.GitCommand("remote", "rename", remoteName, renameTarget)
if err != nil {
return err
}
err = run.PrepareCmd(renameCmd).Run()
if err != nil {
return err

13
pkg/iostreams/iostreams.go
View file

@ -12,6 +12,7 @@ import (
"time"
"github.com/briandowns/spinner"
"github.com/cli/safeexec"
"github.com/google/shlex"
"github.com/mattn/go-colorable"
"github.com/mattn/go-isatty"
@ -160,7 +161,11 @@ func (s *IOStreams) StartPager() error {
pagerEnv = append(pagerEnv, "LV=-c")
}
pagerCmd := exec.Command(pagerArgs[0], pagerArgs[1:]...)
pagerExe, err := safeexec.LookPath(pagerArgs[0])
if err != nil {
return err
}
pagerCmd := exec.Command(pagerExe, pagerArgs[1:]...)
pagerCmd.Env = pagerEnv
pagerCmd.Stdout = s.Out
pagerCmd.Stderr = s.ErrOut
@ -228,7 +233,11 @@ func (s *IOStreams) TerminalWidth() int {
}
if isCygwinTerminal(out) {
tputCmd := exec.Command("tput", "cols")
tputExe, err := safeexec.LookPath("tput")
if err != nil {
return defaultWidth
}
tputCmd := exec.Command(tputExe, "cols")
tputCmd.Stdin = os.Stdin
if out, err := tputCmd.Output(); err == nil {
if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil {

8
pkg/surveyext/editor_manual.go
View file

@ -7,6 +7,7 @@ import (
"os"
"os/exec"
"github.com/cli/safeexec"
shellquote "github.com/kballard/go-shellquote"
)
@ -55,7 +56,12 @@ func Edit(editorCommand, fn, initialValue string, stdin io.Reader, stdout io.Wri
}
args = append(args, f.Name())
cmd := exec.Command(args[0], args[1:]...)
editorExe, err := safeexec.LookPath(args[0])
if err != nil {
return "", err
}
cmd := exec.Command(editorExe, args[1:]...)
cmd.Stdin = stdin
cmd.Stdout = stdout
cmd.Stderr = stderr