From 762ccd4a498f5c17723b91d56b9304434ada5540 Mon Sep 17 00:00:00 2001 From: Chris Darroch Date: Mon, 28 Mar 2022 23:16:16 -0700 Subject: [PATCH] subprocess: report errors when finding executables On Windows, when spawning a process, Go first looks for an executable file with the correct name in the current directory, and only if it fails to find one there does it look in the directories listed in the PATH environment variable. We would prefer not to replicate this behaviour and instead search only in the directories in PATH. Therefore, starting with the mitigation of CVE-2020-27955 in commit 74d5f2397f9abe4834bf1fe1fa02fd6c141b77ce, we resolve paths to executables ourselves rather than rely on Go to do so. The subprocess.LookPath() function we introduced in that change is adapted from Go's os/exec package. When it cannot find an executable in PATH, it returns an empty path string and an exec.ErrNotFound error. When that happens, we do not detect the condition and simply set the command path to the empty string. This can lead to undesirable behaviour in which a bug in the Go os/exec library causes it to run another executable other than the one we intended. When we set the command path to the empty string and then ask to execute the command, the native Go version of the LookPath() function for Windows is run with an empty path because filepath.Base() returns "." when passed the empty string and because we have left the current working directory also set to an empty string: https://github.com/golang/go/blob/1724077b789ad92972ab1ac03788389645306cbb/src/os/exec/exec.go#L348-L353 Since the path string does not contain any path separator characters the current working directory is searched to try to find a file with a matching name and executable file extension (e.g., ".exe" or ".com"). To search the current working directory, the "." directory name is prepended to the given path with filepath.Join(): https://github.com/golang/go/blob/1724077b789ad92972ab1ac03788389645306cbb/src/os/exec/lp_windows.go#L84 The filepath.Join() function ignores empty arguments, so this results in an incorrect filename of ".". All potential executable file extensions from the PATHEXT environment variable (or from a fixed list if that variable is not defined) are then appended to this filename, including their leading dot separator characters, thus producing filenames like "..com" and "..exe": https://github.com/golang/go/blob/1724077b789ad92972ab1ac03788389645306cbb/src/os/exec/lp_windows.go#L46-L50 Should a file with one of these names exist in the current working directory, its name will be returned, which means that it will be executed instead of the command we expected to run. This is obviously undesirable and presents a risk to users. (Note that batch files named "..bat" and "..cmd" will also be found in the same manner, but they will not actually be executed due to an undocumented feature of the Windows API's family of CreateProcess*() functions, which are used by Go to spawn processes. When passed an lpApplicationName argument ending with a ".bat" or ".cmd" extension, the CreateProcess*() functions appear to instead execute "cmd.exe" and construct a "/c" command string from the lpCommandLine argument. The value of that argument is set using the full command line we are attempting to execute, and as such its first element is the actual name of the executable we intended to run; therefore, the command interpreter attempts to run the executable as a batch script and fails, and the "..bat" or "..cmd" file is effectively ignored.) To recap, when Git LFS attempts to execute a program on Windows, if the executable is not found anywhere in PATH but does exist in the current working directory, then when we call exec.Command() Go's internal LookPath() function will find the executable and not set the internal lookPathErr flag: https://github.com/golang/go/blob/1724077b789ad92972ab1ac03788389645306cbb/src/os/exec/exec.go#L174-L179 Since we do not want to run executables in the current working directory, we subsequently run our own LookPath() function, which we use to reset the cmd.Path field. However, when our LookPath() returns an empty string, we do not detect that and instead set cmd.Path to that value. Then when we ask Go to run the command, because lookPathErr is nil, it proceeds, and the empty path causes it to find and run the first matching file in the working directory named "..com" or "..exe" (or any similar name with an executable file extension except for "..bat" and "..cmd"). We can prevent this behaviour by detecting when our LookPath() function returns an error and propagating it upwards to all callers of our subprocess.ExecCommand() function. We also add checks for this error at appropriate points and log or report the error as necessary. One particular circumstance of note occurs when a user has a Cygwin-installed "uname" in their PATH but not "cygpath", but "cygpath.exe" does exist in their current working directory. Then we will attempt to execute "cygpath" because we use the presence of "uname" as an indication that Cygwin is fully installed. Should a "..exe" or similar file also be present in the working directory, then it will be executed instead of "cygpath.exe". As with all other instances where we call subprocess.ExecCommand(), in tools.translateCygwinPath() we will now check for a returned error before trying to actually execute "cygpath". Unlike many of the other cases, though, we do not need to report the error in this one; instead, we simply return the current path from translateCygwinPath() instead of canonicalizing it, just as we do already if the "cygpath" executable fails for some reason. Finally, we add a new test to t/t-path.sh which checks for the incorrect execution of a "..exe" binary on Windows when "git.exe" is not found in PATH but does exist in the current working directory. This test passes when run with a Git LFS binary that includes the remediations from this commit, and fails otherwise. For our "malicious" binary named "..exe" we make use of the lfstest-badpathcheck test helper we added in a previous commit. We only run this test on Windows because the underlying bug in Go is Windows-specific as it depends on path extensions from PATHEXT being appended to the file name ".". Co-authored-by: brian m. carlson --- commands/command_clone.go | 5 +- commands/command_ls_files.go | 7 +- commands/command_status.go | 6 +- commands/path_windows.go | 5 +- commands/pull.go | 5 +- creds/creds.go | 13 +++- git/config.go | 5 +- git/git.go | 109 ++++++++++++++++++++++++------- git/ls_files.go | 5 +- git/rev_list_scanner.go | 5 +- lfs/extension.go | 6 +- lfshttp/certs_darwin.go | 12 +++- lfshttp/ssh.go | 7 +- lfshttp/standalone/standalone.go | 5 +- ssh/connection.go | 5 +- subprocess/subprocess.go | 11 +++- subprocess/subprocess_nix.go | 10 ++- subprocess/subprocess_windows.go | 10 ++- t/t-credentials-no-prompt.sh | 3 +- t/t-path.sh | 34 ++++++++++ tools/cygwin_windows.go | 5 +- tools/os_tools.go | 7 +- tq/custom.go | 5 +- 23 files changed, 229 insertions(+), 56 deletions(-) diff --git a/commands/command_clone.go b/commands/command_clone.go index 4d93547f..bdaf3562 100644 --- a/commands/command_clone.go +++ b/commands/command_clone.go @@ -118,8 +118,11 @@ func postCloneSubmodules(args []string) error { // Use `git submodule foreach --recursive` to cascade into nested submodules // Also good to call a new instance of git-lfs rather than do things // inside this instance, since that way we get a clean env in that subrepo - cmd := subprocess.ExecCommand("git", "submodule", "foreach", "--recursive", + cmd, err := subprocess.ExecCommand("git", "submodule", "foreach", "--recursive", "git lfs pull") + if err != nil { + return err + } cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout diff --git a/commands/command_ls_files.go b/commands/command_ls_files.go index a468fcc2..0cccd9be 100644 --- a/commands/command_ls_files.go +++ b/commands/command_ls_files.go @@ -4,6 +4,7 @@ import ( "os" "strings" + "github.com/git-lfs/git-lfs/v3/errors" "github.com/git-lfs/git-lfs/v3/git" "github.com/git-lfs/git-lfs/v3/lfs" "github.com/git-lfs/git-lfs/v3/tools/humanize" @@ -50,7 +51,11 @@ func lsFilesCommand(cmd *cobra.Command, args []string) { } else { fullref, err := git.CurrentRef() if err != nil { - ref = git.EmptyTree() + ref, err = git.EmptyTree() + if err != nil { + ExitWithError(errors.Wrap( + err, tr.Tr.Get("Could not read empty Git tree object"))) + } } else { ref = fullref.Sha } diff --git a/commands/command_status.go b/commands/command_status.go index faf31bce..e965823c 100644 --- a/commands/command_status.go +++ b/commands/command_status.go @@ -28,8 +28,12 @@ func statusCommand(cmd *cobra.Command, args []string) { ref, _ := git.CurrentRef() scanIndexAt := "HEAD" + var err error if ref == nil { - scanIndexAt = git.EmptyTree() + scanIndexAt, err = git.EmptyTree() + if err != nil { + ExitWithError(err) + } } scanner, err := lfs.NewPointerScanner(cfg.GitEnv(), cfg.OSEnv()) diff --git a/commands/path_windows.go b/commands/path_windows.go index 2e700221..0b2dd445 100644 --- a/commands/path_windows.go +++ b/commands/path_windows.go @@ -48,7 +48,10 @@ func cleanRootPath(pattern string) string { if len(winBashPrefix) < 1 { // cmd.Path is something like C:\Program Files\Git\usr\bin\pwd.exe - cmd := subprocess.ExecCommand("pwd") + cmd, err := subprocess.ExecCommand("pwd") + if err != nil { + return pattern + } winBashPrefix = strings.Replace(filepath.Dir(filepath.Dir(filepath.Dir(cmd.Path))), `\`, "/", -1) + "/" } diff --git a/commands/pull.go b/commands/pull.go index f68d107c..644e5203 100644 --- a/commands/pull.go +++ b/commands/pull.go @@ -147,7 +147,10 @@ func (i *gitIndexer) Add(path string) error { if i.cmd == nil { // Fire up the update-index command - cmd := git.UpdateIndexFromStdin() + cmd, err := git.UpdateIndexFromStdin() + if err != nil { + return err + } cmd.Stdout = &i.output cmd.Stderr = &i.output stdin, err := cmd.StdinPipe() diff --git a/creds/creds.go b/creds/creds.go index 2596dd62..a4dd328f 100644 --- a/creds/creds.go +++ b/creds/creds.go @@ -241,7 +241,11 @@ func (a *AskPassCredentialHelper) getFromProgram(valueType credValueType, u *url // 'cmd' will run the GIT_ASKPASS (or core.askpass) command prompting // for the desired valueType (`Username` or `Password`) - cmd := subprocess.ExecCommand(a.Program, a.args(fmt.Sprintf("%s for %q", valueString, u))...) + cmd, errVal := subprocess.ExecCommand(a.Program, a.args(fmt.Sprintf("%s for %q", valueString, u))...) + if errVal != nil { + tracerx.Printf("creds: failed to find GIT_ASKPASS command: %s", a.Program) + return "", errVal + } cmd.Stderr = &err cmd.Stdout = &value @@ -301,7 +305,10 @@ func (h *commandCredentialHelper) Approve(creds Creds) error { func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, error) { output := new(bytes.Buffer) - cmd := subprocess.ExecCommand("git", "credential", subcommand) + cmd, err := subprocess.ExecCommand("git", "credential", subcommand) + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git credential %s`: %v", subcommand, err)) + } cmd.Stdin = bufferCreds(input) cmd.Stdout = output /* @@ -316,7 +323,7 @@ func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, e */ cmd.Stderr = os.Stderr - err := cmd.Start() + err = cmd.Start() if err == nil { err = cmd.Wait() } diff --git a/git/config.go b/git/config.go index 35be2d2b..fb56f43a 100644 --- a/git/config.go +++ b/git/config.go @@ -199,7 +199,10 @@ func (c *Configuration) Source() (*ConfigurationSource, error) { func (c *Configuration) gitConfig(args ...string) (string, error) { args = append([]string{"config", "--includes"}, args...) - cmd := subprocess.ExecCommand("git", args...) + cmd, err := subprocess.ExecCommand("git", args...) + if err != nil { + return "", err + } if len(c.GitDir) > 0 { cmd.Dir = c.GitDir } diff --git a/git/git.go b/git/git.go index d53ec12a..8759585f 100644 --- a/git/git.go +++ b/git/git.go @@ -148,17 +148,20 @@ func IsZeroObjectID(s string) bool { return false } -func EmptyTree() string { +func EmptyTree() (string, error) { emptyTreeMutex.Lock() defer emptyTreeMutex.Unlock() if len(emptyTree) == 0 { - cmd := gitNoLFS("hash-object", "-t", "tree", "/dev/null") + cmd, err := gitNoLFS("hash-object", "-t", "tree", "/dev/null") + if err != nil { + return "", errors.New(tr.Tr.Get("failed to find `git hash-object`: %v", err)) + } cmd.Stdin = nil out, _ := cmd.Output() emptyTree = strings.TrimSpace(string(out)) } - return emptyTree + return emptyTree, nil } // Some top level information about a commit (only first line of message) @@ -198,7 +201,7 @@ func gitConfigNoLFS(args ...string) []string { } // Invoke Git with disabled LFS filters -func gitNoLFS(args ...string) *subprocess.Cmd { +func gitNoLFS(args ...string) (*subprocess.Cmd, error) { return subprocess.ExecCommand("git", gitConfigNoLFS(args...)...) } @@ -211,7 +214,7 @@ func gitNoLFSBuffered(args ...string) (*subprocess.BufferedCmd, error) { } // Invoke Git with enabled LFS filters -func git(args ...string) *subprocess.Cmd { +func git(args ...string) (*subprocess.Cmd, error) { return subprocess.ExecCommand("git", args...) } @@ -253,7 +256,10 @@ func DiffIndex(ref string, cached bool, refresh bool) (*bufio.Scanner, error) { } func HashObject(r io.Reader) (string, error) { - cmd := gitNoLFS("hash-object", "--stdin") + cmd, err := gitNoLFS("hash-object", "--stdin") + if err != nil { + return "", errors.New(tr.Tr.Get("failed to find `git hash-object`: %v", err)) + } cmd.Stdin = r out, err := cmd.Output() if err != nil { @@ -381,7 +387,10 @@ func (c *Configuration) RemoteBranchForLocalBranch(localBranch string) string { } func RemoteList() ([]string, error) { - cmd := gitNoLFS("remote") + cmd, err := gitNoLFS("remote") + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git remote`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { @@ -401,7 +410,10 @@ func RemoteList() ([]string, error) { } func RemoteURLs(push bool) (map[string][]string, error) { - cmd := gitNoLFS("remote", "-v") + cmd, err := gitNoLFS("remote", "-v") + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git remote -v`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { @@ -451,7 +463,10 @@ func MapRemoteURL(url string, push bool) (string, bool) { // Refs returns all of the local and remote branches and tags for the current // repository. Other refs (HEAD, refs/stash, git notes) are ignored. func LocalRefs() ([]*Ref, error) { - cmd := gitNoLFS("show-ref") + cmd, err := gitNoLFS("show-ref") + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git show-ref`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { @@ -500,7 +515,10 @@ func UpdateRefIn(wd string, ref *Ref, to []byte, reason string) error { args = append(args, "-m", reason) } - cmd := gitNoLFS(args...) + cmd, err := gitNoLFS(args...) + if err != nil { + return errors.New(tr.Tr.Get("failed to find `git update-ref`: %v", err)) + } cmd.Dir = wd return cmd.Run() @@ -578,7 +596,7 @@ func RewriteLocalPathAsURL(path string) string { return fmt.Sprintf("file://%s%s", slash, filepath.ToSlash(path)) } -func UpdateIndexFromStdin() *subprocess.Cmd { +func UpdateIndexFromStdin() (*subprocess.Cmd, error) { return git("update-index", "-q", "--refresh", "--stdin") } @@ -588,10 +606,13 @@ func UpdateIndexFromStdin() *subprocess.Cmd { // includeRemoteBranches: true to include refs on remote branches // onlyRemote: set to non-blank to only include remote branches on a single remote func RecentBranches(since time.Time, includeRemoteBranches bool, onlyRemote string) ([]*Ref, error) { - cmd := gitNoLFS("for-each-ref", + cmd, err := gitNoLFS("for-each-ref", `--sort=-committerdate`, `--format=%(refname) %(objectname) %(committerdate:iso)`, "refs") + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git for-each-ref`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { return nil, errors.New(tr.Tr.Get("failed to call `git for-each-ref`: %v", err)) @@ -687,8 +708,11 @@ func FormatGitDate(tm time.Time) string { // Get summary information about a commit func GetCommitSummary(commit string) (*CommitSummary, error) { - cmd := gitNoLFS("show", "-s", + cmd, err := gitNoLFS("show", "-s", `--format=%H|%h|%P|%ai|%ci|%ae|%an|%ce|%cn|%s`, commit) + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git show`: %v", err)) + } out, err := cmd.CombinedOutput() if err != nil { @@ -722,7 +746,10 @@ func GetCommitSummary(commit string) (*CommitSummary, error) { } func GitAndRootDirs() (string, string, error) { - cmd := gitNoLFS("rev-parse", "--git-dir", "--show-toplevel") + cmd, err := gitNoLFS("rev-parse", "--git-dir", "--show-toplevel") + if err != nil { + return "", "", errors.New(tr.Tr.Get("failed to find `git rev-parse --git-dir --show-toplevel`: %v", err)) + } buf := &bytes.Buffer{} cmd.Stderr = buf @@ -760,7 +787,10 @@ func GitAndRootDirs() (string, string, error) { } func RootDir() (string, error) { - cmd := gitNoLFS("rev-parse", "--show-toplevel") + cmd, err := gitNoLFS("rev-parse", "--show-toplevel") + if err != nil { + return "", errors.New(tr.Tr.Get("failed to find `git rev-parse --show-toplevel`: %v", err)) + } out, err := cmd.Output() if err != nil { return "", errors.New(tr.Tr.Get("failed to call `git rev-parse --show-toplevel`: %v %v", err, string(out))) @@ -775,7 +805,12 @@ func RootDir() (string, error) { } func GitDir() (string, error) { - cmd := gitNoLFS("rev-parse", "--git-dir") + cmd, err := gitNoLFS("rev-parse", "--git-dir") + if err != nil { + // The %w format specifier is unique to fmt.Errorf(), so we + // do not pass it to tr.Tr.Get(). + return "", fmt.Errorf("%s: %w", tr.Tr.Get("failed to find `git rev-parse --git-dir`"), err) + } buf := &bytes.Buffer{} cmd.Stderr = buf out, err := cmd.Output() @@ -797,7 +832,10 @@ func GitCommonDir() (string, error) { return GitDir() } - cmd := gitNoLFS("rev-parse", "--git-common-dir") + cmd, err := gitNoLFS("rev-parse", "--git-common-dir") + if err != nil { + return "", errors.New(tr.Tr.Get("failed to find `git rev-parse --git-common-dir`: %v", err)) + } out, err := cmd.Output() buf := &bytes.Buffer{} cmd.Stderr = buf @@ -1055,14 +1093,17 @@ func CloneWithoutFilters(flags CloneFlags, args []string) error { // Now args cmdargs = append(cmdargs, args...) - cmd := gitNoLFS(cmdargs...) + cmd, err := gitNoLFS(cmdargs...) + if err != nil { + return errors.New(tr.Tr.Get("failed to find `git clone`: %v", err)) + } // Assign all streams direct cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin - err := cmd.Start() + err = cmd.Start() if err != nil { return errors.New(tr.Tr.Get("failed to start `git clone`: %v", err)) } @@ -1102,7 +1143,10 @@ func Checkout(treeish string, paths []string, force bool) error { // currently cached locally. No remote request is made to verify them. func CachedRemoteRefs(remoteName string) ([]*Ref, error) { var ret []*Ref - cmd := gitNoLFS("show-ref") + cmd, err := gitNoLFS("show-ref") + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git show-ref`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { @@ -1157,7 +1201,10 @@ func Fetch(remotes ...string) error { // accessing the remote via git ls-remote. func RemoteRefs(remoteName string) ([]*Ref, error) { var ret []*Ref - cmd := gitNoLFS("ls-remote", "--heads", "--tags", "-q", remoteName) + cmd, err := gitNoLFS("ls-remote", "--heads", "--tags", "-q", remoteName) + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git ls-remote`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { @@ -1216,8 +1263,11 @@ func AllRefs() ([]*Ref, error) { // the given working directory "wd", or an error if those references could not // be loaded. func AllRefsIn(wd string) ([]*Ref, error) { - cmd := gitNoLFS( + cmd, err := gitNoLFS( "for-each-ref", "--format=%(objectname)%00%(refname)") + if err != nil { + return nil, lfserrors.Wrap(err, tr.Tr.Get("failed to find `git for-each-ref`: %v", err)) + } cmd.Dir = wd outp, err := cmd.StdoutPipe() @@ -1262,12 +1312,15 @@ func GetTrackedFiles(pattern string) ([]string, error) { rootWildcard := len(safePattern) < len(pattern) && strings.ContainsRune(safePattern, '*') var ret []string - cmd := gitNoLFS( + cmd, err := gitNoLFS( "-c", "core.quotepath=false", // handle special chars in filenames "ls-files", "--cached", // include things which are staged but not committed right now "--", // no ambiguous patterns safePattern) + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git ls-files`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { @@ -1321,7 +1374,10 @@ func GetFilesChanged(from, to string) ([]string, error) { } args = append(args, "--") // no ambiguous patterns - cmd := gitNoLFS(args...) + cmd, err := gitNoLFS(args...) + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find `git diff-tree`: %v", err)) + } outp, err := cmd.StdoutPipe() if err != nil { return nil, errors.New(tr.Tr.Get("failed to call `git diff-tree`: %v", err)) @@ -1352,7 +1408,10 @@ func IsFileModified(filepath string) (bool, error) { "--", // separator in case filename ambiguous filepath, } - cmd := git(args...) + cmd, err := git(args...) + if err != nil { + return false, lfserrors.Wrap(err, tr.Tr.Get("failed to find `git status`")) + } outp, err := cmd.StdoutPipe() if err != nil { return false, lfserrors.Wrap(err, tr.Tr.Get("Failed to call `git status`")) diff --git a/git/ls_files.go b/git/ls_files.go index 01087215..dac435b7 100644 --- a/git/ls_files.go +++ b/git/ls_files.go @@ -35,7 +35,10 @@ func NewLsFiles(workingDir string, standardExclude bool, untracked bool) (*LsFil if untracked { args = append(args, "--others") } - cmd := gitNoLFS(args...) + cmd, err := gitNoLFS(args...) + if err != nil { + return nil, err + } cmd.Dir = workingDir tracerx.Printf("NewLsFiles: running in %s git %s", diff --git a/git/rev_list_scanner.go b/git/rev_list_scanner.go index 064785b6..04521331 100644 --- a/git/rev_list_scanner.go +++ b/git/rev_list_scanner.go @@ -169,7 +169,10 @@ func NewRevListScanner(include, excluded []string, opt *ScanRefsOptions) (*RevLi return nil, err } - cmd := gitNoLFS(args...).Cmd + cmd, err := gitNoLFS(args...) + if err != nil { + return nil, err + } if len(opt.WorkingDir) > 0 { cmd.Dir = opt.WorkingDir } diff --git a/lfs/extension.go b/lfs/extension.go index 33a3a061..2b841e8f 100644 --- a/lfs/extension.go +++ b/lfs/extension.go @@ -76,7 +76,11 @@ func pipeExtensions(cfg *config.Configuration, request *pipeRequest) (response p arg := strings.Replace(value, "%f", request.fileName, -1) args = append(args, arg) } - cmd := subprocess.ExecCommand(name, args...) + var cmd *subprocess.Cmd + cmd, err = subprocess.ExecCommand(name, args...) + if err != nil { + return + } ec := &extCommand{cmd: cmd, result: &pipeExtResult{name: e.Name}} extcmds = append(extcmds, ec) } diff --git a/lfshttp/certs_darwin.go b/lfshttp/certs_darwin.go index 2067b2c1..232e9871 100644 --- a/lfshttp/certs_darwin.go +++ b/lfshttp/certs_darwin.go @@ -19,7 +19,11 @@ func appendRootCAsForHostFromPlatform(pool *x509.CertPool, host string) *x509.Ce // either, for consistency. // find system.keychain for user-added certs (don't assume location) - cmd := subprocess.ExecCommand("/usr/bin/security", "list-keychains") + cmd, err := subprocess.ExecCommand("/usr/bin/security", "list-keychains") + if err != nil { + tracerx.Printf("Error getting command to list keychains: %v", err) + return nil + } kcout, err := cmd.Output() if err != nil { tracerx.Printf("Error listing keychains: %v", err) @@ -54,7 +58,11 @@ func appendRootCAsForHostFromPlatform(pool *x509.CertPool, host string) *x509.Ce } func appendRootCAsFromKeychain(pool *x509.CertPool, name, keychain string) *x509.CertPool { - cmd := subprocess.ExecCommand("/usr/bin/security", "find-certificate", "-a", "-p", "-c", name, keychain) + cmd, err := subprocess.ExecCommand("/usr/bin/security", "find-certificate", "-a", "-p", "-c", name, keychain) + if err != nil { + tracerx.Printf("Error getting command to read keychain %q: %v", keychain, err) + return pool + } data, err := cmd.Output() if err != nil { tracerx.Printf("Error reading keychain %q: %v", keychain, err) diff --git a/lfshttp/ssh.go b/lfshttp/ssh.go index 77176186..fa341e04 100644 --- a/lfshttp/ssh.go +++ b/lfshttp/ssh.go @@ -80,7 +80,10 @@ func (c *sshAuthClient) Resolve(e Endpoint, method string) (sshAuthResponse, err } exe, args := ssh.GetLFSExeAndArgs(c.os, c.git, &e.SSHMetadata, "git-lfs-authenticate", endpointOperation(e, method), false) - cmd := subprocess.ExecCommand(exe, args...) + cmd, err := subprocess.ExecCommand(exe, args...) + if err != nil { + return res, err + } // Save stdout and stderr in separate buffers var outbuf, errbuf bytes.Buffer @@ -90,7 +93,7 @@ func (c *sshAuthClient) Resolve(e Endpoint, method string) (sshAuthResponse, err now := time.Now() // Execute command - err := cmd.Start() + err = cmd.Start() if err == nil { err = cmd.Wait() } diff --git a/lfshttp/standalone/standalone.go b/lfshttp/standalone/standalone.go index 768319de..de0cdb22 100644 --- a/lfshttp/standalone/standalone.go +++ b/lfshttp/standalone/standalone.go @@ -119,7 +119,10 @@ func gitDirAtPath(path string) (string, error) { return "", err } - cmd := subprocess.ExecCommand("git", "rev-parse", "--git-dir") + cmd, err := subprocess.ExecCommand("git", "rev-parse", "--git-dir") + if err != nil { + return "", errors.Wrap(err, tr.Tr.Get("failed to find `git rev-parse --git-dir`")) + } cmd.Cmd.Env = env out, err := cmd.Output() if err != nil { diff --git a/ssh/connection.go b/ssh/connection.go index 747d7085..81400587 100644 --- a/ssh/connection.go +++ b/ssh/connection.go @@ -34,7 +34,10 @@ func NewSSHTransfer(osEnv config.Environment, gitEnv config.Environment, meta *S func startConnection(id int, osEnv config.Environment, gitEnv config.Environment, meta *SSHMetadata, operation string) (*PktlineConnection, error) { exe, args := GetLFSExeAndArgs(osEnv, gitEnv, meta, "git-lfs-transfer", operation, true) - cmd := subprocess.ExecCommand(exe, args...) + cmd, err := subprocess.ExecCommand(exe, args...) + if err != nil { + return nil, err + } r, err := cmd.StdoutPipe() if err != nil { return nil, err diff --git a/subprocess/subprocess.go b/subprocess/subprocess.go index 5b6aa058..9ec1f080 100644 --- a/subprocess/subprocess.go +++ b/subprocess/subprocess.go @@ -20,7 +20,10 @@ import ( // stdout & stderr pipes, wrapped in a BufferedCmd. The stdout buffer will be // of stdoutBufSize bytes. func BufferedExec(name string, args ...string) (*BufferedCmd, error) { - cmd := ExecCommand(name, args...) + cmd, err := ExecCommand(name, args...) + if err != nil { + return nil, err + } stdout, err := cmd.StdoutPipe() if err != nil { return nil, err @@ -49,7 +52,11 @@ func BufferedExec(name string, args ...string) (*BufferedCmd, error) { // SimpleExec is a small wrapper around os/exec.Command. func SimpleExec(name string, args ...string) (string, error) { - return Output(ExecCommand(name, args...)) + cmd, err := ExecCommand(name, args...) + if err != nil { + return "", err + } + return Output(cmd) } func Output(cmd *Cmd) (string, error) { diff --git a/subprocess/subprocess_nix.go b/subprocess/subprocess_nix.go index 4b79045e..611fbb17 100644 --- a/subprocess/subprocess_nix.go +++ b/subprocess/subprocess_nix.go @@ -8,9 +8,13 @@ import ( ) // ExecCommand is a small platform specific wrapper around os/exec.Command -func ExecCommand(name string, arg ...string) *Cmd { +func ExecCommand(name string, arg ...string) (*Cmd, error) { cmd := exec.Command(name, arg...) - cmd.Path, _ = LookPath(name) + var err error + cmd.Path, err = LookPath(name) + if err != nil { + return nil, err + } cmd.Env = fetchEnvironment() - return newCmd(cmd) + return newCmd(cmd), nil } diff --git a/subprocess/subprocess_windows.go b/subprocess/subprocess_windows.go index 0af64ff0..d56c7f5e 100644 --- a/subprocess/subprocess_windows.go +++ b/subprocess/subprocess_windows.go @@ -9,10 +9,14 @@ import ( ) // ExecCommand is a small platform specific wrapper around os/exec.Command -func ExecCommand(name string, arg ...string) *Cmd { +func ExecCommand(name string, arg ...string) (*Cmd, error) { cmd := exec.Command(name, arg...) - cmd.Path, _ = LookPath(name) + var err error + cmd.Path, err = LookPath(name) + if err != nil { + return nil, err + } cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} cmd.Env = fetchEnvironment() - return newCmd(cmd) + return newCmd(cmd), nil } diff --git a/t/t-credentials-no-prompt.sh b/t/t-credentials-no-prompt.sh index a57a2e04..7be261d9 100755 --- a/t/t-credentials-no-prompt.sh +++ b/t/t-credentials-no-prompt.sh @@ -45,8 +45,7 @@ begin_test "askpass: push with bad askpass" git config "credential.helper" "" GIT_TERMINAL_PROMPT=0 GIT_ASKPASS="lfs-askpass-2" SSH_ASKPASS="dont-call-me" GIT_TRACE=1 git push origin main 2>&1 | tee push.log - grep "filling with GIT_ASKPASS" push.log # attempt askpass - grep 'credential fill error: exec: "lfs-askpass-2"' push.log # askpass fails + grep "failed to find GIT_ASKPASS command" push.log # attempt askpass grep "creds: git credential fill" push.log # attempt git credential ) end_test diff --git a/t/t-path.sh b/t/t-path.sh index e25ba391..168df350 100755 --- a/t/t-path.sh +++ b/t/t-path.sh @@ -117,3 +117,37 @@ begin_test "does not look in current directory for git with credential helper" [ ! -f exploit ] ) end_test + +begin_test "does not look in current directory for wrong binary using PATHEXT" +( + set -e + + # Windows is the only platform where Go searches for executable files + # by appending file extensions from PATHEXT. + [ "$IS_WINDOWS" -eq 0 ] && exit 0 + + reponame="$(basename "$0" ".sh")-notfound" + git init "$reponame" + cd "$reponame" + + # Go on Windows always looks in the current directory first when creating + # a command handler, so we need a dummy git.exe for it to find there since + # we will restrict PATH to exclude the real Git when we run "git-lfs env" + # below. If our git-lfs incorrectly proceeds to run the command handler + # despite not finding Git in PATH either, Go may then search for a file + # named "." with any path extension from PATHEXT and execute that file + # instead, so we create a malicious file named "..exe" to check this case. + touch "git$X" + cp "$BINPATH/lfstest-badpathcheck$X" ".$X" + + # This should always succeed, even if git-lfs is incorrectly searching for + # executables in the current directory first, because the "git-lfs env" + # command ignores all errors when it runs "git config". So we should always + # pass this step and then, if our malicious program was executed, detect + # its output below. If this command does fail, something else is wrong. + PATH="$BINPATH" PATHEXT="$X" "git-lfs$X" env >output.log 2>&1 + + grep "exploit" output.log && false + [ ! -f exploit ] +) +end_test diff --git a/tools/cygwin_windows.go b/tools/cygwin_windows.go index 36f42854..aa1b93e5 100644 --- a/tools/cygwin_windows.go +++ b/tools/cygwin_windows.go @@ -38,7 +38,10 @@ func isCygwin() bool { return cygwinState.Enabled() } - cmd := subprocess.ExecCommand("uname") + cmd, err := subprocess.ExecCommand("uname") + if err != nil { + return false + } out, err := cmd.Output() if err != nil { return false diff --git a/tools/os_tools.go b/tools/os_tools.go index be9a2100..bac09268 100644 --- a/tools/os_tools.go +++ b/tools/os_tools.go @@ -28,7 +28,12 @@ func Getwd() (dir string, err error) { } func translateCygwinPath(path string) (string, error) { - cmd := subprocess.ExecCommand("cygpath", "-w", path) + cmd, err := subprocess.ExecCommand("cygpath", "-w", path) + if err != nil { + // If cygpath doesn't exist, that's okay: just return the paths + // as we got it. + return path, nil + } // cygpath uses ISO-8850-1 as the default encoding if the locale is not // set, resulting in breakage, since we want a UTF-8 path. env := make([]string, 0, len(cmd.Env)+1) diff --git a/tq/custom.go b/tq/custom.go index 2ad8f071..773a6aa4 100644 --- a/tq/custom.go +++ b/tq/custom.go @@ -125,7 +125,10 @@ func (a *customAdapter) WorkerStarting(workerNum int) (interface{}, error) { // If concurrent = false we have already dialled back workers to 1 a.Trace("xfer: starting up custom transfer process %q for worker %d", a.name, workerNum) cmdName, cmdArgs := subprocess.FormatForShell(subprocess.ShellQuoteSingle(a.path), a.args) - cmd := subprocess.ExecCommand(cmdName, cmdArgs...) + cmd, err := subprocess.ExecCommand(cmdName, cmdArgs...) + if err != nil { + return nil, errors.New(tr.Tr.Get("failed to find custom transfer command %q remote: %v", a.path, err)) + } outp, err := cmd.StdoutPipe() if err != nil { return nil, errors.New(tr.Tr.Get("failed to get stdout for custom transfer command %q remote: %v", a.path, err))