diff --git a/pkg/db/fixtures/label_tasks.yml b/pkg/db/fixtures/label_tasks.yml index 83f9524d1..96c8d1d89 100644 --- a/pkg/db/fixtures/label_tasks.yml +++ b/pkg/db/fixtures/label_tasks.yml @@ -22,10 +22,14 @@ task_id: 35 label_id: 5 created: 2018-12-01 15:13:12 -# Attaches label #6 to task #34 (project 20, owned by user 13 and not shared -# with user 1). Regression fixture for GHSA-hj5c-mhh2-g7jq — without this -# row the label cannot reproduce the precedence leak. +# GHSA-hj5c-mhh2-g7jq: without this row label #6 cannot reproduce the leak. - id: 7 task_id: 34 label_id: 6 created: 2018-12-01 15:13:12 +# Label #8 attached to an inaccessible task, for the creator-branch +# maxPermission fallback. +- id: 8 + task_id: 34 + label_id: 8 + created: 2018-12-01 15:13:12 diff --git a/pkg/db/fixtures/labels.yml b/pkg/db/fixtures/labels.yml index 8403ea30e..d685d6392 100644 --- a/pkg/db/fixtures/labels.yml +++ b/pkg/db/fixtures/labels.yml @@ -23,18 +23,23 @@ created_by_id: 2 updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 -# Label #6 is owned by user 13 and attached only to task #34, which lives in -# project 20 (owned by user 13 and not shared with anyone). User 1 must NOT -# be able to read this label. Regression fixture for GHSA-hj5c-mhh2-g7jq. +# Regression for GHSA-hj5c-mhh2-g7jq: only attached to task #34 +# (project 20, user 13 only), must be unreachable for everyone else. - id: 6 title: 'Label #6 - private, attached only to private task' created_by_id: 13 updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 -# Label #7 is owned by user 1 but has no label_tasks row. Protects the -# "creator can still see their own unattached labels" path. +# Covers the creator-can-read-own-unattached-label path. - id: 7 title: 'Label #7 - created by user 1, no task attachment' created_by_id: 1 updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 +# Covers the creator-branch maxPermission fallback: user 1's label whose +# only label_tasks row points at task #34 (inaccessible to user 1). +- id: 8 + title: 'Label #8 - user 1 creator, only attached to inaccessible task' + created_by_id: 1 + updated: 2018-12-02 15:13:12 + created: 2018-12-01 15:13:12 diff --git a/pkg/models/label_permissions.go b/pkg/models/label_permissions.go index e095930c3..1c5df95c1 100644 --- a/pkg/models/label_permissions.go +++ b/pkg/models/label_permissions.go @@ -60,23 +60,17 @@ func (l *Label) isLabelOwner(s *xorm.Session, a web.Auth) (bool, error) { return lorig.CreatedByID == a.GetID(), nil } -// Helper method to check if a user can see a specific label. +// hasAccessToLabel reports whether the caller can read a label and, if so, +// the caller's maximum permission on it. // -// A user can read a label when at least one of the following is true: -// -// 1. The auth is a real user and the user created the label, OR -// 2. The label is attached to a task in a project the auth can access. -// -// The implementation uses explicit builder.And / builder.Or grouping so -// the boolean precedence is unambiguous. A previous implementation chained -// xorm session .Where / .Or / .And calls which SQL flattened to -// `WHERE A OR B OR C AND D`, leaking any label with any label_tasks row +// The access cond is assembled with explicit builder.And / builder.Or. +// Chaining xorm's session .Where/.Or/.And instead flattens the SQL to +// `A OR B OR C AND D`, which leaked any label with any label_tasks row // to any authenticated user (GHSA-hj5c-mhh2-g7jq). func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPermission int, err error) { linkShare, isLinkShare := a.(*LinkSharing) - // Build the "task is in a project the caller can access" subquery. var accessibleProjects builder.Cond if isLinkShare { accessibleProjects = builder.Eq{"project_id": linkShare.ProjectID} @@ -95,11 +89,6 @@ func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPerm Where(accessibleProjects), ) - // A user can see a label if: - // - they created it (only when the auth is an actual user), OR - // - it is attached to a task in a project they have access to. - // - // The outer AND enforces that the result is scoped to the requested label ID. accessBranches := []builder.Cond{labelAttachedToAccessibleTask} if !isLinkShare { accessBranches = append(accessBranches, builder.Eq{"labels.created_by_id": a.GetID()}) @@ -110,28 +99,48 @@ func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPerm builder.Or(accessBranches...), ) - ll := &LabelTask{} has, err = s.Table("labels"). - Select("label_tasks.*"). Join("LEFT", "label_tasks", "label_tasks.label_id = labels.id"). Where(cond). - Get(ll) + Exist(&Label{}) if err != nil || !has { return } - // If the label was matched via an attached task, compute the caller's - // permission level from that task. Otherwise (creator-only branch with - // no attachment) default to read permission. - if ll.TaskID > 0 { - t := &Task{ID: ll.TaskID} - _, maxPermission, err = t.CanRead(s, a) - if err != nil { - return - } + // maxPermission is derived only from label_tasks rows whose task is + // actually accessible. The pre-fix code used Get(ll) against the + // unrestricted LEFT JOIN, so it could return an inaccessible row and + // yield a wrong (or errored) permission. + accessibleTaskIDs := []int64{} + err = s.Table("label_tasks"). + Join("INNER", "tasks", "tasks.id = label_tasks.task_id"). + Where(builder.And( + builder.Eq{"label_tasks.label_id": l.ID}, + accessibleProjects, + )). + Cols("label_tasks.task_id"). + Find(&accessibleTaskIDs) + if err != nil { return } - maxPermission = int(PermissionRead) + for _, taskID := range accessibleTaskIDs { + t := &Task{ID: taskID} + _, taskPermission, tErr := t.CanRead(s, a) + if tErr != nil { + err = tErr + return + } + if taskPermission > maxPermission { + maxPermission = taskPermission + } + } + + // Creator-branch fallback: access came from created_by_id with no + // accessible task to derive a permission from. + if len(accessibleTaskIDs) == 0 { + maxPermission = int(PermissionRead) + } + return } diff --git a/pkg/models/label_test.go b/pkg/models/label_test.go index d4b89324b..aa73ae647 100644 --- a/pkg/models/label_test.go +++ b/pkg/models/label_test.go @@ -125,6 +125,16 @@ func TestLabel_ReadAll(t *testing.T) { Updated: testUpdatedTime, }, }, + { + Label: Label{ + ID: 8, + Title: "Label #8 - user 1 creator, only attached to inaccessible task", + CreatedByID: 1, + CreatedBy: user1, + Created: testCreatedTime, + Updated: testUpdatedTime, + }, + }, }, }, { @@ -193,13 +203,15 @@ func TestLabel_ReadOne(t *testing.T) { ExportFileID: 1, } tests := []struct { - name string - fields fields - want *Label - wantErr bool - errType func(error) bool - auth web.Auth - wantForbidden bool + name string + fields fields + want *Label + wantErr bool + errType func(error) bool + auth web.Auth + wantForbidden bool + assertMaxPermission bool + wantMaxPermission int }{ { name: "Get label #1", @@ -214,7 +226,9 @@ func TestLabel_ReadOne(t *testing.T) { Created: testCreatedTime, Updated: testUpdatedTime, }, - auth: &user.User{ID: 1}, + auth: &user.User{ID: 1}, + assertMaxPermission: true, + wantMaxPermission: int(PermissionRead), }, { name: "Get nonexistant label", @@ -235,6 +249,8 @@ func TestLabel_ReadOne(t *testing.T) { auth: &user.User{ID: 1}, }, { + // Label 4 is attached to tasks in project 1 (user 1 is admin), + // so the accessible-tasks iteration must yield PermissionAdmin. name: "Get label #4 - other user", fields: fields{ ID: 4, @@ -258,13 +274,13 @@ func TestLabel_ReadOne(t *testing.T) { Created: testCreatedTime, Updated: testUpdatedTime, }, - auth: &user.User{ID: 1}, + auth: &user.User{ID: 1}, + assertMaxPermission: true, + wantMaxPermission: int(PermissionAdmin), }, { - // Regression for GHSA-hj5c-mhh2-g7jq: label 6 is owned by user 13 - // and attached only to task 34 (project 20, not shared with user 1). - // The old hasAccessToLabel query leaked any label with any - // label_tasks row to any authenticated user. + // PoC for GHSA-hj5c-mhh2-g7jq: label 6 is reachable only via task + // 34 in the private project 20, user 1 must not see it. name: "PoC GHSA-hj5c-mhh2-g7jq: label 6 attached only to unreachable task must be forbidden", fields: fields{ ID: 6, @@ -286,7 +302,29 @@ func TestLabel_ReadOne(t *testing.T) { Created: testCreatedTime, Updated: testUpdatedTime, }, - auth: &user.User{ID: 1}, + auth: &user.User{ID: 1}, + assertMaxPermission: true, + wantMaxPermission: int(PermissionRead), + }, + { + // Label 8's only label_tasks row points at inaccessible task 34, + // so access must come from the creator branch and the + // maxPermission fallback to PermissionRead must kick in. + name: "creator can read own label only attached to inaccessible task", + fields: fields{ + ID: 8, + }, + want: &Label{ + ID: 8, + Title: "Label #8 - user 1 creator, only attached to inaccessible task", + CreatedByID: 1, + CreatedBy: user1, + Created: testCreatedTime, + Updated: testUpdatedTime, + }, + auth: &user.User{ID: 1}, + assertMaxPermission: true, + wantMaxPermission: int(PermissionRead), }, { // Non-creator must not be able to read an unattached label owned @@ -318,13 +356,16 @@ func TestLabel_ReadOne(t *testing.T) { s := db.NewSession() defer s.Close() - allowed, _, _ := l.CanRead(s, tt.auth) + allowed, maxPermission, _ := l.CanRead(s, tt.auth) if !allowed && !tt.wantForbidden { t.Errorf("Label.CanRead() forbidden, want %v", tt.wantForbidden) } if allowed && tt.wantForbidden { t.Errorf("Label.CanRead() allowed, want forbidden") } + if tt.assertMaxPermission && maxPermission != tt.wantMaxPermission { + t.Errorf("Label.CanRead() maxPermission = %d, want %d", maxPermission, tt.wantMaxPermission) + } err := l.ReadOne(s, tt.auth) if (err != nil) != tt.wantErr { t.Errorf("Label.ReadOne() error = %v, wantErr %v", err, tt.wantErr)