From c8645d2a708759527d101b1deaf7bc0c1313190b Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Mon, 15 Apr 2024 10:07:45 +0200 Subject: [PATCH] hooks: Harden when we accept push options that change repo settings It is possible to change some repo settings (its visibility, and template status) via `git push` options: `-o repo.private=true`, `-o repo.template=true`. Previously, there weren't sufficient permission checks on these, and anyone who could `git push` to a repository - including via an AGit workflow! - was able to change either of these settings. To guard against this, the pre-receive hook will now check if either of these options are present, and if so, will perform additional permission checks to ensure that these can only be set by a repository owner or an administrator. Additionally, changing these settings is disabled for forks, even for the fork's owner. There's still a case where the owner of a repository can change the visibility of it, and it will not propagate to forks (it propagates to forks when changing the visibility via the API), but that's an inconsistency, not a security issue. Signed-off-by: Gergely Nagy (cherry picked from commit cc80e661531794fff7f8a336eaaefdb7e3bd3956) Conflicts: tests/integration/git_push_test.go DeleteRepositoryDirectly does not exist CreateRepoOptions is in repo_module --- routers/private/hook_pre_receive.go | 55 ++++++++++++++++++++ tests/integration/git_push_test.go | 80 +++++++++++++++++++---------- 2 files changed, 108 insertions(+), 27 deletions(-) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 7f37d73795..947edb77aa 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -101,6 +101,57 @@ func (ctx *preReceiveContext) AssertCreatePullRequest() bool { return true } +func (ctx *preReceiveContext) canChangeSettings() bool { + if !ctx.loadPusherAndPermission() { + return false + } + + perm, err := access_model.GetUserRepoPermission(ctx, ctx.Repo.Repository, ctx.user) + if err != nil { + return false + } + if !perm.IsOwner() && !perm.IsAdmin() { + return false + } + + if ctx.Repo.Repository.IsFork { + return false + } + + return true +} + +func (ctx *preReceiveContext) assertChangeSettings() bool { + opts := web.GetForm(ctx).(*private.HookOptions) + + if len(opts.GitPushOptions) == 0 { + return true + } + + _, hasPrivateOpt := opts.GitPushOptions[private.GitPushOptionRepoPrivate] + _, hasTemplateOpt := opts.GitPushOptions[private.GitPushOptionRepoTemplate] + + if !hasPrivateOpt && !hasTemplateOpt { + // If neither `repo.private` nor `repo.template` is present in + // the push options, we're good to go without further permission + // checking. + return true + } + + // Either `repo.private` or `repo.template` is among the push options, + // do some permission checks. + if !ctx.canChangeSettings() { + if ctx.Written() { + return false + } + ctx.JSON(http.StatusForbidden, private.Response{ + UserMsg: "Permission denied for changing repo settings.", + }) + return false + } + return true +} + // HookPreReceive checks whether a individual commit is acceptable func HookPreReceive(ctx *gitea_context.PrivateContext) { opts := web.GetForm(ctx).(*private.HookOptions) @@ -111,6 +162,10 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { opts: opts, } + if !ourCtx.assertChangeSettings() { + return + } + // Iterate across the provided old commit IDs for i := range opts.OldCommitIDs { oldCommitID := opts.OldCommitIDs[i] diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go index a64c2953c5..55a856fe93 100644 --- a/tests/integration/git_push_test.go +++ b/tests/integration/git_push_test.go @@ -8,31 +8,22 @@ import ( "testing" "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" + repo_module "code.gitea.io/gitea/modules/repository" repo_service "code.gitea.io/gitea/services/repository" "github.com/stretchr/testify/require" ) -func TestGitPush(t *testing.T) { - onGiteaRun(t, testGitPush) +func TestOptionsGitPush(t *testing.T) { + onGiteaRun(t, testOptionsGitPush) } -func testGitPush(t *testing.T, u *url.URL) { - t.Run("Push branch with options", func(t *testing.T) { - runTestGitPush(t, u, func(t *testing.T, gitPath string) { - branchName := "branch-with-options" - doGitCreateBranch(gitPath, branchName)(t) - doGitPushTestRepository(gitPath, "origin", branchName, "-o", "repo.private=true", "-o", "repo.template=true")(t) - }) - }) -} - -func runTestGitPush(t *testing.T, u *url.URL, gitOperation func(t *testing.T, gitPath string)) { +func testOptionsGitPush(t *testing.T, u *url.URL) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - repo, err := repo_service.CreateRepository(db.DefaultContext, user, user, repo_service.CreateRepoOptions{ + repo, err := repo_service.CreateRepository(db.DefaultContext, user, user, repo_module.CreateRepoOptions{ Name: "repo-to-push", Description: "test git push", AutoInit: false, @@ -46,22 +37,57 @@ func runTestGitPush(t *testing.T, u *url.URL, gitOperation func(t *testing.T, gi doGitInitTestRepository(gitPath)(t) - oldPath := u.Path - oldUser := u.User - defer func() { - u.Path = oldPath - u.User = oldUser - }() u.Path = repo.FullName() + ".git" u.User = url.UserPassword(user.LowerName, userPassword) - doGitAddRemote(gitPath, "origin", u)(t) - gitRepo, err := git.OpenRepository(git.DefaultContext, gitPath) - require.NoError(t, err) - defer gitRepo.Close() + { + // owner sets private & template to true via push options + branchName := "branch1" + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepository(gitPath, "origin", branchName, "-o", "repo.private=true", "-o", "repo.template=true")(t) + repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, "repo-to-push") + require.NoError(t, err) + require.True(t, repo.IsPrivate) + require.True(t, repo.IsTemplate) + } - gitOperation(t, gitPath) + { + // owner sets private & template to false via push options + branchName := "branch2" + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepository(gitPath, "origin", branchName, "-o", "repo.private=false", "-o", "repo.template=false")(t) + repo, err = repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, "repo-to-push") + require.NoError(t, err) + require.False(t, repo.IsPrivate) + require.False(t, repo.IsTemplate) + } - require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, user.ID, repo.ID)) + { + // create a collaborator with write access + collaborator := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + u.User = url.UserPassword(collaborator.LowerName, userPassword) + doGitAddRemote(gitPath, "collaborator", u)(t) + repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, "repo-to-push") + require.NoError(t, err) + repo_module.AddCollaborator(db.DefaultContext, repo, collaborator) + } + + { + // collaborator with write access is allowed to push + branchName := "branch3" + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepository(gitPath, "collaborator", branchName)(t) + } + + { + // collaborator with write access fails to change private & template via push options + branchName := "branch4" + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepositoryFail(gitPath, "collaborator", branchName, "-o", "repo.private=true", "-o", "repo.template=true")(t) + repo, err = repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, "repo-to-push") + require.NoError(t, err) + require.False(t, repo.IsPrivate) + require.False(t, repo.IsTemplate) + } }