diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-05-02 15:32:05 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-05-02 15:40:35 +0300 |
commit | e134a10baea70bbf661a7ba4d9d3857809f87564 (patch) | |
tree | 1da536b694021e1d7b939fc2c10373a9042d188f | |
parent | cc8901bc9389a9f85267fe820acc3777494ea337 (diff) |
gittest: Do not run commands in seed Git repositories directly
While we already ensure that no seed test Git repositories are being
written into by running tests under a different user in our CI, we don't
check whether the repositories are used to spawn any read-only commands.
And this has in fact been fine until now: permissions were such that
this always worked.
This has changed with CVE-2022-24765 though: Git has started to operate
in repositories completely in case it is owned by a different user. As a
consequence, this breaks our testing infrastructure whenever a test is
trying to run a command in the test seed repositories directly given
that they're owned by `root`, and we ourselves run with UID 9999 in our
CI.
Luckily, commands like git-upload-pack(1) still work, which means that
we're still able to clone such repositories. And there are only very few
tests which don't first use e.g. `gittest.CloneRepository()` before
actually run commands directly in there, too, because we don't expose a
simple way to obtain the path of those seed repositories outside of the
`gittest` package. The only offenders are `gittest.ChecksumTestRepo()`
and `gittest.BundleTestRepo()`.
Refactor those two helper functions to instead operate on a cloned
repository instead of running commands in the seed repositories
directly. This fixes compatibility with all Git versions which include a
fix for above CVE.
-rw-r--r-- | internal/backup/backup_test.go | 22 | ||||
-rw-r--r-- | internal/git/gittest/repo.go | 12 | ||||
-rw-r--r-- | internal/gitaly/service/repository/fetch_bundle_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack_test.go | 3 |
4 files changed, 20 insertions, 19 deletions
diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index d47221f6a..eecacaa58 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -301,8 +301,12 @@ func testManagerRestore(t *testing.T, ctx context.Context) { return repo } + _, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{ + Seed: gittest.SeedGitLabTest, + }) + repoChecksum := gittest.ChecksumRepo(t, cfg, repoPath) + path := testhelper.TempDir(t) - testRepoChecksum := gittest.ChecksumTestRepo(t, cfg, "gitlab-test.git") for _, tc := range []struct { desc string @@ -320,9 +324,9 @@ func testManagerRestore(t *testing.T, ctx context.Context) { repo := createRepo(t) require.NoError(t, os.MkdirAll(filepath.Join(path, repo.RelativePath), os.ModePerm)) bundlePath := filepath.Join(path, repo.RelativePath+".bundle") - gittest.BundleTestRepo(t, cfg, "gitlab-test.git", bundlePath) + gittest.BundleRepo(t, cfg, repoPath, bundlePath) - return repo, testRepoChecksum + return repo, repoChecksum }, expectExists: true, }, @@ -334,10 +338,10 @@ func testManagerRestore(t *testing.T, ctx context.Context) { bundlePath := filepath.Join(path, repo.RelativePath+".bundle") customHooksPath := filepath.Join(path, repo.RelativePath, "custom_hooks.tar") require.NoError(t, os.MkdirAll(filepath.Join(path, repo.RelativePath), os.ModePerm)) - gittest.BundleTestRepo(t, cfg, "gitlab-test.git", bundlePath) + gittest.BundleRepo(t, cfg, repoPath, bundlePath) testhelper.CopyFile(t, "../gitaly/service/repository/testdata/custom_hooks.tar", customHooksPath) - return repo, testRepoChecksum + return repo, repoChecksum }, expectedPaths: []string{ "custom_hooks/pre-commit.sample", @@ -376,9 +380,9 @@ func testManagerRestore(t *testing.T, ctx context.Context) { require.NoError(t, os.MkdirAll(filepath.Dir(filepath.Join(path, repo.RelativePath)), os.ModePerm)) bundlePath := filepath.Join(path, repo.RelativePath+".bundle") - gittest.BundleTestRepo(t, cfg, "gitlab-test.git", bundlePath) + gittest.BundleRepo(t, cfg, repoPath, bundlePath) - return repo, testRepoChecksum + return repo, repoChecksum }, expectExists: true, }, @@ -394,9 +398,9 @@ func testManagerRestore(t *testing.T, ctx context.Context) { require.NoError(t, os.WriteFile(filepath.Join(repoBackupPath, "LATEST"), []byte(backupID), os.ModePerm)) require.NoError(t, os.WriteFile(filepath.Join(backupPath, "LATEST"), []byte("001"), os.ModePerm)) bundlePath := filepath.Join(backupPath, "001.bundle") - gittest.BundleTestRepo(t, cfg, "gitlab-test.git", bundlePath) + gittest.BundleRepo(t, cfg, repoPath, bundlePath) - return repo, testRepoChecksum + return repo, repoChecksum }, expectExists: true, }, diff --git a/internal/git/gittest/repo.go b/internal/git/gittest/repo.go index 1d606fa7f..020ae8271 100644 --- a/internal/git/gittest/repo.go +++ b/internal/git/gittest/repo.go @@ -315,22 +315,18 @@ func CloneRepo(t testing.TB, cfg config.Cfg, storage config.Storage, opts ...Clo return repo, absolutePath } -// BundleTestRepo creates a bundle of a local test repo. E.g. -// `gitlab-test.git`. `patterns` define the bundle contents as per +// BundleRepo creates a bundle of a repository. `patterns` define the bundle contents as per // `git-rev-list-args`. If there are no patterns then `--all` is assumed. -func BundleTestRepo(t testing.TB, cfg config.Cfg, sourceRepo, bundlePath string, patterns ...string) { +func BundleRepo(t testing.TB, cfg config.Cfg, repoPath, bundlePath string, patterns ...string) { if len(patterns) == 0 { patterns = []string{"--all"} } - repoPath := testRepositoryPath(t, sourceRepo) Exec(t, cfg, append([]string{"-C", repoPath, "bundle", "create", bundlePath}, patterns...)...) } -// ChecksumTestRepo calculates the checksum of a local test repo. E.g. -// `gitlab-test.git`. -func ChecksumTestRepo(t testing.TB, cfg config.Cfg, sourceRepo string) *git.Checksum { +// ChecksumRepo calculates the checksum of a repository. +func ChecksumRepo(t testing.TB, cfg config.Cfg, repoPath string) *git.Checksum { var checksum git.Checksum - repoPath := testRepositoryPath(t, sourceRepo) lines := bytes.Split(Exec(t, cfg, "-C", repoPath, "show-ref", "--head"), []byte("\n")) for _, line := range lines { checksum.AddBytes(line) diff --git a/internal/gitaly/service/repository/fetch_bundle_test.go b/internal/gitaly/service/repository/fetch_bundle_test.go index f11327fce..4f30b9f18 100644 --- a/internal/gitaly/service/repository/fetch_bundle_test.go +++ b/internal/gitaly/service/repository/fetch_bundle_test.go @@ -101,7 +101,7 @@ func TestServer_FetchBundle_transaction(t *testing.T) { tmp := testhelper.TempDir(t) bundlePath := filepath.Join(tmp, "test.bundle") - gittest.BundleTestRepo(t, cfg, "gitlab-test.git", bundlePath) + gittest.BundleRepo(t, cfg, repoPath, bundlePath) hookManager.Reset() _, stopGitServer := gittest.HTTPServer(ctx, t, gitCmdFactory, repoPath, nil) diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 54f07fe26..8b56ce272 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -839,7 +839,8 @@ func TestPostReceive_allRejected(t *testing.T) { ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, - stdin io.Reader, stdout, stderr io.Writer) error { + stdin io.Reader, stdout, stderr io.Writer, + ) error { return errors.New("not allowed") }, gitalyhook.NopPostReceive, |