diff --git a/pkg/api/api.go b/pkg/api/api.go index 4b9e6d9708c..faddd78771b 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -133,7 +133,7 @@ func (hs *HTTPServer) registerRoutes() { r.Get("/api/login/ping", quota("session"), routing.Wrap(hs.LoginAPIPing)) // expose plugin file system assets - r.Get("/public/plugins/:pluginId/*", hs.GetPluginAssets) + r.Get("/public/plugins/:pluginId/*", hs.getPluginAssets) // authed api r.Group("/api", func(apiRoute routing.RouteRegister) { diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go index 7ece17c06cf..65f895f753d 100644 --- a/pkg/api/plugins.go +++ b/pkg/api/plugins.go @@ -3,12 +3,10 @@ package api import ( "encoding/json" "errors" - "fmt" "net/http" "os" "path/filepath" "sort" - "strings" "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana/pkg/api/dtos" @@ -21,14 +19,6 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -var permittedFileExts = []string{ - ".html", ".xhtml", ".css", ".js", ".json", ".jsonld", ".map", ".mjs", - ".jpeg", ".jpg", ".png", ".gif", ".svg", ".webp", ".ico", - ".woff", ".woff2", ".eot", ".ttf", ".otf", - ".wav", ".mp3", - ".md", ".pdf", ".txt", -} - func (hs *HTTPServer) GetPluginList(c *models.ReqContext) response.Response { typeFilter := c.Query("type") enabledFilter := c.Query("enabled") @@ -262,10 +252,10 @@ func (hs *HTTPServer) CollectPluginMetrics(c *models.ReqContext) response.Respon return response.CreateNormalResponse(headers, resp.PrometheusMetrics, http.StatusOK) } -// GetPluginAssets returns public plugin assets (images, JS, etc.) +// getPluginAssets returns public plugin assets (images, JS, etc.) // // /public/plugins/:pluginId/* -func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) { +func (hs *HTTPServer) getPluginAssets(c *models.ReqContext) { pluginID := c.Params("pluginId") plugin := hs.PluginManager.GetPlugin(pluginID) if plugin == nil { @@ -276,6 +266,11 @@ func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) { requestedFile := filepath.Clean(c.Params("*")) pluginFilePath := filepath.Join(plugin.PluginDir, requestedFile) + if !plugin.IncludedInSignature(requestedFile) { + hs.log.Warn("Access to requested plugin file will be forbidden in upcoming Grafana versions as the file "+ + "is not included in the plugin signature", "file", requestedFile) + } + // It's safe to ignore gosec warning G304 since we already clean the requested file path and subsequently // use this with a prefix of the plugin's directory, which is set during plugin loading // nolint:gosec @@ -300,12 +295,6 @@ func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) { return } - if accessForbidden(fi.Name()) { - c.JsonApiErr(403, "Plugin file access forbidden", - fmt.Errorf("access is forbidden to plugin file %s", pluginFilePath)) - return - } - if hs.Cfg.Env == setting.Dev { c.Resp.Header().Set("Cache-Control", "max-age=0, must-revalidate, no-cache") } else { @@ -448,14 +437,3 @@ func translatePluginRequestErrorToAPIError(err error) response.Response { return response.Error(500, "Plugin request failed", err) } - -func accessForbidden(pluginFilename string) bool { - ext := filepath.Ext(pluginFilename) - - for _, permittedExt := range permittedFileExts { - if strings.EqualFold(permittedExt, ext) { - return false - } - } - return true -} diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index 47ea607cda6..2814bea3ad1 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -1,89 +1,201 @@ package api import ( + "encoding/json" + "fmt" + "io/ioutil" + "os" + "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/manager" + "github.com/grafana/grafana/pkg/setting" ) -func Test_accessForbidden(t *testing.T) { - type testCase struct { - filename string - } - tests := []struct { - name string - t testCase - accessForbidden bool - }{ - { - name: ".exe files are forbidden", - t: testCase{ - filename: "test.exe", - }, - accessForbidden: true, - }, - { - name: ".sh files are forbidden", - t: testCase{ - filename: "test.sh", - }, - accessForbidden: true, - }, - { - name: "js is not forbidden", - t: testCase{ +func Test_GetPluginAssets(t *testing.T) { + pluginID := "test-plugin" + pluginDir := "." + tmpFile, err := ioutil.TempFile(pluginDir, "") + require.NoError(t, err) + t.Cleanup(func() { + err := os.RemoveAll(tmpFile.Name()) + assert.NoError(t, err) + }) + expectedBody := "Plugin test" + _, err = tmpFile.WriteString(expectedBody) + assert.NoError(t, err) - filename: "module.js", - }, - accessForbidden: false, - }, - { - name: "logos are not forbidden", - t: testCase{ + requestedFile := filepath.Clean(tmpFile.Name()) - filename: "logo.svg", + t.Run("Given a request for an existing plugin file that is listed as a signature covered file", func(t *testing.T) { + p := &plugins.PluginBase{ + Id: pluginID, + PluginDir: pluginDir, + SignedFiles: map[string]struct{}{ + requestedFile: {}, }, - accessForbidden: false, - }, - { - name: "JPGs are not forbidden", - t: testCase{ - filename: "img/test.jpg", + } + service := &pluginManager{ + plugins: map[string]*plugins.PluginBase{ + pluginID: p, }, - accessForbidden: false, - }, - { - name: "JPEGs are not forbidden", - t: testCase{ - filename: "img/test.jpeg", + } + l := &logger{} + + url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) + pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l, + func(sc *scenarioContext) { + callGetPluginAsset(sc) + + require.Equal(t, 200, sc.resp.Code) + assert.Equal(t, expectedBody, sc.resp.Body.String()) + assert.Empty(t, l.warnings) + }) + }) + + t.Run("Given a request for an existing plugin file that is not listed as a signature covered file", func(t *testing.T) { + p := &plugins.PluginBase{ + Id: pluginID, + PluginDir: pluginDir, + } + service := &pluginManager{ + plugins: map[string]*plugins.PluginBase{ + pluginID: p, }, - accessForbidden: false, - }, - { - name: "ext case is ignored", - t: testCase{ - filename: "scripts/runThis.SH", + } + l := &logger{} + + url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) + pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l, + func(sc *scenarioContext) { + callGetPluginAsset(sc) + + require.Equal(t, 200, sc.resp.Code) + assert.Equal(t, expectedBody, sc.resp.Body.String()) + assert.Empty(t, l.warnings) + }) + }) + + t.Run("Given a request for an non-existing plugin file", func(t *testing.T) { + p := &plugins.PluginBase{ + Id: pluginID, + PluginDir: pluginDir, + } + service := &pluginManager{ + plugins: map[string]*plugins.PluginBase{ + pluginID: p, }, - accessForbidden: true, - }, - { - name: "no file ext is forbidden", - t: testCase{ - filename: "scripts/runThis", + } + l := &logger{} + + requestedFile := "nonExistent" + url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) + pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l, + func(sc *scenarioContext) { + callGetPluginAsset(sc) + + var respJson map[string]interface{} + err := json.NewDecoder(sc.resp.Body).Decode(&respJson) + require.NoError(t, err) + require.Equal(t, 404, sc.resp.Code) + assert.Equal(t, "Plugin file not found", respJson["message"]) + assert.Empty(t, l.warnings) + }) + }) + + t.Run("Given a request for an non-existing plugin", func(t *testing.T) { + service := &pluginManager{ + plugins: map[string]*plugins.PluginBase{}, + } + l := &logger{} + + requestedFile := "nonExistent" + url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) + pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l, + func(sc *scenarioContext) { + callGetPluginAsset(sc) + + var respJson map[string]interface{} + err := json.NewDecoder(sc.resp.Body).Decode(&respJson) + require.NoError(t, err) + assert.Equal(t, 404, sc.resp.Code) + assert.Equal(t, "Plugin not found", respJson["message"]) + assert.Empty(t, l.warnings) + }) + }) + + t.Run("Given a request for a core plugin's file", func(t *testing.T) { + service := &pluginManager{ + plugins: map[string]*plugins.PluginBase{ + pluginID: { + IsCorePlugin: true, + }, }, - accessForbidden: true, - }, - { - name: "empty file ext is forbidden", - t: testCase{ - filename: "scripts/runThis.", - }, - accessForbidden: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := accessForbidden(tt.t.filename); got != tt.accessForbidden { - t.Errorf("accessForbidden() = %v, accessForbidden %v", got, tt.accessForbidden) - } - }) - } + } + l := &logger{} + + url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) + pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l, + func(sc *scenarioContext) { + callGetPluginAsset(sc) + + require.Equal(t, 200, sc.resp.Code) + assert.Equal(t, expectedBody, sc.resp.Body.String()) + assert.Empty(t, l.warnings) + }) + }) +} + +func callGetPluginAsset(sc *scenarioContext) { + sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() +} + +func pluginAssetScenario(t *testing.T, desc string, url string, urlPattern string, pluginManager plugins.Manager, + logger log.Logger, fn scenarioFunc) { + t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { + defer bus.ClearBusHandlers() + + hs := HTTPServer{ + Cfg: setting.NewCfg(), + PluginManager: pluginManager, + log: logger, + } + + sc := setupScenarioContext(t, url) + sc.defaultHandler = func(c *models.ReqContext) { + sc.context = c + hs.getPluginAssets(c) + } + + sc.m.Get(urlPattern, sc.defaultHandler) + + fn(sc) + }) +} + +type pluginManager struct { + manager.PluginManager + + plugins map[string]*plugins.PluginBase +} + +func (pm *pluginManager) GetPlugin(id string) *plugins.PluginBase { + return pm.plugins[id] +} + +type logger struct { + log.Logger + + warnings []string +} + +func (l *logger) Warn(msg string, ctx ...interface{}) { + l.warnings = append(l.warnings, msg) } diff --git a/pkg/plugins/manager/manager.go b/pkg/plugins/manager/manager.go index 7a4699dc9f5..3efbb2bdfbe 100644 --- a/pkg/plugins/manager/manager.go +++ b/pkg/plugins/manager/manager.go @@ -512,6 +512,7 @@ func (pm *PluginManager) loadPlugin(jsonParser *json.Decoder, pluginBase *plugin pb.Signature = pluginBase.Signature pb.SignatureType = pluginBase.SignatureType pb.SignatureOrg = pluginBase.SignatureOrg + pb.SignedFiles = pluginBase.SignedFiles pm.plugins[pb.Id] = pb pm.log.Debug("Successfully added plugin", "id", pb.Id) @@ -581,6 +582,7 @@ func (s *PluginScanner) loadPlugin(pluginJSONFilePath string) error { pluginCommon.Signature = signatureState.Status pluginCommon.SignatureType = signatureState.Type pluginCommon.SignatureOrg = signatureState.SigningOrg + pluginCommon.SignedFiles = signatureState.Files s.plugins[currentDir] = &pluginCommon diff --git a/pkg/plugins/manager/manager_test.go b/pkg/plugins/manager/manager_test.go index 64bef480aa8..32121dd6877 100644 --- a/pkg/plugins/manager/manager_test.go +++ b/pkg/plugins/manager/manager_test.go @@ -232,6 +232,7 @@ func TestPluginManager_Init(t *testing.T) { Signature: plugins.PluginSignatureValid, SignatureType: plugins.GrafanaType, SignatureOrg: "Grafana Labs", + SignedFiles: plugins.PluginFiles{"plugin.json": {}}, Dependencies: plugins.PluginDependencies{ GrafanaVersion: "*", Plugins: []plugins.PluginDependencyItem{}, @@ -497,6 +498,7 @@ func TestPluginManager_Installer(t *testing.T) { Signature: plugins.PluginSignatureValid, SignatureType: plugins.GrafanaType, SignatureOrg: "Grafana Labs", + SignedFiles: plugins.PluginFiles{"plugin.json": {}}, Dependencies: plugins.PluginDependencies{ GrafanaVersion: "*", Plugins: []plugins.PluginDependencyItem{}, diff --git a/pkg/plugins/manager/manifest.go b/pkg/plugins/manager/manifest.go index 1d5118c1d55..2a161d298ef 100644 --- a/pkg/plugins/manager/manifest.go +++ b/pkg/plugins/manager/manifest.go @@ -169,7 +169,7 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin } } - manifestFiles := make(map[string]bool, len(manifest.Files)) + manifestFiles := make(map[string]struct{}, len(manifest.Files)) // Verify the manifest contents log.Debug("Verifying contents of plugin manifest", "plugin", plugin.Id) @@ -207,7 +207,7 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin Status: plugins.PluginSignatureModified, }, nil } - manifestFiles[p] = true + manifestFiles[p] = struct{}{} } if manifest.isV2() { @@ -241,6 +241,7 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin Status: plugins.PluginSignatureValid, Type: manifest.SignatureType, SigningOrg: manifest.SignedByOrgName, + Files: manifestFiles, }, nil } diff --git a/pkg/plugins/models.go b/pkg/plugins/models.go index 414ad9b0b77..613ee44b2a5 100644 --- a/pkg/plugins/models.go +++ b/pkg/plugins/models.go @@ -73,6 +73,7 @@ type PluginBase struct { IsCorePlugin bool `json:"-"` SignatureType PluginSignatureType `json:"-"` SignatureOrg string `json:"-"` + SignedFiles PluginFiles `json:"-"` GrafanaNetVersion string `json:"-"` GrafanaNetHasUpdate bool `json:"-"` @@ -80,6 +81,23 @@ type PluginBase struct { Root *PluginBase } +func (p *PluginBase) IncludedInSignature(file string) bool { + // permit Core plugin files + if p.IsCorePlugin { + return true + } + + // permit when no signed files (no MANIFEST) + if p.SignedFiles == nil { + return true + } + + if _, exists := p.SignedFiles[file]; !exists { + return false + } + return true +} + type PluginDependencies struct { GrafanaVersion string `json:"grafanaVersion"` Plugins []PluginDependencyItem `json:"plugins"` diff --git a/pkg/plugins/state.go b/pkg/plugins/state.go index 313b6ec3c83..3cf2b879123 100644 --- a/pkg/plugins/state.go +++ b/pkg/plugins/state.go @@ -31,8 +31,11 @@ const ( PrivateType PluginSignatureType = "private" ) +type PluginFiles map[string]struct{} + type PluginSignatureState struct { Status PluginSignatureStatus Type PluginSignatureType SigningOrg string + Files PluginFiles }