Insecure RBAC cleanup / stale permission data after plugin uninstall

POC demonstrating potential stale RBAC data after uninstall prior to cleanup:\n\n- Scenario: A plugin registers an RBAC policy granting elevated permissions (e.g., read/write access to a sensitive resource) when installed. Prior to this fix, uninstall would remove the plugin but leave the RBAC policy in place, potentially allowing a user with valid tokens to exercise the plugin's permissions even after uninstall. The fix ensures CleanupPluginRBAC is invoked on removal to revoke such RBAC entries.\n\nGo-based PoC (conceptual, in-memory):\n```go\npackage main\n\nimport ( "fmt" )\n\n// Simplified in-memory RBAC store for demonstration\ntype RBACStore struct { entries map[string]string } // key: pluginID:perm, value: description\n\nfunc (s *RBACStore) Add(pluginID, perm string) { key := pluginID + ":" + perm; s.entries[key] = "granted" }\nfunc (s *RBACStore) Cleanup(pluginIDs ...string) { // this represents CleanupPluginRBAC in the fix; it should remove all entries for given plugins\n for _, pid := range pluginIDs {\n for k := range s.entries {\n if len(k) >= len(pid) && k[:len(pid)+1] == pid+":" { delete(s.entries, k) }\n }\n }\n}\nfunc (s *RBACStore) CheckAccess(pluginID, perm string) bool { key := pluginID + ":" + perm; _, ok := s.entries[key]; return ok }\n\nfunc main() {\n store := &RBACStore{entries: make(map[string]string)}\n\n // Step 1: Install plugin and grant elevated permission\n pluginID := "evil-plugin"\n store.Add(pluginID, "read:secret-space")\n fmt.Println("Before uninstall, access granted? ", store.CheckAccess(pluginID, "read:secret-space")) // true\n\n // Step 2: Uninstall plugin without cleanup (pre-fix behavior) — simulate by not calling Cleanup\n // In the real system, this is where CleanupPluginRBAC would be invoked. The entry still exists.\n\n // Step 3: After uninstall, access still permitted due to stale RBAC entry\n if store.CheckAccess(pluginID, "read:secret-space") {\n fmt.Println("Attack vector worked: stale RBAC entry allows access after uninstall.")\n } else {\n fmt.Println("Access correctly blocked after uninstall.")\n }\n\n // Step 4: With fix, Cleanup would remove entries for the plugin during uninstall.\n store.Cleanup(pluginID)\n if !store.CheckAccess(pluginID, "read:secret-space") {\n fmt.Println("RBAC entry cleaned up; access now blocked.")\n } else {\n fmt.Println("RBAC cleanup failed to remove entry.")\n }\n}\n```\nNotes:\n- The exact RBAC data model in Grafana is more complex, but this shows the vulnerability pattern: stale RBAC data tied to a plugin could remain after uninstall if cleanup is not performed. The patch makes CleanupPluginRBAC run on uninstall to revoke such permissions. The PoC uses an in-memory store for clarity and locality; in a real Grafana deployment, the RBAC store would be persisted and tied to the plugin lifecycle.\n

