Authorization bypass / Access control weakness

PoC (conceptual reproduction of the authorization weakness addressed by this fix): Background: - The vulnerability scenario assumes a folder hierarchy where a user has restricted rights that depend on inherited permissions from parent folders. - Prior to this fix, clients could (a) query access rights for a folder and (b) rely on or infer access control without a consistent server-side derivation that respected parent-chain inheritance, potentially leading to permission leakage or mis-privileged actions. Reproduction steps (pre-fix behavior, hypothetical): 1) Authenticate as a user with limited rights (e.g., read on a parent folder but no explicit rights on a child). 2) Request access information for a child folder that inherits permissions from the parent, via: GET https://grafana.example/api/folders/{child_uid}?accesscontrol=true (or through equivalent client-side RBAC probing). 3) Observe that the API returns an AccessControl map that either over-approximates or inaccurately reflects the user’s effective rights (e.g., seeing keys like dashboards:create or folders:write even if not actually granted due to inherited constraints). 4) Attempt an action that should be forbidden for this user (e.g., create a dashboard in the child folder) using the returned rights or by directly calling the action endpoint (POST /api/dashboards/db with folder context). 5) The action succeeds due to the mismatch between reported rights and actual enforcement, demonstrating an authorization bypass/ leakage. Fix-reproducing PoC (post-fix behavior): 1) Authenticate as the same restricted user. 2) Request access information for the child folder again: GET https://grafana.example/api/folders/{child_uid}?accesscontrol=true 3) Observe the AccessControl map only contains keys for actions the user is actually granted (e.g., none or only read/view-related actions). 4) Attempt the same restricted action (e.g., dashboards:create) in the child folder. The server should: - Deny the operation (403) if not granted, or - Succeed only if the user has a granted permission, consistent with the AccessControl map 5) The test confirms that AccessControl reflects only the user’s true permissions after resolving the parent-chain inheritance via the batch RBAC flow. Concrete exploit example (hypothetical): - Before fix, a GET /api/folders/{child_uid}?accesscontrol=true might reveal that actions such as dashboards:write or folders:delete are allowed for a user who only has read on the parent due to inheritance quirks, enabling a subsequent action like POST /api/dashboards/db to create a dashboard in the child folder. - After fix, the AccessControl map would only include actions the user actually has, preventing leakage of higher-privilege actions and blocking the unauthorized operation. Note: The actual PoC in a running Grafana instance would require a realistic folder hierarchy and user role setup. The PoC steps above illustrate the authentication, probing, and action attempt sequence that would demonstrate a vulnerability pre-fix and its mitigation post-fix.

