X Tutup
Skip to content

Commit fc3f517

Browse files
mislavmislav
authored
Merge pull request from GHSA-fqfh-778m-2v32
Ensure that only PATH is searched when shelling out to external commands
2 parents 3a1e002 + 38f68d8 commit fc3f517

File tree

15 files changed

+177
-47
lines changed

15 files changed

+177
-47
lines changed

‎cmd/gh/main.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/cli/cli/pkg/cmdutil"
2323
"github.com/cli/cli/update"
2424
"github.com/cli/cli/utils"
25+
"github.com/cli/safeexec"
2526
"github.com/mattn/go-colorable"
2627
"github.com/mgutz/ansi"
2728
"github.com/spf13/cobra"
@@ -107,7 +108,13 @@ func main() {
107108
}
108109

109110
if isShell {
110-
externalCmd := exec.Command(expandedArgs[0], expandedArgs[1:]...)
111+
exe, err := safeexec.LookPath(expandedArgs[0])
112+
if err != nil {
113+
fmt.Fprintf(stderr, "failed to run external command: %s", err)
114+
os.Exit(3)
115+
}
116+
117+
externalCmd := exec.Command(exe, expandedArgs[1:]...)
111118
externalCmd.Stderr = os.Stderr
112119
externalCmd.Stdout = os.Stdout
113120
externalCmd.Stdin = os.Stdin

‎git/git.go‎

Lines changed: 75 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@ import (
1010
"os/exec"
1111
"path"
1212
"regexp"
13+
"runtime"
1314
"strings"
1415

1516
"github.com/cli/cli/internal/run"
17+
"github.com/cli/safeexec"
1618
)
1719

1820
// ErrNotOnAnyBranch indicates that the user is in detached HEAD state
@@ -37,7 +39,10 @@ func (r TrackingRef) String() string {
3739
// ShowRefs resolves fully-qualified refs to commit hashes
3840
func ShowRefs(ref ...string) ([]Ref, error) {
3941
args := append([]string{"show-ref", "--verify", "--"}, ref...)
40-
showRef := exec.Command("git", args...)
42+
showRef, err := GitCommand(args...)
43+
if err != nil {
44+
return nil, err
45+
}
4146
output, err := run.PrepareCmd(showRef).Output()
4247

4348
var refs []Ref
@@ -57,7 +62,10 @@ func ShowRefs(ref ...string) ([]Ref, error) {
5762

5863
// CurrentBranch reads the checked-out branch for the git repository
5964
func CurrentBranch() (string, error) {
60-
refCmd := GitCommand("symbolic-ref", "--quiet", "HEAD")
65+
refCmd, err := GitCommand("symbolic-ref", "--quiet", "HEAD")
66+
if err != nil {
67+
return "", err
68+
}
6169

6270
output, err := run.PrepareCmd(refCmd).Output()
6371
if err == nil {
@@ -78,13 +86,19 @@ func CurrentBranch() (string, error) {
7886
}
7987

8088
func listRemotes() ([]string, error) {
81-
remoteCmd := exec.Command("git", "remote", "-v")
89+
remoteCmd, err := GitCommand("remote", "-v")
90+
if err != nil {
91+
return nil, err
92+
}
8293
output, err := run.PrepareCmd(remoteCmd).Output()
8394
return outputLines(output), err
8495
}
8596

8697
func Config(name string) (string, error) {
87-
configCmd := exec.Command("git", "config", name)
98+
configCmd, err := GitCommand("config", name)
99+
if err != nil {
100+
return "", err
101+
}
88102
output, err := run.PrepareCmd(configCmd).Output()
89103
if err != nil {
90104
return "", fmt.Errorf("unknown config key: %s", name)
@@ -94,12 +108,23 @@ func Config(name string) (string, error) {
94108

95109
}
96110

97-
var GitCommand = func(args ...string) *exec.Cmd {
98-
return exec.Command("git", args...)
111+
var GitCommand = func(args ...string) (*exec.Cmd, error) {
112+
gitExe, err := safeexec.LookPath("git")
113+
if err != nil {
114+
programName := "git"
115+
if runtime.GOOS == "windows" {
116+
programName = "Git for Windows"
117+
}
118+
return nil, fmt.Errorf("unable to find git executable in PATH; please install %s before retrying", programName)
119+
}
120+
return exec.Command(gitExe, args...), nil
99121
}
100122

101123
func UncommittedChangeCount() (int, error) {
102-
statusCmd := GitCommand("status", "--porcelain")
124+
statusCmd, err := GitCommand("status", "--porcelain")
125+
if err != nil {
126+
return 0, err
127+
}
103128
output, err := run.PrepareCmd(statusCmd).Output()
104129
if err != nil {
105130
return 0, err
@@ -123,10 +148,13 @@ type Commit struct {
123148
}
124149

125150
func Commits(baseRef, headRef string) ([]*Commit, error) {
126-
logCmd := GitCommand(
151+
logCmd, err := GitCommand(
127152
"-c", "log.ShowSignature=false",
128153
"log", "--pretty=format:%H,%s",
129154
"--cherry", fmt.Sprintf("%s...%s", baseRef, headRef))
155+
if err != nil {
156+
return nil, err
157+
}
130158
output, err := run.PrepareCmd(logCmd).Output()
131159
if err != nil {
132160
return []*Commit{}, err
@@ -154,7 +182,10 @@ func Commits(baseRef, headRef string) ([]*Commit, error) {
154182
}
155183

156184
func CommitBody(sha string) (string, error) {
157-
showCmd := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
185+
showCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
186+
if err != nil {
187+
return "", err
188+
}
158189
output, err := run.PrepareCmd(showCmd).Output()
159190
if err != nil {
160191
return "", err
@@ -164,7 +195,10 @@ func CommitBody(sha string) (string, error) {
164195

165196
// Push publishes a git ref to a remote and sets up upstream configuration
166197
func Push(remote string, ref string, cmdOut, cmdErr io.Writer) error {
167-
pushCmd := GitCommand("push", "--set-upstream", remote, ref)
198+
pushCmd, err := GitCommand("push", "--set-upstream", remote, ref)
199+
if err != nil {
200+
return err
201+
}
168202
pushCmd.Stdout = cmdOut
169203
pushCmd.Stderr = cmdErr
170204
return run.PrepareCmd(pushCmd).Run()
@@ -179,7 +213,10 @@ type BranchConfig struct {
179213
// ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` part of git config
180214
func ReadBranchConfig(branch string) (cfg BranchConfig) {
181215
prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch))
182-
configCmd := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix))
216+
configCmd, err := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix))
217+
if err != nil {
218+
return
219+
}
183220
output, err := run.PrepareCmd(configCmd).Output()
184221
if err != nil {
185222
return
@@ -209,21 +246,28 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) {
209246
}
210247

211248
func DeleteLocalBranch(branch string) error {
212-
branchCmd := GitCommand("branch", "-D", branch)
213-
err := run.PrepareCmd(branchCmd).Run()
214-
return err
249+
branchCmd, err := GitCommand("branch", "-D", branch)
250+
if err != nil {
251+
return err
252+
}
253+
return run.PrepareCmd(branchCmd).Run()
215254
}
216255

217256
func HasLocalBranch(branch string) bool {
218-
configCmd := GitCommand("rev-parse", "--verify", "refs/heads/"+branch)
219-
_, err := run.PrepareCmd(configCmd).Output()
257+
configCmd, err := GitCommand("rev-parse", "--verify", "refs/heads/"+branch)
258+
if err != nil {
259+
return false
260+
}
261+
_, err = run.PrepareCmd(configCmd).Output()
220262
return err == nil
221263
}
222264

223265
func CheckoutBranch(branch string) error {
224-
configCmd := GitCommand("checkout", branch)
225-
err := run.PrepareCmd(configCmd).Run()
226-
return err
266+
configCmd, err := GitCommand("checkout", branch)
267+
if err != nil {
268+
return err
269+
}
270+
return run.PrepareCmd(configCmd).Run()
227271
}
228272

229273
func parseCloneArgs(extraArgs []string) (args []string, target string) {
@@ -252,7 +296,10 @@ func RunClone(cloneURL string, args []string) (target string, err error) {
252296

253297
cloneArgs = append([]string{"clone"}, cloneArgs...)
254298

255-
cloneCmd := GitCommand(cloneArgs...)
299+
cloneCmd, err := GitCommand(cloneArgs...)
300+
if err != nil {
301+
return "", err
302+
}
256303
cloneCmd.Stdin = os.Stdin
257304
cloneCmd.Stdout = os.Stdout
258305
cloneCmd.Stderr = os.Stderr
@@ -262,7 +309,10 @@ func RunClone(cloneURL string, args []string) (target string, err error) {
262309
}
263310

264311
func AddUpstreamRemote(upstreamURL, cloneDir string) error {
265-
cloneCmd := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL)
312+
cloneCmd, err := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL)
313+
if err != nil {
314+
return err
315+
}
266316
cloneCmd.Stdout = os.Stdout
267317
cloneCmd.Stderr = os.Stderr
268318
return run.PrepareCmd(cloneCmd).Run()
@@ -274,7 +324,10 @@ func isFilesystemPath(p string) bool {
274324

275325
// ToplevelDir returns the top-level directory path of the current repository
276326
func ToplevelDir() (string, error) {
277-
showCmd := exec.Command("git", "rev-parse", "--show-toplevel")
327+
showCmd, err := GitCommand("rev-parse", "--show-toplevel")
328+
if err != nil {
329+
return "", err
330+
}
278331
output, err := run.PrepareCmd(showCmd).Output()
279332
return firstLine(output), err
280333

‎git/remote.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package git
33
import (
44
"fmt"
55
"net/url"
6-
"os/exec"
76
"regexp"
87
"strings"
98

@@ -45,7 +44,10 @@ func Remotes() (RemoteSet, error) {
4544
remotes := parseRemotes(list)
4645

4746
// this is affected by SetRemoteResolution
48-
remoteCmd := exec.Command("git", "config", "--get-regexp", `^remote\..*\.gh-resolved$`)
47+
remoteCmd, err := GitCommand("config", "--get-regexp", `^remote\..*\.gh-resolved$`)
48+
if err != nil {
49+
return nil, err
50+
}
4951
output, _ := run.PrepareCmd(remoteCmd).Output()
5052
for _, l := range outputLines(output) {
5153
parts := strings.SplitN(l, " ", 2)
@@ -107,8 +109,11 @@ func parseRemotes(gitRemotes []string) (remotes RemoteSet) {
107109

108110
// AddRemote adds a new git remote and auto-fetches objects from it
109111
func AddRemote(name, u string) (*Remote, error) {
110-
addCmd := exec.Command("git", "remote", "add", "-f", name, u)
111-
err := run.PrepareCmd(addCmd).Run()
112+
addCmd, err := GitCommand("remote", "add", "-f", name, u)
113+
if err != nil {
114+
return nil, err
115+
}
116+
err = run.PrepareCmd(addCmd).Run()
112117
if err != nil {
113118
return nil, err
114119
}
@@ -136,6 +141,9 @@ func AddRemote(name, u string) (*Remote, error) {
136141
}
137142

138143
func SetRemoteResolution(name, resolution string) error {
139-
addCmd := exec.Command("git", "config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
144+
addCmd, err := GitCommand("config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
145+
if err != nil {
146+
return err
147+
}
140148
return run.PrepareCmd(addCmd).Run()
141149
}

‎go.mod‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ require (
77
github.com/MakeNowJust/heredoc v1.0.0
88
github.com/briandowns/spinner v1.11.1
99
github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684
10+
github.com/cli/safeexec v1.0.0
1011
github.com/cpuguy83/go-md2man/v2 v2.0.0
1112
github.com/enescakir/emoji v1.0.0
1213
github.com/google/go-cmp v0.5.2

‎go.sum‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ github.com/briandowns/spinner v1.11.1/go.mod h1:QOuQk7x+EaDASo80FEXwlwiA+j/PPIcX
4444
github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
4545
github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684 h1:YMyvXRstOQc7n6eneHfudVMbARSCmZ7EZGjtTkkeB3A=
4646
github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684/go.mod h1:UA27Kwj3QHialP74iU6C+Gpc8Y7IOAKupeKMLLBURWM=
47+
github.com/cli/safeexec v1.0.0 h1:0VngyaIyqACHdcMNWfo6+KdUYnqEr2Sg+bSP1pdF+dI=
48+
github.com/cli/safeexec v1.0.0/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q=
4749
github.com/cli/shurcooL-graphql v0.0.0-20200707151639-0f7232a2bf7e h1:aq/1jlmtZoS6nlSp3yLOTZQ50G+dzHdeRNENgE/iBew=
4850
github.com/cli/shurcooL-graphql v0.0.0-20200707151639-0f7232a2bf7e/go.mod h1:it23pLwxmz6OyM6I5O0ATIXQS1S190Nas26L5Kahp4U=
4951
github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=

‎internal/run/run.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"os"
77
"os/exec"
8+
"path/filepath"
89
"strings"
910
)
1011

@@ -23,7 +24,13 @@ var PrepareCmd = func(cmd *exec.Cmd) Runnable {
2324
// SetPrepareCmd overrides PrepareCmd and returns a func to revert it back
2425
func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() {
2526
origPrepare := PrepareCmd
26-
PrepareCmd = fn
27+
PrepareCmd = func(cmd *exec.Cmd) Runnable {
28+
// normalize git executable name for consistency in tests
29+
if baseName := filepath.Base(cmd.Args[0]); baseName == "git" || baseName == "git.exe" {
30+
cmd.Args[0] = baseName
31+
}
32+
return fn(cmd)
33+
}
2734
return func() {
2835
PrepareCmd = origPrepare
2936
}
@@ -36,7 +43,9 @@ type cmdWithStderr struct {
3643

3744
func (c cmdWithStderr) Output() ([]byte, error) {
3845
if os.Getenv("DEBUG") != "" {
39-
fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args)
46+
// print commands, but omit the full path to an executable
47+
debugArgs := append([]string{filepath.Base(c.Cmd.Args[0])}, c.Cmd.Args[1:]...)
48+
fmt.Fprintf(os.Stderr, "%v\n", debugArgs)
4049
}
4150
if c.Cmd.Stderr != nil {
4251
return c.Cmd.Output()

‎pkg/browser/browser.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"runtime"
77
"strings"
88

9+
"github.com/cli/safeexec"
910
"github.com/google/shlex"
1011
)
1112

@@ -31,7 +32,7 @@ func ForOS(goos, url string) *exec.Cmd {
3132
case "darwin":
3233
args = append(args, url)
3334
case "windows":
34-
exe = "cmd"
35+
exe, _ = lookPath("cmd")
3536
r := strings.NewReplacer("&", "^&")
3637
args = append(args, "/c", "start", r.Replace(url))
3738
default:
@@ -51,8 +52,13 @@ func FromLauncher(launcher, url string) (*exec.Cmd, error) {
5152
return nil, err
5253
}
5354

55+
exe, err := lookPath(args[0])
56+
if err != nil {
57+
return nil, err
58+
}
59+
5460
args = append(args, url)
55-
cmd := exec.Command(args[0], args[1:]...)
61+
cmd := exec.Command(exe, args[1:]...)
5662
cmd.Stderr = os.Stderr
5763
return cmd, nil
5864
}
@@ -71,4 +77,4 @@ func linuxExe() string {
7177
return exe
7278
}
7379

74-
var lookPath = exec.LookPath
80+
var lookPath = safeexec.LookPath

‎pkg/browser/browser_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,22 @@ func TestForOS(t *testing.T) {
4949
goos: "windows",
5050
url: "https://example.com/path?a=1&b=2&c=3",
5151
},
52+
exe: "cmd",
5253
want: []string{"cmd", "/c", "start", "https://example.com/path?a=1^&b=2^&c=3"},
5354
},
5455
}
5556
for _, tt := range tests {
57+
origLookPath := lookPath
5658
lookPath = func(file string) (string, error) {
5759
if file == tt.exe {
5860
return file, nil
5961
} else {
6062
return "", errors.New("not found")
6163
}
6264
}
65+
defer func() {
66+
lookPath = origLookPath
67+
}()
6368

6469
t.Run(tt.name, func(t *testing.T) {
6570
if cmd := ForOS(tt.args.goos, tt.args.url); !reflect.DeepEqual(cmd.Args, tt.want) {

0 commit comments

Comments
 (0)
X Tutup