Access Control / Authorization Bypass

Proof-of-concept (conceptual, no running Grafana instance required): Preconditions: - A resource (e.g., a Dashboard) exists in source folder S (e.g., Folder A). - The user has update/write permissions on S but has no permissions on destination folder D (e.g., Folder B). - The provisioning file path that represents the intended move points to a location under D (destination derived from file path). Steps (to reproduce the vulnerability in versions prior to this patch): 1) Prepare a provisioning file for the resource that resides in source folder S but whose file path resolves to destination folder D (e.g., provisioning path indicates Folder B). 2) Authenticate as a user with VERB_Update on S but without VERB_Update on D. 3) Invoke the provisioning authorization for updating/moving the resource from S to D. 4) Observe that the authorization check only verifies against the source folder (S) and permits the move, effectively bypassing D’s access restrictions. Expected behavior on patched versions (this commit): - The system derives the destination folder D from the file path and, if a move is detected (S != D), performs checks on both source (S) and destination (D). Since the user lacks access to D, the operation is denied. Code-oriented illustration (pseudocode): // Pre-fix vulnerability scenario (would succeed in older code) parsed := &ParsedResource{ Existing: oldResourceInS, Meta: &Meta{Folder: "D"}, GVR: ... } authorizer := NewAuthorizer(repo, reader, mockAccess, false) err := authorizer.AuthorizeResource(ctx, parsed, utils.VerbUpdate) // In vulnerable versions, err == nil because only source (S) was checked and user had access to S. // Post-fix scenario (with patch): // The same call would perform two checks: on S and on D. If the user lacks access to D, an error is returned. Notes: - This PoC is conceptual and intended to illustrate the vulnerability path; exact API names may vary with the Grafana provisioning test harness.

