diff --git a/changes/16773-query-pack-metadata-leak b/changes/16773-query-pack-metadata-leak new file mode 100644 index 0000000000..9800aa05be --- /dev/null +++ b/changes/16773-query-pack-metadata-leak @@ -0,0 +1 @@ +- Fixed query (report) responses so that pack metadata (ID, name, description) is only included when the requesting user is authorized to read packs, preventing cross-fleet pack metadata disclosure via query name collisions. diff --git a/frontend/components/forms/fields/SelectTargetsDropdown/SelectTargetsMenu/SelectTargetsMenu.jsx b/frontend/components/forms/fields/SelectTargetsDropdown/SelectTargetsMenu/SelectTargetsMenu.jsx index 70c13508c0..51ece82b5b 100644 --- a/frontend/components/forms/fields/SelectTargetsDropdown/SelectTargetsMenu/SelectTargetsMenu.jsx +++ b/frontend/components/forms/fields/SelectTargetsDropdown/SelectTargetsMenu/SelectTargetsMenu.jsx @@ -34,7 +34,12 @@ const SelectTargetsMenuWrapper = ( const renderTargets = (targetType) => { const targets = filter(options, targetFilter(targetType)); const targetsOutput = []; - const targetTitle = targetType === "all" ? "all hosts" : targetType; + let targetTitle = targetType; + if (targetType === "all") { + targetTitle = "all hosts"; + } else if (targetType === "teams") { + targetTitle = "fleets"; + } targetsOutput.push(
@@ -52,7 +57,7 @@ const SelectTargetsMenuWrapper = ( className={`${baseClass}__not-found`} key={`${targetType}-notfound`} > - Unable to find any matching {targetType}. + Unable to find any matching {targetTitle}. ); @@ -108,7 +113,7 @@ const SelectTargetsMenuWrapper = ( const renderTargetGroups = ( <> {renderTargets("all")} - {isPremiumTier && renderTargets("fleets")} + {isPremiumTier && renderTargets("teams")} {renderTargets("labels")} {renderTargets("hosts")} > diff --git a/server/service/queries.go b/server/service/queries.go index 36d41001b8..236851845b 100644 --- a/server/service/queries.go +++ b/server/service/queries.go @@ -38,9 +38,29 @@ func (svc *Service) GetQuery(ctx context.Context, id uint) (*fleet.Query, error) if err := svc.authz.Authorize(ctx, query, fleet.ActionRead); err != nil { return nil, err } + svc.filterQueryPacksForUser(ctx, query) return query, nil } +// filterQueryPacksForUser removes from the given queries the packs that the +// requesting user is not authorized to read. Packs are associated to queries +// by name (see loadPacksForQueries), so a query's Packs field may include +// packs of same-named queries scoped to teams the user has no access to. +func (svc *Service) filterQueryPacksForUser(ctx context.Context, queries ...*fleet.Query) { + for _, query := range queries { + if len(query.Packs) == 0 { + continue + } + authorizedPacks := make([]fleet.Pack, 0, len(query.Packs)) + for _, pack := range query.Packs { + if err := svc.authz.Authorize(ctx, &pack, fleet.ActionRead); err == nil { + authorizedPacks = append(authorizedPacks, pack) + } + } + query.Packs = authorizedPacks + } +} + //////////////////////////////////////////////////////////////////////////////// // List Queries //////////////////////////////////////////////////////////////////////////////// @@ -116,6 +136,8 @@ func (svc *Service) ListQueries(ctx context.Context, opt fleet.ListOptions, team return nil, 0, 0, nil, err } + svc.filterQueryPacksForUser(ctx, queries...) + return queries, count, inheritedCount, meta, nil } @@ -476,6 +498,7 @@ func (svc *Service) ModifyQuery(ctx context.Context, id uint, p fleet.QueryPaylo return nil, ctxerr.Wrap(ctx, err, "create activity for query modification") } + svc.filterQueryPacksForUser(ctx, query) return query, nil } diff --git a/server/service/queries_test.go b/server/service/queries_test.go index 4a0687ca1f..2a18519151 100644 --- a/server/service/queries_test.go +++ b/server/service/queries_test.go @@ -851,6 +851,70 @@ func TestQueryAuth(t *testing.T) { } } +func TestQueryResponsesFilterUnauthorizedPacks(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + + teamID := uint(1) + // Simulates a pack scoped to another team that got associated to this + // query via the name-based join in loadPacksForQueries. + otherTeamPack := fleet.Pack{ID: 123, Name: "other team pack", Description: "secret"} + teamQuery := fleet.Query{ + ID: 88, + Name: "shared name", + TeamID: &teamID, + } + + ds.QueryFunc = func(ctx context.Context, id uint) (*fleet.Query, error) { + q := teamQuery + q.Packs = []fleet.Pack{otherTeamPack} + return &q, nil + } + ds.ListQueriesFunc = func(ctx context.Context, opts fleet.ListQueryOptions) ([]*fleet.Query, int, int, *fleet.PaginationMetadata, error) { + q := teamQuery + q.Packs = []fleet.Pack{otherTeamPack} + return []*fleet.Query{&q}, 1, 0, nil, nil + } + + // A team observer can read the team query but is not authorized to read + // packs, so pack metadata must be filtered out of the response. + teamObserver := &fleet.User{ + ID: 44, + Teams: []fleet.UserTeam{ + { + Team: fleet.Team{ID: teamID}, + Role: fleet.RoleObserver, + }, + }, + } + observerCtx := viewer.NewContext(ctx, viewer.Viewer{User: teamObserver}) + + query, err := svc.GetQuery(observerCtx, teamQuery.ID) + require.NoError(t, err) + require.Empty(t, query.Packs) + + queries, _, _, _, err := svc.ListQueries(observerCtx, fleet.ListOptions{}, &teamID, nil, false, nil) + require.NoError(t, err) + require.Len(t, queries, 1) + require.Empty(t, queries[0].Packs) + + // A global admin is authorized to read packs, so pack metadata is kept. + globalAdmin := &fleet.User{ + ID: 1, + GlobalRole: new(fleet.RoleAdmin), + } + adminCtx := viewer.NewContext(ctx, viewer.Viewer{User: globalAdmin}) + + query, err = svc.GetQuery(adminCtx, teamQuery.ID) + require.NoError(t, err) + require.Equal(t, []fleet.Pack{otherTeamPack}, query.Packs) + + queries, _, _, _, err = svc.ListQueries(adminCtx, fleet.ListOptions{}, &teamID, nil, false, nil) + require.NoError(t, err) + require.Len(t, queries, 1) + require.Equal(t, []fleet.Pack{otherTeamPack}, queries[0].Packs) +} + func TestQueryReportIsClipped(t *testing.T) { ds := new(mock.Store) svc, ctx := newTestService(t, ds, nil, nil)