diff --git a/apps/folder/pkg/apis/folder/v1/types.go b/apps/folder/pkg/apis/folder/v1/types.go index 2512e91520155..859dbc5b6b1eb 100644 --- a/apps/folder/pkg/apis/folder/v1/types.go +++ b/apps/folder/pkg/apis/folder/v1/types.go @@ -58,6 +58,15 @@ type FolderAccessInfo struct { CanEdit bool `json:"canEdit"` CanAdmin bool `json:"canAdmin"` CanDelete bool `json:"canDelete"` + + // AccessControl is a flat map of folder-domain action strings to bool, + // reflecting permissions after parent-chain inheritance has been resolved + // by the authorization system. Mirrors the shape of legacy + // dtos.Folder.AccessControl so clients can drop their dual call to + // /api/folders/{uid}?accesscontrol=true. Only keys for actions the user + // is granted appear here; absent keys mean "not granted". + // +optional + AccessControl map[string]bool `json:"accessControl,omitempty"` } func (FolderAccessInfo) OpenAPIModelName() string { diff --git a/packages/grafana-api-clients/src/clients/rtkq/folder/v1beta1/endpoints.gen.ts b/packages/grafana-api-clients/src/clients/rtkq/folder/v1beta1/endpoints.gen.ts index 8dac8369520f3..3572a5e56fe14 100644 --- a/packages/grafana-api-clients/src/clients/rtkq/folder/v1beta1/endpoints.gen.ts +++ b/packages/grafana-api-clients/src/clients/rtkq/folder/v1beta1/endpoints.gen.ts @@ -467,6 +467,10 @@ export type Status = { }; export type Patch = object; export type FolderAccessInfo = { + /** AccessControl is a flat map of folder-domain action strings to bool, reflecting permissions after parent-chain inheritance has been resolved by the authorization system. Mirrors the shape of legacy dtos.Folder.AccessControl so clients can drop their dual call to /api/folders/{uid}?accesscontrol=true. Only keys for actions the user is granted appear here; absent keys mean "not granted". */ + accessControl?: { + [key: string]: boolean; + }; /** APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources */ apiVersion?: string; canAdmin: boolean; diff --git a/packages/grafana-openapi/src/apis/folder.grafana.app-v1.json b/packages/grafana-openapi/src/apis/folder.grafana.app-v1.json index 558900c2513d7..18803b3910596 100644 --- a/packages/grafana-openapi/src/apis/folder.grafana.app-v1.json +++ b/packages/grafana-openapi/src/apis/folder.grafana.app-v1.json @@ -848,6 +848,14 @@ "type": "object", "required": ["canSave", "canEdit", "canAdmin", "canDelete"], "properties": { + "accessControl": { + "description": "AccessControl is a flat map of folder-domain action strings to bool, reflecting permissions after parent-chain inheritance has been resolved by the authorization system. Mirrors the shape of legacy dtos.Folder.AccessControl so clients can drop their dual call to /api/folders/{uid}?accesscontrol=true. Only keys for actions the user is granted appear here; absent keys mean \"not granted\".", + "type": "object", + "additionalProperties": { + "type": "boolean", + "default": false + } + }, "apiVersion": { "description": "APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources", "type": "string" diff --git a/packages/grafana-openapi/src/apis/folder.grafana.app-v1beta1.json b/packages/grafana-openapi/src/apis/folder.grafana.app-v1beta1.json index 7da577e0575b6..8c909fb2b031a 100644 --- a/packages/grafana-openapi/src/apis/folder.grafana.app-v1beta1.json +++ b/packages/grafana-openapi/src/apis/folder.grafana.app-v1beta1.json @@ -848,6 +848,14 @@ "type": "object", "required": ["canSave", "canEdit", "canAdmin", "canDelete"], "properties": { + "accessControl": { + "description": "AccessControl is a flat map of folder-domain action strings to bool, reflecting permissions after parent-chain inheritance has been resolved by the authorization system. Mirrors the shape of legacy dtos.Folder.AccessControl so clients can drop their dual call to /api/folders/{uid}?accesscontrol=true. Only keys for actions the user is granted appear here; absent keys mean \"not granted\".", + "type": "object", + "additionalProperties": { + "type": "boolean", + "default": false + } + }, "apiVersion": { "description": "APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources", "type": "string" diff --git a/pkg/registry/apis/folders/sub_access.go b/pkg/registry/apis/folders/sub_access.go index 11a26179a582f..0928e329e7ea1 100644 --- a/pkg/registry/apis/folders/sub_access.go +++ b/pkg/registry/apis/folders/sub_access.go @@ -57,6 +57,87 @@ func (r *subAccessREST) Connect(ctx context.Context, name string, opts runtime.O }), nil } +// folderTier mirrors the legacy folder permission levels (View / Edit / Admin) +// that folder.go uses to bundle dashboard, alerting, library-panel, and +// annotation actions onto a folder scope. +type folderTier int + +const ( + tierNone folderTier = iota + tierViewer + tierEditor + tierAdmin +) + +// folderTierCheck is one of the five folder-resource probes we send. The +// CorrelationID must match the [\w-]{1,36} regex enforced downstream, so we +// use a short stable slug instead of the legacy "domain:verb" action key. +type folderTierCheck struct { + correlationID string + verb string +} + +// folderTierChecks are the only items we send to BatchCheck. Sub-resource +// permissions (dashboards, alerts, library panels, annotations) are inferred +// from the resulting tier — they are NOT checked individually, matching the +// legacy folder View/Edit/Admin bundling in +// pkg/services/accesscontrol/ossaccesscontrol/folder.go. +var folderTierChecks = []folderTierCheck{ + {correlationID: "get", verb: utils.VerbGet}, + {correlationID: "create", verb: utils.VerbCreate}, + {correlationID: "update", verb: utils.VerbUpdate}, + {correlationID: "delete", verb: utils.VerbDelete}, + {correlationID: "setperms", verb: utils.VerbSetPermissions}, +} + +// Action bundles below mirror FolderViewActions / FolderEditActions / +// FolderAdminActions and DashboardViewActions / DashboardEditActions / +// DashboardAdminActions in pkg/services/accesscontrol/ossaccesscontrol/. They +// are inlined to avoid pulling that package's heavy DI graph into the apiserver +// edge. Keep in sync if either bundle changes. +var ( + folderViewActions = []string{ + "folders:read", + "alert.rules:read", + "library.panels:read", + "alert.silences:read", + } + folderEditActions = append(append([]string{}, folderViewActions...), []string{ + "folders:write", + "folders:delete", + "folders:create", + "dashboards:create", + "alert.rules:create", + "alert.rules:write", + "alert.rules:delete", + "alert.silences:create", + "alert.silences:write", + "library.panels:create", + "library.panels:write", + "library.panels:delete", + }...) + folderAdminActions = append(append([]string{}, folderEditActions...), []string{ + "folders.permissions:read", + "folders.permissions:write", + }...) + + dashboardViewActions = []string{ + "dashboards:read", + "annotations:read", + } + dashboardEditActions = append(append([]string{}, dashboardViewActions...), []string{ + "dashboards:write", + "dashboards:delete", + "annotations:write", + "annotations:delete", + "annotations:create", + }...) + dashboardAdminActions = append(append([]string{}, dashboardEditActions...), []string{ + "dashboards.permissions:read", + "dashboards.permissions:write", + }...) +) + func (r *subAccessREST) getAccessInfo(ctx context.Context, name string) (*foldersV1.FolderAccessInfo, error) { ns, err := request.NamespaceInfoFrom(ctx, true) if err != nil { @@ -67,7 +148,6 @@ func (r *subAccessREST) getAccessInfo(ctx context.Context, name string) (*folder return nil, err } - // Can view is managed here (and in the Authorizer) f, err := r.getter.Get(ctx, name, &v1.GetOptions{}) if err != nil { return nil, err @@ -76,37 +156,96 @@ func (r *subAccessREST) getAccessInfo(ctx context.Context, name string) (*folder if err != nil { return nil, err } - var tmp authlib.CheckResponse - check := func(verb string) bool { - if err != nil { - return false + parent := obj.GetFolder() + + checks := make([]authlib.BatchCheckItem, len(folderTierChecks)) + for i, c := range folderTierChecks { + checks[i] = authlib.BatchCheckItem{ + CorrelationID: c.correlationID, + Verb: c.verb, + Group: foldersV1.GROUP, + Resource: foldersV1.RESOURCE, + Name: name, + Folder: parent, } - tmp, err = r.accessClient.Check(ctx, user, authlib.CheckRequest{ - Verb: verb, - Group: foldersV1.GROUP, - Resource: foldersV1.RESOURCE, - Namespace: ns.Value, - Name: name, - }, obj.GetFolder()) - return tmp.Allowed } - rsp := &foldersV1.FolderAccessInfo{} - rsp.CanAdmin = check(utils.VerbSetPermissions) + batchResp, err := r.accessClient.BatchCheck(ctx, user, authlib.BatchCheckRequest{ + Namespace: ns.Value, + Checks: checks, + }) if err != nil { return nil, err } - rsp.CanDelete = rsp.CanAdmin || check(utils.VerbDelete) - if err != nil { - return nil, err + + allowed := make(map[string]bool, len(folderTierChecks)) + for _, c := range folderTierChecks { + result := batchResp.Results[c.correlationID] + if result.Error != nil { + return nil, result.Error + } + allowed[c.correlationID] = result.Allowed } - rsp.CanEdit = rsp.CanAdmin || check(utils.VerbUpdate) - if err != nil { - return nil, err + + // Can* mirrors the legacy pkg/api/folder.go newToFolderDto computation: + // canEdit / canSave both gate on folders:write, canDelete on folders:delete, + // canAdmin on the permissions verbs. CanAdmin implies the other three + // because the seeded Admin role bundles those actions. + canAdmin := allowed["setperms"] + rsp := &foldersV1.FolderAccessInfo{ + CanAdmin: canAdmin, + CanEdit: canAdmin || allowed["update"], + CanSave: canAdmin || allowed["update"], + CanDelete: canAdmin || allowed["delete"], } - rsp.CanSave = rsp.CanAdmin || check(utils.VerbCreate) // or the same as update? - if err != nil { - return nil, err + + if ac := actionsForTier(resolveTier(allowed)); len(ac) > 0 { + rsp.AccessControl = ac } + return rsp, nil } + +// resolveTier picks the highest tier the user qualifies for. Highest match +// wins: setPermissions → Admin; create/update/delete → Editor; get → Viewer. +func resolveTier(allowed map[string]bool) folderTier { + switch { + case allowed["setperms"]: + return tierAdmin + case allowed["create"] || allowed["update"] || allowed["delete"]: + return tierEditor + case allowed["get"]: + return tierViewer + default: + return tierNone + } +} + +// actionsForTier extrapolates the full RBAC action map for the tier. The +// returned set matches what the legacy /api/folders/:uid?accesscontrol=true +// endpoint produces for a folder at View / Edit / Admin level. +func actionsForTier(tier folderTier) map[string]bool { + var actions []string + switch tier { + case tierAdmin: + actions = make([]string, 0, len(folderAdminActions)+len(dashboardAdminActions)) + actions = append(actions, folderAdminActions...) + actions = append(actions, dashboardAdminActions...) + case tierEditor: + actions = make([]string, 0, len(folderEditActions)+len(dashboardEditActions)) + actions = append(actions, folderEditActions...) + actions = append(actions, dashboardEditActions...) + case tierViewer: + actions = make([]string, 0, len(folderViewActions)+len(dashboardViewActions)) + actions = append(actions, folderViewActions...) + actions = append(actions, dashboardViewActions...) + default: + return nil + } + + out := make(map[string]bool, len(actions)) + for _, a := range actions { + out[a] = true + } + return out +} diff --git a/pkg/registry/apis/folders/sub_access_test.go b/pkg/registry/apis/folders/sub_access_test.go new file mode 100644 index 0000000000000..d747a397fe3ba --- /dev/null +++ b/pkg/registry/apis/folders/sub_access_test.go @@ -0,0 +1,220 @@ +package folders + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/endpoints/request" + + authlib "github.com/grafana/authlib/types" + folders "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1" + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" + grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" + "github.com/grafana/grafana/pkg/services/user" +) + +func TestSubAccessREST_getAccessInfo(t *testing.T) { + type testCase struct { + name string + allowed map[string]bool // verb -> allowed + checkErr error + itemErr error // attached to the first BatchCheckItem's result + parentFolder string + expectCanAdmin bool + expectCanEdit bool + expectCanDelete bool + expectCanSave bool + expectActionsTier folderTier // tier whose action bundle should appear in AccessControl + expectNilAC bool // when tier is None, AccessControl is nil + expectErr bool + assertItem func(t *testing.T, item authlib.BatchCheckItem) + } + + tcs := []testCase{ + { + name: "setPermissions allowed → Admin tier; all Can* true; admin bundle returned", + allowed: map[string]bool{ + utils.VerbSetPermissions: true, + }, + expectCanAdmin: true, + expectCanEdit: true, + expectCanDelete: true, + expectCanSave: true, + expectActionsTier: tierAdmin, + }, + { + name: "every verb allowed → Admin tier (setPermissions still wins)", + allowed: map[string]bool{ + utils.VerbGet: true, + utils.VerbCreate: true, + utils.VerbUpdate: true, + utils.VerbDelete: true, + utils.VerbSetPermissions: true, + }, + expectCanAdmin: true, + expectCanEdit: true, + expectCanDelete: true, + expectCanSave: true, + expectActionsTier: tierAdmin, + }, + { + name: "update + get allowed → Editor tier; CanEdit/CanSave true (gated on update), CanDelete false (no delete verb)", + ... [truncated]