kodev-org
/
vikunja
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(security): move reparent Admin gate into UpdateProject 

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).
This commit is contained in:
kolaente 2026-04-09 15:17:39 +02:00 committed by kolaente
parent b6dc0096af
commit 75e1f72c6e
4 changed files with 64 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
pkg/models/project.go
View File

@ -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 {

6
pkg/models/project_permissions.go
View File

@ -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)

72
pkg/models/project_test.go
View File

@ -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) {

9
pkg/webtests/project_test.go
View File

@ -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`)