From 8eba631f8d1b0bbf6c78b7ea2309346621dc6d5a 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 Signed-off-by: Earl Warren --- routers/private/hook_pre_receive.go | 61 +++++++++++++++++++ tests/integration/git_push_test.go | 92 +++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 061349284..cb356a184 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -4,6 +4,7 @@ package private import ( + "errors" "fmt" "net/http" "os" @@ -101,6 +102,60 @@ func (ctx *preReceiveContext) AssertCreatePullRequest() bool { return true } +var errPermissionDenied = errors.New("permission denied for changing repo settings") + +func (ctx *preReceiveContext) canChangeSettings() error { + if !ctx.loadPusherAndPermission() { + return errPermissionDenied + } + + if !ctx.userPerm.IsOwner() && !ctx.userPerm.IsAdmin() { + return errPermissionDenied + } + + if ctx.Repo.Repository.IsFork { + return errPermissionDenied + } + + return nil +} + +func (ctx *preReceiveContext) validatePushOptions() error { + opts := web.GetForm(ctx).(*private.HookOptions) + + if len(opts.GitPushOptions) == 0 { + return nil + } + + changesRepoSettings := false + for key := range opts.GitPushOptions { + switch key { + case private.GitPushOptionRepoPrivate, private.GitPushOptionRepoTemplate: + changesRepoSettings = true + case "topic", "force-push", "title", "description": + // Agit options + default: + return fmt.Errorf("unknown option %s", key) + } + } + + if changesRepoSettings { + return ctx.canChangeSettings() + } + + return nil +} + +func (ctx *preReceiveContext) assertPushOptions() bool { + if err := ctx.validatePushOptions(); err != nil { + ctx.JSON(http.StatusForbidden, private.Response{ + UserMsg: fmt.Sprintf("options validation failed: %v", err), + }) + 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 +166,12 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { opts: opts, } + if !ourCtx.assertPushOptions() { + log.Trace("Git push options validation failed") + return + } + log.Trace("Git push options validation succeeded") + // 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 0a3572480..838ee0ff7 100644 --- a/tests/integration/git_push_test.go +++ b/tests/integration/git_push_test.go @@ -7,12 +7,17 @@ import ( "fmt" "net/url" "testing" + "time" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" + 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" + "code.gitea.io/gitea/modules/log" + repo_module "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/test" repo_service "code.gitea.io/gitea/services/repository" "github.com/stretchr/testify/assert" @@ -146,3 +151,90 @@ func runTestGitPush(t *testing.T, u *url.URL, gitOperation func(t *testing.T, gi require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, repo.ID)) } + +func TestOptionsGitPush(t *testing.T) { + onGiteaRun(t, testOptionsGitPush) +} + +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{ + Name: "repo-to-push", + Description: "test git push", + AutoInit: false, + DefaultBranch: "main", + IsPrivate: false, + }) + require.NoError(t, err) + require.NotEmpty(t, repo) + + gitPath := t.TempDir() + + doGitInitTestRepository(gitPath)(t) + + u.Path = repo.FullName() + ".git" + u.User = url.UserPassword(user.LowerName, userPassword) + doGitAddRemote(gitPath, "origin", u)(t) + + t.Run("Unknown push options are rejected", func(t *testing.T) { + logChecker, cleanup := test.NewLogChecker(log.DEFAULT, log.TRACE) + logChecker.Filter("unknown option").StopMark("Git push options validation") + defer cleanup() + branchName := "branch0" + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepositoryFail(gitPath, "origin", branchName, "-o", "repo.template=false", "-o", "uknownoption=randomvalue")(t) + logFiltered, logStopped := logChecker.Check(5 * time.Second) + assert.True(t, logStopped) + assert.True(t, logFiltered[0]) + }) + + t.Run("Owner sets private & template to true via push options", func(t *testing.T) { + 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) + }) + + t.Run("Owner sets private & template to false via push options", func(t *testing.T) { + 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) + }) + + // 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_module.AddCollaborator(db.DefaultContext, repo, collaborator) + + t.Run("Collaborator with write access is allowed to push", func(t *testing.T) { + branchName := "branch3" + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepository(gitPath, "collaborator", branchName)(t) + }) + + t.Run("Collaborator with write access fails to change private & template via push options", func(t *testing.T) { + logChecker, cleanup := test.NewLogChecker(log.DEFAULT, log.TRACE) + logChecker.Filter("permission denied for changing repo settings").StopMark("Git push options validation") + defer cleanup() + 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) + logFiltered, logStopped := logChecker.Check(5 * time.Second) + assert.True(t, logStopped) + assert.True(t, logFiltered[0]) + }) + + require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, repo.ID)) +}