diff --git a/pkg/plugins/auth/models.go b/pkg/plugins/auth/models.go index 16248c1383947..2127ed3b244bb 100644 --- a/pkg/plugins/auth/models.go +++ b/pkg/plugins/auth/models.go @@ -24,3 +24,8 @@ type ExternalServiceRegistry interface { RegisterExternalService(ctx context.Context, pluginID string, pType string, svc *IAM) (*ExternalService, error) RemoveExternalService(ctx context.Context, pluginID string) error } + +// RBACCleanered to clean up RBAC data associated with plugins when they are uninstalled. +type RBACCleaner interface { + CleanupPluginRBAC(ctx context.Context, pluginIDs ...string) error +} diff --git a/pkg/plugins/manager/installer.go b/pkg/plugins/manager/installer.go index 21e5d2f9ab62e..840b6f3051de1 100644 --- a/pkg/plugins/manager/installer.go +++ b/pkg/plugins/manager/installer.go @@ -30,17 +30,18 @@ type PluginInstaller struct { installing sync.Map log log.Logger serviceRegistry auth.ExternalServiceRegistry + rbacCleaner auth.RBACCleaner } func ProvideInstaller(cfg *config.PluginManagementCfg, pluginRegistry registry.Service, pluginLoader loader.Service, - pluginRepo repo.Service, serviceRegistry auth.ExternalServiceRegistry) *PluginInstaller { + pluginRepo repo.Service, serviceRegistry auth.ExternalServiceRegistry, rbacCleaner auth.RBACCleaner) *PluginInstaller { return New(cfg, pluginRegistry, pluginLoader, pluginRepo, - storage.FileSystem(log.NewPrettyLogger("installer.fs"), cfg.PluginsPaths[0]), storage.SimpleDirNameGeneratorFunc, serviceRegistry) + storage.FileSystem(log.NewPrettyLogger("installer.fs"), cfg.PluginsPaths[0]), storage.SimpleDirNameGeneratorFunc, serviceRegistry, rbacCleaner) } func New(cfg *config.PluginManagementCfg, pluginRegistry registry.Service, pluginLoader loader.Service, pluginRepo repo.Service, pluginStorage storage.ZipExtractor, pluginStorageDirFunc storage.DirNameGeneratorFunc, - serviceRegistry auth.ExternalServiceRegistry) *PluginInstaller { + serviceRegistry auth.ExternalServiceRegistry, rbacCleaner auth.RBACCleaner) *PluginInstaller { return &PluginInstaller{ pluginLoader: pluginLoader, pluginRegistry: pluginRegistry, @@ -51,6 +52,7 @@ func New(cfg *config.PluginManagementCfg, pluginRegistry registry.Service, plugi installing: sync.Map{}, log: log.New("plugin.installer"), serviceRegistry: serviceRegistry, + rbacCleaner: rbacCleaner, } } @@ -212,6 +214,10 @@ func (m *PluginInstaller) Remove(ctx context.Context, pluginID, version string) } } + if err := m.rbacCleaner.CleanupPluginRBAC(ctx, pluginID); err != nil { + m.log.Error("Failed to cleanup plugin RBAC. Stale RBAC data can be cleaned up on next startup by setting the cfg.RBAC.PluginsCleanup config option", "pluginId", pluginID, "error", err) + } + has, err := m.serviceRegistry.HasExternalService(ctx, pluginID) if err == nil && has { return m.serviceRegistry.RemoveExternalService(ctx, pluginID) diff --git a/pkg/plugins/manager/installer_test.go b/pkg/plugins/manager/installer_test.go index 2ad327c9a953f..38a97965178b3 100644 --- a/pkg/plugins/manager/installer_test.go +++ b/pkg/plugins/manager/installer_test.go @@ -71,7 +71,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := inst.Add(context.Background(), pluginID, v1, testCompatOpts()) require.NoError(t, err) @@ -99,7 +99,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, nil }, } - inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := inst.Add(context.Background(), pluginID, v1, plugins.NewAddOpts(v1, runtime.GOOS, runtime.GOARCH, url)) require.NoError(t, err) }) @@ -222,7 +222,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - pm := New(&config.PluginManagementCfg{}, reg, &pluginfakes.FakeLoader{}, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + pm := New(&config.PluginManagementCfg{}, reg, &pluginfakes.FakeLoader{}, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := pm.Add(context.Background(), p.ID, "3.2.0", testCompatOpts()) require.ErrorIs(t, err, plugins.ErrInstallCorePlugin) @@ -287,7 +287,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := inst.Add(context.Background(), p3, "", testCompatOpts()) require.NoError(t, err) require.Equal(t, []string{p1Zip, p2Zip, p3Zip}, loadedPaths) @@ -339,7 +339,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := inst.Add(context.Background(), p1, "", testCompatOpts()) require.NoError(t, err) require.Equal(t, []string{p2Zip, p1Zip}, loadedPaths) @@ -387,7 +387,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(&config.PluginManagementCfg{}, reg, loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, reg, loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := inst.Add(context.Background(), testPluginID, "", testCompatOpts()) require.NoError(t, err) require.Equal(t, []string{"test-plugin.zip"}, loadedPaths) @@ -452,7 +452,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, pluginfakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err := inst.Add(context.Background(), parentPluginID, "", plugins.NewAddOpts("10.0.0", runtime.GOOS, runtime.GOARCH, parentURL)) require.NoError(t, err) require.Equal(t, []string{"dependency-plugin.zip", "parent-plugin.zip"}, loadedPaths) @@ -532,7 +532,7 @@ func TestPluginInstaller_Removal(t *testing.T) { _, err = os.Stat(pluginDir) require.NoError(t, err) - inst := New(&config.PluginManagementCfg{}, registry, loader, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, registry, loader, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err = inst.Remove(context.Background(), "localfs-plugin", "1.0.0") require.NoError(t, err) @@ -580,11 +580,108 @@ func TestPluginInstaller_Removal(t *testing.T) { _, err = os.Stat(pluginDir) require.NoError(t, err) - inst := New(&config.PluginManagementCfg{}, registry, loader, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}) + inst := New(&config.PluginManagementCfg{}, registry, loader, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, &pluginfakes.FakeRBACCleaner{}) err = inst.Remove(context.Background(), "staticfs-plugin", "1.0.0") require.NoError(t, err) _, err = os.Stat(pluginDir) require.ErrorIs(t, err, os.ErrNotExist) }) + + t.Run("CleanupPluginRBAC is called during plugin removal", func(t *testing.T) { + pluginDir := filepath.Join(tmpDir, "rbac-cleanup-plugin") + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "rbac-cleanup-plugin", + "name": "RBAC Cleanup Test Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + localFS := plugins.NewLocalFS(pluginDir) + pluginV1 := createPlugin(t, "rbac-cleanup-plugin", plugins.ClassExternal, true, true, func(plugin *plugins.Plugin) { + plugin.Info.Version = "1.0.0" + plugin.FS = localFS + }) + + registry := &pluginfakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + "rbac-cleanup-plugin": pluginV1, + }, + } + + loader := &pluginfakes.FakeLoader{ + UnloadFunc: func(_ context.Context, p *plugins.Plugin) (*plugins.Plugin, error) { + return p, nil + }, + } + + var cleanedUpPluginIDs []string + rbacCleaner := &pluginfakes.FakeRBACCleaner{ + CleanupFunc: func(ctx context.Context, pluginIDs []string) error { + cleanedUpPluginIDs = append(cleanedUpPluginIDs, pluginIDs...) + return nil + }, + } + + inst := New(&config.PluginManagementCfg{}, registry, loader, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, rbacCleaner) + err = inst.Remove(context.Background(), "rbac-cleanup-plugin", "1.0.0") + require.NoError(t, err) + + require.Equal(t, []string{"rbac-cleanup-plugin"}, cleanedUpPluginIDs) + }) + + t.Run("CleanupPluginRBAC error does not fail plugin removal", func(t *testing.T) { + pluginDir := filepath.Join(tmpDir, "rbac-cleanup-error-plugin") + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "rbac-cleanup-error-plugin", + "name": "RBAC Cleanup Error Test Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + localFS := plugins.NewLocalFS(pluginDir) + pluginV1 := createPlugin(t, "rbac-cleanup-error-plugin", plugins.ClassExternal, true, true, func(plugin *plugins.Plugin) { + plugin.Info.Version = "1.0.0" + plugin.FS = localFS + }) + + registry := &pluginfakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + "rbac-cleanup-error-plugin": pluginV1, + }, + } + + loader := &pluginfakes.FakeLoader{ + UnloadFunc: func(_ context.Context, p *plugins.Plugin) (*plugins.Plugin, error) { + return p, nil + }, + } + + rbacCleaner := &pluginfakes.FakeRBACCleaner{ + CleanupFunc: func(ctx context.Context, pluginIDs []string) error { + return errors.New("RBAC cleanup failed") + }, + } + + inst := New(&config.PluginManagementCfg{}, registry, loader, &pluginfakes.FakePluginRepo{}, &pluginfakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &pluginfakes.FakeAuthService{}, rbacCleaner) + err = inst.Remove(context.Background(), "rbac-cleanup-error-plugin", "1.0.0") + require.NoError(t, err) + + _, err = os.Stat(pluginDir) + require.True(t, os.IsNotExist(err)) + }) } diff --git a/pkg/plugins/manager/pluginfakes/fakes.go b/pkg/plugins/manager/pluginfakes/fakes.go index e449fd9bb2e31..56180e9c03893 100644 --- a/pkg/plugins/manager/pluginfakes/fakes.go +++ b/pkg/plugins/manager/pluginfakes/fakes.go @@ -546,6 +546,17 @@ func (f *FakeAuthService) RemoveExternalService(ctx context.Context, pluginID st return nil } +type FakeRBACCleaner struct { + CleanupFunc func(ctx context.Context, pluginIDs []string) error +} + +func (f *FakeRBACCleaner) CleanupPluginRBAC(ctx context.Context, pluginIDs ...string) error { + if f.CleanupFunc != nil { + return f.CleanupFunc(ctx, pluginIDs) + } + return nil +} + type FakeDiscoverer struct { DiscoverFunc func(ctx context.Context, src plugins.PluginSource) ([]*plugins.FoundBundle, error) } diff --git a/pkg/server/wire_gen.go b/pkg/server/wire_gen.go index 9fea305121151..b159236858961 100644 --- a/pkg/server/wire_gen.go +++ b/pkg/server/wire_gen.go @@ -646,7 +646,7 @@ func Initialize(ctx context.Context, cfg *setting.Cfg, opts Options, apiOpts api if err != nil { return nil, err } - pluginInstaller := manager4.ProvideInstaller(pluginManagementCfg, inMemory, loaderLoader, repoManager, serviceregistrationService) + pluginInstaller := manager4.ProvideInstaller(pluginManagementCfg, inMemory, loaderLoader, repoManager, serviceregistrationService, acimplService) ossProvider := guardian.ProvideGuardian() cacheServiceImpl := service7.ProvideCacheService(cacheService, sqlStore, ossProvider) shortURLService := shorturlimpl.ProvideService(sqlStore) @@ -1332,7 +1332,7 @@ func InitializeForTest(ctx context.Context, t sqlutil.ITestDB, testingT interfac if err != nil { return nil, err } - pluginInstaller := manager4.ProvideInstaller(pluginManagementCfg, inMemory, loaderLoader, repoManager, serviceregistrationService) + pluginInstaller := manager4.ProvideInstaller(pluginManagementCfg, inMemory, loaderLoader, repoManager, serviceregistrationService, acimplService) ossProvider := guardian.ProvideGuardian() cacheServiceImpl := service7.ProvideCacheService(cacheService, sqlStore, ossProvider) userAuthTokenService, err := authimpl.ProvideUserAuthTokenService(sqlStore, serverLockService, quotaService, secretsService, cfg, tracingService, featureToggles) diff --git a/pkg/server/wireexts_oss.go b/pkg/server/wireexts_oss.go index b07eead4b99bb..fbc2eb32694cf 100644 --- a/pkg/server/wireexts_oss.go +++ b/pkg/server/wireexts_oss.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/plugins" + pluginauth "github.com/grafana/grafana/pkg/plugins/auth" "github.com/grafana/grafana/pkg ... [truncated]