diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go index b8f65257ddc..9612551fe01 100644 --- a/pkg/api/plugins.go +++ b/pkg/api/plugins.go @@ -264,14 +264,27 @@ func (hs *HTTPServer) getPluginAssets(c *models.ReqContext) { return } - requestedFile := filepath.Clean(macaron.Params(c.Req)["*"]) - pluginFilePath := filepath.Join(plugin.PluginDir, requestedFile) + // prepend slash for cleaning relative paths + requestedFile := filepath.Clean(filepath.Join("/", macaron.Params(c.Req)["*"])) + rel, err := filepath.Rel("/", requestedFile) + if err != nil { + // slash is prepended above therefore this is not expected to fail + c.JsonApiErr(500, "Failed to get the relative path", err) + return + } - if !plugin.IncludedInSignature(requestedFile) { + if !plugin.IncludedInSignature(rel) { 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) } + absPluginDir, err := filepath.Abs(plugin.PluginDir) + if err != nil { + c.JsonApiErr(500, "Failed to get plugin absolute path", nil) + return + } + + pluginFilePath := filepath.Join(absPluginDir, rel) // 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 diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index 2814bea3ad1..aa6edb3007a 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -24,9 +24,13 @@ func Test_GetPluginAssets(t *testing.T) { pluginDir := "." tmpFile, err := ioutil.TempFile(pluginDir, "") require.NoError(t, err) + tmpFileInParentDir, err := ioutil.TempFile("..", "") + require.NoError(t, err) t.Cleanup(func() { err := os.RemoveAll(tmpFile.Name()) assert.NoError(t, err) + err = os.RemoveAll(tmpFileInParentDir.Name()) + assert.NoError(t, err) }) expectedBody := "Plugin test" _, err = tmpFile.WriteString(expectedBody) @@ -60,6 +64,30 @@ func Test_GetPluginAssets(t *testing.T) { }) }) + t.Run("Given a request for a relative path", func(t *testing.T) { + p := &plugins.PluginBase{ + Id: pluginID, + PluginDir: pluginDir, + SignedFiles: map[string]struct{}{ + requestedFile: {}, + }, + } + service := &pluginManager{ + plugins: map[string]*plugins.PluginBase{ + pluginID: p, + }, + } + l := &logger{} + + url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, tmpFileInParentDir.Name()) + pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l, + func(sc *scenarioContext) { + callGetPluginAsset(sc) + + require.Equal(t, 404, sc.resp.Code) + }) + }) + 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,