diff --git a/cmd/setup/73.go b/cmd/setup/73.go new file mode 100644 index 000000000..bc367841d --- /dev/null +++ b/cmd/setup/73.go @@ -0,0 +1,134 @@ +package setup + +import ( + "context" + _ "embed" + "fmt" + + "github.com/zitadel/zitadel/backend/v3/instrumentation/logging" + "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/database" + "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/eventstore/handler/v2" + "github.com/zitadel/zitadel/internal/query/projection" + "github.com/zitadel/zitadel/internal/repository/instance" + "github.com/zitadel/zitadel/internal/repository/usergrant" +) + +var ( + //go:embed 73.sql + fixUserGrantRoles string +) + +// FixUserGrantRoles repairs grant-based user grants that kept roles which +// should have been cascade-removed (GHSA-v859-c572-qh5p). Only grant-based +// user grants (ug.grant_id set) are in scope: the bug can only manifest via +// ChangeProjectGrant's multi-role cascade to those grants (see 73.sql for +// details), so direct user grants are intentionally left untouched even if +// their roles also happen to mismatch, since that mismatch would be unrelated +// pre-existing data drift, not this CVE. The corruption lives in the +// eventstore event payloads, so we fix the source of truth by pushing a +// corrective user.grant.cascade.changed event with the reconciled role set, +// then let the projection catch up. +type FixUserGrantRoles struct { + eventstore *eventstore.Eventstore +} + +func (mig *FixUserGrantRoles) Execute(ctx context.Context, _ eventstore.Event) error { + instances, err := mig.eventstore.InstanceIDs( + ctx, + eventstore.NewSearchQueryBuilder(eventstore.ColumnsInstanceIDs). + OrderDesc(). + AddQuery(). + AggregateTypes(instance.AggregateType). + EventTypes(instance.InstanceAddedEventType). + Builder().ExcludeAggregateIDs(). + AggregateTypes(instance.AggregateType). + EventTypes(instance.InstanceRemovedEventType). + Builder(), + ) + if err != nil { + return err + } + + ctx = authz.SetCtxData(ctx, authz.CtxData{UserID: "SETUP"}) + for i, instanceID := range instances { + ctx = authz.WithInstanceID(ctx, instanceID) + logging.Info(ctx, "fix user grant roles", "instance", instanceID, "migration", mig.String(), "progress", fmt.Sprintf("%d/%d", i+1, len(instances))) + + // The finder reads projection tables, which may not yet be populated during + // setup. Bring the sources up to date for this instance before querying. + for _, source := range []*handler.Handler{ + projection.ProjectGrantProjection, + projection.UserGrantProjection, + } { + if _, err = source.Trigger(ctx); err != nil { + return err + } + } + + fixedCount, err := mig.fixInstanceUserGrants(ctx, instanceID) + if err != nil { + return err + } + if fixedCount == 0 { + continue + } + logging.Info(ctx, "fixed user grant roles", "instance", instanceID, "migration", mig.String(), "grantsFixed", fixedCount) + // Reflect the corrective events we just pushed in the read model. + _, err = projection.UserGrantProjection.Trigger(ctx) + logging.OnError(ctx, err).Debug("failed triggering user grant projection to update roles") + } + return nil +} + +func (mig *FixUserGrantRoles) fixInstanceUserGrants(ctx context.Context, instanceID string) (fixedCount int, err error) { + var correctedGrants []eventstore.Command + + tx, err := mig.eventstore.Client().BeginTx(ctx, nil) + if err != nil { + return 0, err + } + defer func() { + if err != nil { + _ = tx.Rollback() + return + } + err = tx.Commit() + }() + + rows, err := tx.QueryContext(ctx, fixUserGrantRoles, instanceID) + if err != nil { + return 0, err + } + defer rows.Close() + + for rows.Next() { + var ( + id string + resourceOwner string + correctRoles = database.TextArray[string]{} + ) + if err := rows.Scan(&id, &resourceOwner, &correctRoles); err != nil { + return 0, err + } + aggregate := &eventstore.Aggregate{ + ID: id, + Type: usergrant.AggregateType, + Version: usergrant.AggregateVersion, + ResourceOwner: resourceOwner, + InstanceID: instanceID, + } + correctedGrants = append(correctedGrants, usergrant.NewUserGrantCascadeChangedEvent(ctx, aggregate, correctRoles)) + } + if rows.Err() != nil { + return 0, rows.Err() + } + + _, err = mig.eventstore.PushWithClient(ctx, tx, correctedGrants...) + return len(correctedGrants), err +} + +func (*FixUserGrantRoles) String() string { + return "73_fix_user_grant_roles" +} diff --git a/cmd/setup/73.sql b/cmd/setup/73.sql new file mode 100644 index 000000000..f6a76df14 --- /dev/null +++ b/cmd/setup/73.sql @@ -0,0 +1,39 @@ +-- Finds user grants whose stored roles include roles that are no longer valid +-- (the residue left by the buggy cascade removal, GHSA-v859-c572-qh5p), and +-- returns the corrected role set for each. $1 = instance_id, run once per instance. +-- +-- Scope: only grant-based user grants (ug.grant_id set) can be affected by this +-- bug. It was introduced by ChangeProjectGrant (internal/command/project_grant.go), +-- which cascades a *multi*-role removal to the user grants tied to that project +-- grant. Direct user grants never go through a multi-role removal call, so any +-- role mismatch there is unrelated to this CVE (e.g. pre-existing role_key drift) +-- and must not be "fixed" here to avoid stripping unrelated, legitimate roles. +WITH computed AS ( + SELECT + ug.id, + ug.resource_owner, + ug.roles AS current_roles, + -- Recompute the roles this grant is actually allowed to keep by filtering + -- its current roles down to the ones still granted by the project grant. + COALESCE( + ARRAY( + SELECT r + FROM unnest(ug.roles) AS r -- expand the stored roles array + WHERE r = ANY(pg.granted_role_keys) + ), + ARRAY[]::TEXT[] -- keep NULL out; use empty array + ) AS valid_roles + FROM projections.user_grants5 ug + JOIN projections.project_grants4 pg + ON pg.instance_id = ug.instance_id + AND pg.grant_id = ug.grant_id + WHERE ug.instance_id = $1 + AND ug.grant_id IS NOT NULL AND ug.grant_id <> '' -- direct grants are out of scope, see above + AND ug.roles IS NOT NULL + AND cardinality(ug.roles) > 0 -- nothing to correct on empty grants +) +-- valid_roles is always a subset of current_roles, so a size mismatch means at +-- least one stale role was dropped -> this grant needs a corrective event. +SELECT id, resource_owner, valid_roles +FROM computed +WHERE cardinality(valid_roles) <> cardinality(current_roles); diff --git a/cmd/setup/config.go b/cmd/setup/config.go index fd01c03c2..cd8f117c5 100644 --- a/cmd/setup/config.go +++ b/cmd/setup/config.go @@ -192,6 +192,7 @@ type Steps struct { s70AddEventStoreCommandEnforceOwner *AddEventStoreCommandEnforceOwnerColumn s71JWTProvideAddAudienceColumn *JWTProvideAddAudienceColumn s72AddColumnsToLoginNamesView *AddColumnsToLoginNamesView + s73FixUserGrantRoles *FixUserGrantRoles RelationalTables *TransactionalTables } diff --git a/cmd/setup/setup.go b/cmd/setup/setup.go index c031df36b..3e93eb201 100644 --- a/cmd/setup/setup.go +++ b/cmd/setup/setup.go @@ -255,6 +255,7 @@ func Setup(ctx context.Context, config *Config, steps *Steps, masterKey string) steps.s70AddEventStoreCommandEnforceOwner = &AddEventStoreCommandEnforceOwnerColumn{dbClient: dbClient} steps.s71JWTProvideAddAudienceColumn = &JWTProvideAddAudienceColumn{dbClient: dbClient} steps.s72AddColumnsToLoginNamesView = &AddColumnsToLoginNamesView{dbClient: dbClient} + steps.s73FixUserGrantRoles = &FixUserGrantRoles{eventstore: eventstoreClient} err = projection.Create(ctx, dbClient, eventstoreClient, config.Projections, nil, nil, nil) if err != nil { @@ -380,6 +381,7 @@ func Setup(ctx context.Context, config *Config, steps *Steps, masterKey string) steps.s68TargetAddPayloadTypeColumn, steps.s71JWTProvideAddAudienceColumn, steps.s72AddColumnsToLoginNamesView, + steps.s73FixUserGrantRoles, } { setupErr = executeMigration(ctx, eventstoreClient, step, "migration failed") if setupErr != nil { diff --git a/internal/api/grpc/management/integration_test/user_grant_test.go b/internal/api/grpc/management/integration_test/user_grant_test.go new file mode 100644 index 000000000..f0045bab7 --- /dev/null +++ b/internal/api/grpc/management/integration_test/user_grant_test.go @@ -0,0 +1,117 @@ +//go:build integration + +package management_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/zitadel/zitadel/internal/integration" + mgmt_pb "github.com/zitadel/zitadel/pkg/grpc/management" +) + +// PoC for GHSA-v859-c572-qh5p +// +// Updating a project grant to drop roles cascades into the user grants of that +// grant via removeRoleFromUserGrant (internal/command/user_grant.go), which +// removes the dropped roles in a single call. Because that loop mutates the +// slice while iterating forward, two *adjacent* removed roles cause the second to be +// skipped: the user keeps authorisation that should have been revoked. +// +// Here a user grant holds ["admin","viewer","editor"]; the project grant is +// updated to keep only ["editor"], so "admin" and "viewer" should both be +// stripped. The test asserts the CORRECT result (["editor"]); against faulty +// implementations the grant ends up ["viewer","editor"], so it fails. +func TestServer_UpdateProjectGrant_cascadeRoleRemovalSkipsAdjacent(t *testing.T) { + const ( + roleAdmin = "admin" + roleViewer = "viewer" + roleEditor = "editor" + ) + + // Org that the project will be granted to. + grantedOrg := Instance.CreateOrganization(IAMOwnerCTX, integration.OrganizationName(), integration.Email()) + grantedOrgID := grantedOrg.GetOrganizationId() + grantedOrgCTX := integration.SetOrgID(IAMOwnerCTX, grantedOrgID) + + // Project (owned by the default org) with three roles, in order. + projectResp, err := Client.AddProject(OrgCTX, &mgmt_pb.AddProjectRequest{ + Name: integration.ProjectName(), + }) + require.NoError(t, err) + projectID := projectResp.GetId() + + for _, role := range []string{roleAdmin, roleViewer, roleEditor} { + _, err = Client.AddProjectRole(OrgCTX, &mgmt_pb.AddProjectRoleRequest{ + ProjectId: projectID, + RoleKey: role, + DisplayName: role, + }) + require.NoError(t, err) + } + + // Grant the project (with all three roles) to the granted org. + grantResp, err := Client.AddProjectGrant(OrgCTX, &mgmt_pb.AddProjectGrantRequest{ + ProjectId: projectID, + GrantedOrgId: grantedOrgID, + RoleKeys: []string{roleAdmin, roleViewer, roleEditor}, + }) + require.NoError(t, err) + grantID := grantResp.GetGrantId() + + // A user in the granted org, holding all three roles on the project grant. + userResp := Instance.CreateHumanUserVerified(IAMOwnerCTX, grantedOrgID, integration.Email(), "+41791234567") + userID := userResp.GetUserId() + + userGrantResp, err := Client.AddUserGrant(grantedOrgCTX, &mgmt_pb.AddUserGrantRequest{ + UserId: userID, + ProjectId: projectID, + ProjectGrantId: grantID, + RoleKeys: []string{roleAdmin, roleViewer, roleEditor}, + }) + require.NoError(t, err) + userGrantID := userGrantResp.GetUserGrantId() + + // UpdateProjectGrant only cascades to user grants the query side already + // returns, so wait for the projection to catch up to all three roles. + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(CTX, time.Minute) + require.EventuallyWithT(t, func(ct *assert.CollectT) { + grant, err := Client.GetUserGrantByID(grantedOrgCTX, &mgmt_pb.GetUserGrantByIDRequest{ + UserId: userID, + GrantId: userGrantID, + }) + assert.NoError(ct, err) + assert.ElementsMatch(ct, []string{roleAdmin, roleViewer, roleEditor}, grant.GetUserGrant().GetRoleKeys()) + }, retryDuration, tick) + + // Drop "admin" and "viewer" from the project grant, keeping only "editor". + // This cascades removeRoleFromUserGrant(["admin","viewer"]) into the user grant. + _, err = Client.UpdateProjectGrant(OrgCTX, &mgmt_pb.UpdateProjectGrantRequest{ + ProjectId: projectID, + GrantId: grantID, + RoleKeys: []string{roleEditor}, + }) + require.NoError(t, err) + + // Wait until the cascade has been applied and projected. "admin" (the first + // removed role) is dropped by both the buggy and fixed code, so its absence + // is a stable signal that the cascade has converged. + retryDuration, tick = integration.WaitForAndTickWithMaxDuration(CTX, time.Minute) + var finalRoleKeys []string + require.EventuallyWithT(t, func(ct *assert.CollectT) { + grant, err := Client.GetUserGrantByID(grantedOrgCTX, &mgmt_pb.GetUserGrantByIDRequest{ + UserId: userID, + GrantId: userGrantID, + }) + assert.NoError(ct, err) + finalRoleKeys = grant.GetUserGrant().GetRoleKeys() + assert.NotContains(ct, finalRoleKeys, roleAdmin) + }, retryDuration, tick) + + // Correct expectation: only "editor" survives. With the off-by-one bug the + // user grant is ["viewer","editor"] — "viewer" was skipped during removal. + assert.ElementsMatch(t, []string{roleEditor}, finalRoleKeys) +} diff --git a/internal/command/project_role.go b/internal/command/project_role.go index efee594a3..200321ac4 100644 --- a/internal/command/project_role.go +++ b/internal/command/project_role.go @@ -193,7 +193,7 @@ func (c *Commands) RemoveProjectRole(ctx context.Context, projectID, key, resour } for _, grantID := range cascadeUserGrantIDs { - event, err := c.removeRoleFromUserGrant(ctx, grantID, []string{key}, true) + event, err := c.removeRoleFromUserGrant(ctx, grantID, map[string]bool{key:true}, true) if err != nil { logging.LogWithFields("COMMAND-mK0of", "usergrantid", grantID).WithError(err).Warn("could not cascade remove role on user grant") continue diff --git a/internal/command/user_grant.go b/internal/command/user_grant.go index f6e918d26..a4703a237 100644 --- a/internal/command/user_grant.go +++ b/internal/command/user_grant.go @@ -111,7 +111,7 @@ func (c *Commands) ChangeUserGrant(ctx context.Context, userGrant *domain.UserGr return userGrantWriteModelToUserGrant(existingUserGrant), nil } -func (c *Commands) removeRoleFromUserGrant(ctx context.Context, userGrantID string, roleKeys []string, cascade bool) (_ eventstore.Command, err error) { +func (c *Commands) removeRoleFromUserGrant(ctx context.Context, userGrantID string, roleKeysToRemove map[string]bool, cascade bool) (_ eventstore.Command, err error) { existingUserGrant, err := c.userGrantWriteModelByID(ctx, userGrantID, "") if err != nil { return nil, err @@ -119,21 +119,16 @@ func (c *Commands) removeRoleFromUserGrant(ctx context.Context, userGrantID stri if existingUserGrant.State == domain.UserGrantStateUnspecified || existingUserGrant.State == domain.UserGrantStateRemoved { return nil, zerrors.ThrowNotFound(nil, "COMMAND-3M9sd", "Errors.UserGrant.NotFound") } - keyExists := false - for i, key := range existingUserGrant.RoleKeys { - for _, roleKey := range roleKeys { - if key == roleKey { - keyExists = true - copy(existingUserGrant.RoleKeys[i:], existingUserGrant.RoleKeys[i+1:]) - existingUserGrant.RoleKeys[len(existingUserGrant.RoleKeys)-1] = "" - existingUserGrant.RoleKeys = existingUserGrant.RoleKeys[:len(existingUserGrant.RoleKeys)-1] - continue - } - } - } - if !keyExists { + + beforeLen := len(existingUserGrant.RoleKeys) + existingUserGrant.RoleKeys = slices.DeleteFunc(existingUserGrant.RoleKeys, func(role string) bool { + return roleKeysToRemove[role] + }) + + if beforeLen == len(existingUserGrant.RoleKeys) { return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-5m8g9", "Errors.UserGrant.RoleKeyNotFound") } + changedUserGrant := NewUserGrantWriteModel(userGrantID, existingUserGrant.ResourceOwner) userGrantAgg := UserGrantAggregateFromWriteModel(&changedUserGrant.WriteModel) diff --git a/internal/command/user_grant_test.go b/internal/command/user_grant_test.go index 3662a6ea5..faae23f36 100644 --- a/internal/command/user_grant_test.go +++ b/internal/command/user_grant_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/text/language" "github.com/zitadel/zitadel/internal/api/authz" @@ -3209,3 +3210,74 @@ func TestCommandSide_BulkRemoveUserGrant(t *testing.T) { }) } } + +// PoC for GHSA-v859-c572-qh5p +// Each case asserts the CORRECT post-removal RoleKeys; +// against faulty implementations these assertions fail. +func TestCommandSide_removeRoleFromUserGrant(t *testing.T) { + t.Parallel() + tests := []struct { + name string + existingRoleKeys []string + roleKeysToRemove map[string]bool + want []string + }{ + { + name: "advisory scenario, remove two adjacent leading roles", + existingRoleKeys: []string{"admin", "viewer", "editor"}, + roleKeysToRemove: map[string]bool{"admin":true, "viewer":true}, + want: []string{"editor"}, + }, + { + name: "remove first two of four", + existingRoleKeys: []string{"a", "b", "c", "d"}, + roleKeysToRemove: map[string]bool{"a":true, "b":true}, + want: []string{"c", "d"}, + }, + { + name: "remove two adjacent middle roles", + existingRoleKeys: []string{"a", "b", "c", "d"}, + roleKeysToRemove: map[string]bool{"b":true, "c":true}, + want: []string{"a", "d"}, + }, + { + name: "remove two non-adjacent roles", + existingRoleKeys: []string{"a", "b", "c", "d"}, + roleKeysToRemove: map[string]bool{"a":true, "c":true}, + want: []string{"b", "d"}, + }, + { + name: "remove all roles", + existingRoleKeys: []string{"x", "y", "z"}, + roleKeysToRemove: map[string]bool{"x":true, "y":true, "z":true}, + want: []string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + r := &Commands{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + usergrant.NewUserGrantAddedEvent(t.Context(), + &usergrant.NewAggregate("usergrant1", "org1").Aggregate, + "user1", + "project1", + "", + tt.existingRoleKeys, + ), + ), + ), + )(t), + } + + cmd, err := r.removeRoleFromUserGrant(t.Context(), "usergrant1", tt.roleKeysToRemove, true) + require.NoError(t, err) + + event, ok := cmd.(*usergrant.UserGrantCascadeChangedEvent) + require.True(t, ok, "expected *usergrant.UserGrantCascadeChangedEvent, got %T", cmd) + assert.Equal(t, tt.want, event.RoleKeys) + }) + } +} diff --git a/internal/domain/project_grant.go b/internal/domain/project_grant.go index 6376c9ed7..e99cfc1ef 100644 --- a/internal/domain/project_grant.go +++ b/internal/domain/project_grant.go @@ -1,6 +1,10 @@ package domain -import es_models "github.com/zitadel/zitadel/internal/eventstore/v1/models" +import ( + "slices" + + es_models "github.com/zitadel/zitadel/internal/eventstore/v1/models" +) type ProjectGrant struct { es_models.ObjectRoot @@ -34,11 +38,11 @@ func (p *ProjectGrant) IsValid() bool { return p.GrantedOrgID != "" } -func GetRemovedRoles(existingRoles, newRoles []string) []string { - removed := make([]string, 0) +func GetRemovedRoles(existingRoles, newRoles []string) map[string]bool { + removed := make(map[string]bool, 0) for _, role := range existingRoles { - if !containsRoleKey(role, newRoles) { - removed = append(removed, role) + if !slices.Contains(newRoles, role) { + removed[role] = true } } return removed diff --git a/internal/domain/project_role.go b/internal/domain/project_role.go index c3593a151..bd08b0c1c 100644 --- a/internal/domain/project_role.go +++ b/internal/domain/project_role.go @@ -1,6 +1,8 @@ package domain import ( + "slices" + "github.com/zitadel/zitadel/internal/eventstore/v1/models" ) @@ -30,18 +32,9 @@ func (p *ProjectRole) IsValid() bool { func HasInvalidRoles(validRoles, roles []string) bool { for _, roleKey := range roles { - if !containsRoleKey(roleKey, validRoles) { + if !slices.Contains(validRoles, roleKey) { return true } } return false -} - -func containsRoleKey(roleKey string, validRoles []string) bool { - for _, validRole := range validRoles { - if roleKey == validRole { - return true - } - } - return false -} +} \ No newline at end of file diff --git a/internal/domain/user_grant.go b/internal/domain/user_grant.go index 599b07768..debe0217c 100644 --- a/internal/domain/user_grant.go +++ b/internal/domain/user_grant.go @@ -1,6 +1,10 @@ package domain -import es_models "github.com/zitadel/zitadel/internal/eventstore/v1/models" +import ( + "slices" + + es_models "github.com/zitadel/zitadel/internal/eventstore/v1/models" +) type UserGrant struct { es_models.ObjectRoot @@ -27,7 +31,7 @@ func (u *UserGrant) IsValid() bool { func (g *UserGrant) HasInvalidRoles(validRoles []string) bool { for _, roleKey := range g.RoleKeys { - if !containsRoleKey(roleKey, validRoles) { + if !slices.Contains(validRoles, roleKey) { return true } }