diff --git a/pkg/registry/apis/provisioning/resources/authorizer.go b/pkg/registry/apis/provisioning/resources/authorizer.go index 74559f8757f6c..f569d3d18514a 100644 --- a/pkg/registry/apis/provisioning/resources/authorizer.go +++ b/pkg/registry/apis/provisioning/resources/authorizer.go @@ -151,22 +151,28 @@ func NewAuthorizer(repo *provisioning.Repository, reader repository.Reader, acce // verb on the given resource. // // Authorization Model: -// - For new resources: Uses the folder from the file metadata -// - For existing resources: Uses the folder where the resource currently exists +// - For new resources: checks the destination folder (derived from the file path). +// - For existing resources where the folder is unchanged: checks that single folder. +// - For existing resources where the folder changes (cross-folder move): checks both +// the current DB location AND the destination. The user must have the required verb +// on both to prevent moving resources into folders they cannot access. // -// This distinction is important because the file content is user-controlled, while the -// existing resource location comes from the database. Checking against the actual location -// prevents users from bypassing folder permissions by declaring a different folder in their file. +// The destination folder is always derived from the file path (parser.go), never from +// user-supplied JSON body content, so it cannot be spoofed. // // Example - Creating a new dashboard: -// - File declares: folder="team-a" -// - Checks: create permission on "team-a" (user must be Editor or Admin) -// -// Example - Updating existing dashboard: -// - File declares: folder="public" (user is Editor) -// - Actual location: folder="team-a" (user is Reader - no edit access) -// - Checks: update permission on "team-a" (actual location) -// - Result: DENIED (prevents permission bypass) +// - File path resolves to: folder="team-a" +// - Checks: create permission on "team-a" +// +// Example - Updating a dashboard without changing its folder: +// - File path resolves to: folder="team-a" (same as DB) +// - Checks: update permission on "team-a" +// +// Example - Moving a dashboard to a restricted folder: +// - File path resolves to: folder="team-b" (user has no access) +// - Actual DB location: folder="team-a" (user has Editor access) +// - Checks: update on "team-a" (passes) AND update on "team-b" (fails) +// - Result: DENIED func (a *ProvisioningAuthorizer) AuthorizeResource(ctx context.Context, parsed *ParsedResource, verb string) error { // Determine the resource name for the authorization check var name string @@ -176,23 +182,36 @@ func (a *ProvisioningAuthorizer) AuthorizeResource(ctx context.Context, parsed * name = parsed.Obj.GetName() } - // Determine the folder for the authorization check. - // For new resources, use the folder from the file metadata. - // For existing resources, use the folder where the resource actually exists. - folder := parsed.Meta.GetFolder() + // metaFolder is the destination folder derived from the file path (not from + // user-controlled JSON content — see parser.go:222-236). It is always checked. + metaFolder := parsed.Meta.GetFolder() + + // For existing resources, also check the current DB location when it differs + // from the destination. This prevents a user from moving a resource out of a + // folder they can write to and into one they cannot, by simply writing the file + // to a different path. Both source and destination must be authorised. if parsed.Existing != nil { if meta, err := utils.MetaAccessor(parsed.Existing); err == nil && meta != nil { - folder = meta.GetFolder() + existingFolder := meta.GetFolder() + if existingFolder != metaFolder { + if err := a.access.Check(ctx, authlib.CheckRequest{ + Group: parsed.GVR.Group, + Resource: parsed.GVR.Resource, + Name: name, + Verb: verb, + }, existingFolder); err != nil { + return err + } + } } } - // Perform the authorization check return a.access.Check(ctx, authlib.CheckRequest{ Group: parsed.GVR.Group, Resource: parsed.GVR.Resource, Name: name, Verb: verb, - }, folder) + }, metaFolder) } // getFolderID resolves the folder ID for the given path, always reading diff --git a/pkg/registry/apis/provisioning/resources/authorizer_test.go b/pkg/registry/apis/provisioning/resources/authorizer_test.go index 0173668c14962..067da3e54c6fd 100644 --- a/pkg/registry/apis/provisioning/resources/authorizer_test.go +++ b/pkg/registry/apis/provisioning/resources/authorizer_test.go @@ -17,96 +17,115 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" ) -// TestAuthorizeResource_SecurityFix tests the critical security fix where authorization -// checks the actual resource's folder, not the folder claimed in the file. -func TestAuthorizeResource_SecurityFix(t *testing.T) { - tests := []struct { - name string - fileFolderID string - existingFolder string - hasExisting bool - verb string - expectedFolder string - description string - }{ - { - name: "new resource - uses folder from file metadata", - fileFolderID: "file-claimed-folder", - hasExisting: false, - verb: utils.VerbCreate, - expectedFolder: "file-claimed-folder", - description: "For new resources, should use folder from file metadata", +func makeAuthorizeResourceParsed(t *testing.T, fileFolderID, existingFolder string, hasExisting bool) *ParsedResource { + mockMeta := utils.NewMockGrafanaMetaAccessor(t) + mockMeta.On("GetFolder").Return(fileFolderID) + + parsed := &ParsedResource{ + Obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-dashboard", + }, + }, }, - { - name: "existing resource - SECURITY FIX: uses folder from actual resource, not file", - fileFolderID: "malicious-claimed-folder", // Attacker claims different folder - existingFolder: "actual-resource-folder", // Actual folder - hasExisting: true, - verb: utils.VerbUpdate, - expectedFolder: "actual-resource-folder", - description: "SECURITY FIX: For existing resources, should use folder from actual resource to prevent permission bypass", + Meta: mockMeta, + GVR: schema.GroupVersionResource{ + Group: "dashboard.grafana.app", + Resource: "dashboards", }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - mockAccess := auth.NewMockAccessChecker(t) - repo := &provisioning.Repository{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-repo", - }, - } - - // Create mock metadata for file - mockMeta := utils.NewMockGrafanaMetaAccessor(t) - mockMeta.On("GetFolder").Return(tt.fileFolderID) - - // Create parsed resource with file metadata - parsed := &ParsedResource{ - Obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "test-dashboard", - }, + if hasExisting { + parsed.Existing = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-dashboard", + "annotations": map[string]interface{}{ + "grafana.app/folder": existingFolder, }, }, - Meta: mockMeta, - GVR: schema.GroupVersionResource{ - Group: "dashboard.grafana.app", - Resource: "dashboards", - }, - } + }, + } + } - // Add existing resource if needed with folder annotation - if tt.hasExisting { - parsed.Existing = &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "test-dashboard", - "annotations": map[string]interface{}{ - "grafana.app/folder": tt.existingFolder, - }, - }, - }, - } - } + return parsed +} - // Set up expectation for the Check call - mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { - // Verify the request has correct verb and resource - return req.Verb == tt.verb && - req.Group == parsed.GVR.Group && - req.Resource == parsed.GVR.Resource - }), tt.expectedFolder).Return(nil).Once() +// TestAuthorizeResource tests authorization checks for resource operations. +func TestAuthorizeResource(t *testing.T) { + repo := &provisioning.Repository{ObjectMeta: metav1.ObjectMeta{Name: "test-repo"}} - authorizer := NewAuthorizer(repo, nil, mockAccess, false) - err := authorizer.AuthorizeResource(context.Background(), parsed, tt.verb) + t.Run("new resource uses destination folder only", func(t *testing.T) { + parsed := makeAuthorizeResourceParsed(t, "dest-folder", "", false) + mockAccess := auth.NewMockAccessChecker(t) + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbCreate + }), "dest-folder").Return(nil).Once() - assert.NoError(t, err, tt.description) - mockAccess.AssertExpectations(t) - mockMeta.AssertExpectations(t) - }) - } + err := NewAuthorizer(repo, nil, mockAccess, false).AuthorizeResource(context.Background(), parsed, utils.VerbCreate) + assert.NoError(t, err) + mockAccess.AssertExpectations(t) + }) + + t.Run("same-folder update runs single check", func(t *testing.T) { + parsed := makeAuthorizeResourceParsed(t, "folder-a", "folder-a", true) + mockAccess := auth.NewMockAccessChecker(t) + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate + }), "folder-a").Return(nil).Once() + + err := NewAuthorizer(repo, nil, mockAccess, false).AuthorizeResource(context.Background(), parsed, utils.VerbUpdate) + assert.NoError(t, err) + mockAccess.AssertExpectations(t) + }) + + t.Run("cross-folder move checks both source and destination", func(t *testing.T) { + parsed := makeAuthorizeResourceParsed(t, "folder-b", "folder-a", true) + mockAccess := auth.NewMockAccessChecker(t) + // source check + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate + }), "folder-a").Return(nil).Once() + // destination check + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate + }), "folder-b").Return(nil).Once() + + err := NewAuthorizer(repo, nil, mockAccess, false).AuthorizeResource(context.Background(), parsed, utils.VerbUpdate) + assert.NoError(t, err) + mockAccess.AssertExpectations(t) + }) + + t.Run("cross-folder move denied when destination is inaccessible", func(t *testing.T) { + parsed := makeAuthorizeResourceParsed(t, "restricted-folder", "allowed-folder", true) + mockAccess := auth.NewMockAccessChecker(t) + // source check passes + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate + }), "allowed-folder").Return(nil).Once() + // destination check fails + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate + }), "restricted-folder").Return(assert.AnError).Once() + + err := NewAuthorizer(repo, nil, mockAccess, false).AuthorizeResource(context.Background(), parsed, utils.VerbUpdate) + assert.Error(t, err) + mockAccess.AssertExpectations(t) + }) + + t.Run("cross-folder move denied when source is inaccessible", func(t *testing.T) { + parsed := makeAuthorizeResourceParsed(t, "dest-folder", "restricted-folder", true) + mockAccess := auth.NewMockAccessChecker(t) + // source check fails — destination check never runs + mockAccess.On("Check", mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate + }), "restricted-folder").Return(assert.AnError).Once() + + err := NewAuthorizer(repo, nil, mockAccess, false).AuthorizeResource(context.Background(), parsed, utils.VerbUpdate) + assert.Error(t, err) + mockAccess.AssertExpectations(t) + }) } // TestAuthorizeCreateFolder tests authorization checks for folder creation. diff --git a/pkg/tests/apis/provisioning/foldermetadata/folder_authorization_test.go b/pkg/tests/apis/provisioning/foldermetadata/folder_authorization_test.go index bd80b16897b38..88fcf104b8247 100644 --- a/pkg/tests/apis/provisioning/foldermetadata/folder_authorization_test.go +++ b/pkg/tests/apis/provisioning/foldermetadata/folder_authorization_test.go @@ -8,10 +8,89 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/tests/apis/provisioning/common" ) +// TestIntegrationProvisioning_CrossFolderWriteDeniedWithoutDestinationAccess verifies that +// writing a file to a folder the caller cannot access is denied, even when the resource +// already exists in a folder the caller can write to. +func TestIntegrationProvisioning_CrossFolderWriteDeniedWithoutDestinationAccess(t *testing.T) { + helper := sharedHelper(t) + ctx := context.Background() + + const repoName = "cross-folder-auth-test" + + // Minimal dashboard JSON placed in innerA during repo setup. + dashboardBody := []byte(`{ + "apiVersion": "dashboard.grafana.app/v0alpha1", + "kind": "Dashboard", + "metadata": {"name": "cross-folder-dash"}, + "spec": {"title": "Cross Folder Test Dashboard"} + }`) + + // Write folder metadata and dashboard before creating the repo so the first + // sync picks them all up. + writeToProvisioningPath(t, helper, "innerA/_folder.json", folderMetadataJSON("inner-a-uid", "Inner A")) + writeToProvisioningPath(t, helper, "innerB/_folder.json", folderMetadataJSON("inner-b-uid", "Inner B")) + writeToProvisioningPath(t, helper, "innerA/cross-folder-dash.json", dashboardBody) + + helper.CreateLocalRepo(t, common.TestRepo{ + Name: repoName, + SyncTarget: "folder", + Workflows: []string{"write"}, + SkipResourceAssertions: true, + }) + + helper.SyncAndWait(t, repoName, nil) + + // Verify both folders exist in Grafana before setting permissions. + _, err := helper.Folders.Resource.Get(ctx, "inner-a-uid", metav1.GetOptions{}) + require.NoError(t, err, "innerA should exist after sync") + _, err = helper.Folders.Resource.Get(ctx, "inner-b-uid", metav1.GetOptions{}) + require.NoError(t, err, "innerB should exist after sync") + + // Resolve the Viewer user's integer ID so we can set a user-specific folder ACL. + viewerUserID, err := identity.UserIdentifier(helper.Org1.Viewer.Identity.GetID()) + require.NoError(t, err, "resolve viewer user ID") + + // Grant the Viewer user edit (permission=2) access on innerA only. + // innerB receiv ... [truncated]