Use subprocess for invoking all commands

The fix for CVE-2020-27955 was incomplete because we did not consider places outside of the subprocess code that invoke binaries. As a result, there are still some places where an attacker can execute arbitrary code by placing a malicious binary in the repository. To make sure we've covered all the bases, let's just use the subprocess code for executing all programs, which means that they'll be secure. As of this commit, all users of exec.Command are in test code or the subprocess code itself.
mjydtskhyrtlb · Dec 21, 2020 · 10c4ffc · 10c4ffc
1 parent 7b8779f
commit 10c4ffc
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 9 deletions.
diff --git a/commands/commands.go b/commands/commands.go
@@ -7,7 +7,6 @@ import (
 	"log"
 	"net"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -282,7 +281,7 @@ func PipeMediaCommand(name string, args ...string) error {
 }
 
 func PipeCommand(name string, args ...string) error {
-	cmd := exec.Command(name, args...)
+	cmd := subprocess.ExecCommand(name, args...)
 	cmd.Stdin = os.Stdin
 	cmd.Stderr = os.Stderr
 	cmd.Stdout = os.Stdout

diff --git a/creds/creds.go b/creds/creds.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/git-lfs/git-lfs/config"
 	"github.com/git-lfs/git-lfs/errors"
+	"github.com/git-lfs/git-lfs/subprocess"
 	"github.com/rubyist/tracerx"
 )
 
@@ -232,7 +233,7 @@ 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 := exec.Command(a.Program, a.args(fmt.Sprintf("%s for %q", valueString, u))...)
+	cmd := subprocess.ExecCommand(a.Program, a.args(fmt.Sprintf("%s for %q", valueString, u))...)
 	cmd.Stderr = &err
 	cmd.Stdout = &value
 
@@ -292,7 +293,7 @@ func (h *commandCredentialHelper) Approve(creds Creds) error {
 
 func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, error) {
 	output := new(bytes.Buffer)
-	cmd := exec.Command("git", "credential", subcommand)
+	cmd := subprocess.ExecCommand("git", "credential", subcommand)
 	cmd.Stdin = bufferCreds(input)
 	cmd.Stdout = output
 	/*

diff --git a/lfs/extension.go b/lfs/extension.go
@@ -8,10 +8,10 @@ import (
 	"hash"
 	"io"
 	"os"
-	"os/exec"
 	"strings"
 
 	"github.com/git-lfs/git-lfs/config"
+	"github.com/git-lfs/git-lfs/subprocess"
 )
 
 type pipeRequest struct {
@@ -33,7 +33,7 @@ type pipeExtResult struct {
 }
 
 type extCommand struct {
-	cmd    *exec.Cmd
+	cmd    *subprocess.Cmd
 	out    io.WriteCloser
 	err    *bytes.Buffer
 	hasher hash.Hash
@@ -75,7 +75,7 @@ func pipeExtensions(cfg *config.Configuration, request *pipeRequest) (response p
 			arg := strings.Replace(value, "%f", request.fileName, -1)
 			args = append(args, arg)
 		}
-		cmd := exec.Command(name, args...)
+		cmd := subprocess.ExecCommand(name, args...)
 		ec := &extCommand{cmd: cmd, result: &pipeExtResult{name: e.Name}}
 		extcmds = append(extcmds, ec)
 	}

diff --git a/lfshttp/ssh.go b/lfshttp/ssh.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"os/exec"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -83,7 +82,7 @@ func (c *sshAuthClient) Resolve(e Endpoint, method string) (sshAuthResponse, err
 	}
 
 	exe, args := sshGetLFSExeAndArgs(c.os, c.git, e, method)
-	cmd := exec.Command(exe, args...)
+	cmd := subprocess.ExecCommand(exe, args...)
 
 	// Save stdout and stderr in separate buffers
 	var outbuf, errbuf bytes.Buffer

diff --git a/t/t-path.sh b/t/t-path.sh
@@ -21,3 +21,41 @@ begin_test "does not look in current directory for git"
   ! grep -q 'exploit' output.log
 )
 end_test
+
+begin_test "does not look in current directory for git with credential helper"
+(
+  set -e
+
+  reponame="$(basename "$0" ".sh")-credentials"
+  setup_remote_repo "$reponame"
+
+  clone_repo "$reponame" credentials-1
+  export PATH="$(echo "$PATH" | sed -e "s/:.:/:/g" -e "s/::/:/g")"
+
+  printf "#!/bin/sh\necho exploit >&2\ntouch exploit\n" > git
+  chmod +x git || true
+  printf "echo exploit 1>&2\r\necho >exploit" > git.bat
+
+  git lfs track "*.dat"
+  printf abc > z.dat
+  git add z.dat
+  git add .gitattributes
+  git add git git.bat
+  git commit -m "Add files"
+
+  git push origin HEAD
+  cd ..
+
+  unset GIT_ASKPASS SSH_ASKPASS
+
+  # This needs to succeed.  If it fails, that could be because our malicious
+  # "git" is broken but got invoked anyway.
+  GIT_LFS_SKIP_SMUDGE=1 clone_repo "$reponame" credentials-2
+
+  git lfs pull | tee output.log
+
+  ! grep -q 'exploit' output.log
+  [ ! -f ../exploit ]
+  [ ! -f exploit ]
+)
+end_test