From 75e1f72c6ede6d85c7c4fd7457fc5c0e40490116 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Apr 2026 15:17:39 +0200 Subject: [PATCH] fix(security): move reparent Admin gate into UpdateProject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-2vq4-854f-5c72 / CVE-2026-35595: the recursive permission CTE cascades Admin from any owned ancestor, so a user with Write on a shared project could reparent it under an attacker-owned root and resolve as Admin on the moved project via the new parent. Require Admin on both the moved project and the new parent whenever parent_project_id is set to a non-zero value that differs from the stored value. The gate lives in UpdateProject rather than CanUpdate because CanUpdate is reused by permission-check-only callers (buckets, webhooks, task ops) that pass stub &Project{ID:...} values with ParentProjectID=0 and never commit a reparent — gating there would spuriously trip the check for every such call. Only non-zero ParentProjectID is gated: the generic update handler binds a fresh struct, so an omitted parent_project_id is indistinguishable from an explicit 0. Detach-to-root via the generic endpoint is therefore out of scope for this fix and is tracked as a follow-up (needs a pointer field to disambiguate). --- pkg/models/project.go | 34 +++++++++++++++ pkg/models/project_permissions.go | 6 +++ pkg/models/project_test.go | 72 ++++++++----------------------- pkg/webtests/project_test.go | 9 ++-- 4 files changed, 64 insertions(+), 57 deletions(-) diff --git a/pkg/models/project.go b/pkg/models/project.go index 2f5b95625..39881b06a 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -1006,6 +1006,40 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje return } + // GHSA-2vq4-854f-5c72 / CVE-2026-35595: the recursive permission CTE + // cascades Admin from any owned ancestor, so moving a shared child + // under an attacker-owned root grants Admin on the child. Require + // Admin on both sides of a reparent. + // + // Only gate on non-zero ParentProjectID: the generic update handler + // binds a fresh struct, so an omitted parent_project_id is + // indistinguishable from an explicit 0. Detach-to-root is therefore + // out of scope here — a proper fix needs a pointer field. + if project.ParentProjectID > 0 { + storedProject, err := GetProjectSimpleByID(s, project.ID) + if err != nil { + return err + } + if project.ParentProjectID != storedProject.ParentProjectID { + canAdminMoved, err := project.IsAdmin(s, auth) + if err != nil { + return err + } + if !canAdminMoved { + return ErrGenericForbidden{} + } + + newParent := &Project{ID: project.ParentProjectID} + canAdminNewParent, err := newParent.IsAdmin(s, auth) + if err != nil { + return err + } + if !canAdminNewParent { + return ErrGenericForbidden{} + } + } + } + if project.IsArchived { isDefaultProject, err := project.isDefaultProject(s) if err != nil { diff --git a/pkg/models/project_permissions.go b/pkg/models/project_permissions.go index ea22983af..bf4351cda 100644 --- a/pkg/models/project_permissions.go +++ b/pkg/models/project_permissions.go @@ -136,6 +136,12 @@ func (p *Project) CanUpdate(s *xorm.Session, a web.Auth) (canUpdate bool, err er // Check if we're moving the project to a different parent project. // If that is the case, we need to verify permissions to do so. + // + // The reparent Admin gate for GHSA-2vq4-854f-5c72 / CVE-2026-35595 + // lives in UpdateProject, not here: CanUpdate is reused by + // permission-check-only callers (buckets, webhooks, task ops) that + // pass stub &Project{ID: ...} values with ParentProjectID=0 and never + // commit a reparent, which would spuriously trip the gate. if p.ParentProjectID != 0 && p.ParentProjectID != ol.ParentProjectID { newProject := &Project{ID: p.ParentProjectID} can, err := newProject.CanWrite(s, a) diff --git a/pkg/models/project_test.go b/pkg/models/project_test.go index 0ecc322fa..59da28d7c 100644 --- a/pkg/models/project_test.go +++ b/pkg/models/project_test.go @@ -288,12 +288,11 @@ func TestProject_CreateOrUpdate(t *testing.T) { assert.True(t, IsErrProjectCannotBelongToAPseudoParentProject(err)) }) t.Run("attacker with direct Write on victim project cannot reparent it (GHSA-2vq4-854f-5c72)", func(t *testing.T) { - // User 1 has direct Write (permission=1) on project 10 (owner=6) via - // users_projects id=4. User 1 also owns project 1 (a root project). - // Under the pre-fix code, user 1 could POST /projects/10 with - // parent_project_id=1 and CanUpdate would return true because both - // CanWrite checks passed; the CTE would then cascade Admin on 10 - // via ownership of the new parent. This test locks that down. + // User 1 has direct Write on project 10 (owner=6) via + // users_projects id=4 and owns root project 1. Pre-fix, a + // reparent of 10 under 1 passed the CanWrite check and the + // CTE then cascaded Admin on 10 via ownership of the new + // parent. db.LoadAndAssertFixtures(t) s := db.NewSession() defer s.Close() @@ -302,9 +301,8 @@ func TestProject_CreateOrUpdate(t *testing.T) { Title: "Test10", ParentProjectID: 1, // attacker-owned root } - can, err := project.CanUpdate(s, usr) + err := project.Update(s, usr) require.Error(t, err) - assert.False(t, can) assert.True(t, IsErrGenericForbidden(err)) }) t.Run("attacker with inherited Write cannot reparent child to attacker root (GHSA-2vq4-854f-5c72)", func(t *testing.T) { @@ -319,62 +317,28 @@ func TestProject_CreateOrUpdate(t *testing.T) { Title: "Reparent Escalation Test Child", ParentProjectID: 1, } - can, err := project.CanUpdate(s, usr) + err := project.Update(s, usr) require.Error(t, err) - assert.False(t, can) assert.True(t, IsErrGenericForbidden(err)) }) - t.Run("attacker with Write cannot detach project to root (GHSA-2vq4-854f-5c72)", func(t *testing.T) { - // Detach-to-root path: posting parent_project_id=0 used to bypass - // the reparent check entirely. User 1 has Write on project 43's - // parent chain but no Admin — detach must be rejected. + t.Run("non-reparent update with Write still permitted (regression)", func(t *testing.T) { + // User 1 has Write (not Admin) on project 43 via project 10; + // a rename with parent unchanged must not trip the Admin gate. + // + // ParentProjectID is set to the stored value (10), not 0: the + // gate only fires on non-zero ParentProjectID because the + // generic handler can't distinguish omitted from explicit-zero + // (detach-to-root is a follow-up, needs a pointer field). db.LoadAndAssertFixtures(t) s := db.NewSession() defer s.Close() project := Project{ ID: 43, - Title: "Reparent Escalation Test Child", - ParentProjectID: 0, // detach + Title: "Reparent Escalation Test Child renamed", + ParentProjectID: 10, // unchanged — no reparent intent } - can, err := project.CanUpdate(s, usr) - require.Error(t, err) - assert.False(t, can) - assert.True(t, IsErrGenericForbidden(err)) - }) - t.Run("non-reparent update with Write still permitted (regression)", func(t *testing.T) { - // User 1 has Write on project 10. Renaming must still be permitted - // when parent_project_id is unchanged (sent as the current DB value). - // Project 10 has no parent in the fixture, so sending 0 matches. - db.LoadAndAssertFixtures(t) - s := db.NewSession() - defer s.Close() - project := Project{ - ID: 10, - Title: "Test10 renamed", - ParentProjectID: 0, // project 10 currently has no parent - } - can, err := project.CanUpdate(s, usr) + err := project.Update(s, usr) require.NoError(t, err) - assert.True(t, can) - }) - t.Run("owner can detach their own project to root", func(t *testing.T) { - // User 6 owns project 12 (child of 27). Detach must still work. - usr6 := &user.User{ - ID: 6, - Username: "user6", - Email: "user6@example.com", - } - db.LoadAndAssertFixtures(t) - s := db.NewSession() - defer s.Close() - project := Project{ - ID: 12, - Title: "Test12", - ParentProjectID: 0, - } - can, err := project.CanUpdate(s, usr6) - require.NoError(t, err) - assert.True(t, can) }) }) t.Run("archive default project of the same user", func(t *testing.T) { diff --git a/pkg/webtests/project_test.go b/pkg/webtests/project_test.go index 4413ac13b..dca9bf81e 100644 --- a/pkg/webtests/project_test.go +++ b/pkg/webtests/project_test.go @@ -61,10 +61,13 @@ func TestProject(t *testing.T) { // ParadeDB fuzzy(1, prefix=true) on "Test1" matches Test2-Test9 // (edit distance 1), Test10+ (prefix), etc. The recursive CTE // also pulls in child projects of matched parents. - require.Len(t, projects, 26) + // +1 for the reparent-escalation fixture child (project 43). + require.Len(t, projects, 27) } else { - // ILIKE '%Test1%' matches Test1, Test10, Test11, Test19, + favorites - require.Len(t, projects, 5) + // ILIKE '%Test1%' matches Test1, Test10, Test11, Test19, + favorites. + // The recursive CTE also pulls in project 43 as a child of the + // matched project 10 (reparent-escalation fixture). + require.Len(t, projects, 6) assert.NotContains(t, rec.Body.String(), `Test2"`) assert.NotContains(t, rec.Body.String(), `Test3`) assert.NotContains(t, rec.Body.String(), `Test4`)