From e56612ece27e5b98007ba05351c73aa0a4b36708 Mon Sep 17 00:00:00 2001
From: Paul Okstad <pokstad@gitlab.com>
Date: Fri, 24 Jan 2020 14:11:15 +0000
Subject: Replace CommandWithoutRepo usage with safe version

---
 .../unreleased/po-safecmd-commandwithoutrepo.yml   |  5 ++++
 internal/git/objectpool/clone.go                   | 19 +++++++++++--
 internal/git/objectpool/pool.go                    | 11 ++++++--
 internal/service/remote/remotes.go                 | 10 ++++++-
 internal/service/repository/create.go              | 11 +++++++-
 internal/service/repository/create_from_bundle.go  | 33 +++++++++++-----------
 6 files changed, 66 insertions(+), 23 deletions(-)
 create mode 100644 changelogs/unreleased/po-safecmd-commandwithoutrepo.yml

diff --git a/changelogs/unreleased/po-safecmd-commandwithoutrepo.yml b/changelogs/unreleased/po-safecmd-commandwithoutrepo.yml
new file mode 100644
index 000000000..eb6479124
--- /dev/null
+++ b/changelogs/unreleased/po-safecmd-commandwithoutrepo.yml
@@ -0,0 +1,5 @@
+---
+title: Replace CommandWithoutRepo usage with safe version
+merge_request: 1783
+author:
+type: security
diff --git a/internal/git/objectpool/clone.go b/internal/git/objectpool/clone.go
index f7359e636..45ab2af81 100644
--- a/internal/git/objectpool/clone.go
+++ b/internal/git/objectpool/clone.go
@@ -24,8 +24,23 @@ func (o *ObjectPool) clone(ctx context.Context, repo *gitalypb.Repository) error
 		return err
 	}
 
-	cloneArgs := []string{"-C", path.Dir(targetDir), "clone", "--quiet", "--bare", "--local", repoPath, targetName}
-	cmd, err := git.CommandWithoutRepo(ctx, cloneArgs...)
+	cmd, err := git.SafeCmdWithoutRepo(ctx,
+		[]git.Option{
+			git.ValueFlag{
+				Name:  "-C",
+				Value: path.Dir(targetDir),
+			},
+		},
+		git.SubCmd{
+			Name: "clone",
+			Flags: []git.Option{
+				git.Flag{Name: "--quiet"},
+				git.Flag{Name: "--bare"},
+				git.Flag{Name: "--local"},
+			},
+			Args: []string{repoPath, targetName},
+		},
+	)
 	if err != nil {
 		return err
 	}
diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go
index 5674421ab..ca4628d1b 100644
--- a/internal/git/objectpool/pool.go
+++ b/internal/git/objectpool/pool.go
@@ -108,8 +108,15 @@ func (o *ObjectPool) Init(ctx context.Context) (err error) {
 		return nil
 	}
 
-	initArgs := []string{"init", "--bare", targetDir}
-	cmd, err := git.CommandWithoutRepo(ctx, initArgs...)
+	cmd, err := git.SafeCmdWithoutRepo(ctx, nil,
+		git.SubCmd{
+			Name: "init",
+			Flags: []git.Option{
+				git.Flag{Name: "--bare"},
+			},
+			Args: []string{targetDir},
+		},
+	)
 	if err != nil {
 		return err
 	}
diff --git a/internal/service/remote/remotes.go b/internal/service/remote/remotes.go
index e4a0893e2..7e18fda41 100644
--- a/internal/service/remote/remotes.go
+++ b/internal/service/remote/remotes.go
@@ -73,7 +73,15 @@ func (s *server) FindRemoteRepository(ctx context.Context, req *gitalypb.FindRem
 		return nil, status.Error(codes.InvalidArgument, "FindRemoteRepository: empty remote can't be checked.")
 	}
 
-	cmd, err := git.CommandWithoutRepo(ctx, "ls-remote", req.GetRemote(), "HEAD")
+	cmd, err := git.SafeCmdWithoutRepo(ctx, nil,
+		git.SubCmd{
+			Name: "ls-remote",
+			Args: []string{
+				req.GetRemote(),
+				"HEAD",
+			},
+		},
+	)
 
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "error executing git command: %s", err)
diff --git a/internal/service/repository/create.go b/internal/service/repository/create.go
index bfbbb6864..241a26e05 100644
--- a/internal/service/repository/create.go
+++ b/internal/service/repository/create.go
@@ -19,7 +19,16 @@ func (s *server) CreateRepository(ctx context.Context, req *gitalypb.CreateRepos
 		return nil, helper.ErrInternal(err)
 	}
 
-	cmd, err := git.CommandWithoutRepo(ctx, "init", "--bare", "--quiet", diskPath)
+	cmd, err := git.SafeCmdWithoutRepo(ctx, nil,
+		git.SubCmd{
+			Name: "init",
+			Flags: []git.Option{
+				git.Flag{Name: "--bare"},
+				git.Flag{Name: "--quiet"},
+			},
+			Args: []string{diskPath},
+		},
+	)
 	if err != nil {
 		return nil, helper.ErrInternal(err)
 	}
diff --git a/internal/service/repository/create_from_bundle.go b/internal/service/repository/create_from_bundle.go
index 045132048..07f9da60a 100644
--- a/internal/service/repository/create_from_bundle.go
+++ b/internal/service/repository/create_from_bundle.go
@@ -64,14 +64,15 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr
 		return err
 	}
 
-	args := []string{
-		"clone",
-		"--bare",
-		"--",
-		bundlePath,
-		repoPath,
-	}
-	cmd, err := git.CommandWithoutRepo(ctx, args...)
+	cmd, err := git.SafeCmdWithoutRepo(ctx, nil,
+		git.SubCmd{
+			Name: "clone",
+			Flags: []git.Option{
+				git.Flag{Name: "--bare"},
+			},
+			PostSepArgs: []string{bundlePath, repoPath},
+		},
+	)
 	if err != nil {
 		cleanError := sanitizedError(repoPath, "CreateRepositoryFromBundle: cmd start failed: %v", err)
 		return status.Error(codes.Internal, cleanError)
@@ -82,15 +83,13 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr
 	}
 
 	// We do a fetch to get all refs including keep-around refs
-	args = []string{
-		"-C",
-		repoPath,
-		"fetch",
-		bundlePath,
-		"refs/*:refs/*",
-	}
-
-	cmd, err = git.CommandWithoutRepo(ctx, args...)
+	cmd, err = git.SafeCmdWithoutRepo(ctx,
+		[]git.Option{git.ValueFlag{Name: "-C", Value: repoPath}},
+		git.SubCmd{
+			Name: "fetch",
+			Args: []string{bundlePath, "refs/*:refs/*"},
+		},
+	)
 	if err != nil {
 		cleanError := sanitizedError(repoPath, "CreateRepositoryFromBundle: cmd start failed fetching refs: %v", err)
 		return status.Error(codes.Internal, cleanError)
-- 
cgit v1.2.3