From 303352a89bb84ed584a2268cfe7a31fc35ab18a2 Mon Sep 17 00:00:00 2001 From: Will Browne Date: Tue, 15 Jun 2021 11:55:47 +0200 Subject: [PATCH] Plugins: Ignore symlinked folders when verifying plugin signature (#34434) * add check + test * fix test * add manifest * fix linter * add nolint * separate err cond checks * only collect relevant plugin files * skip symlinks * refactor * add missing test files + enable scanning Chromium.app/ * remove test since case already covered * remove unnecessary changes from before * refactor * remove comment --- pkg/plugins/manager/manager.go | 27 +------- pkg/plugins/manager/manager_test.go | 30 ++++---- pkg/plugins/manager/manifest.go | 68 ++++++++++++++++++- .../testdata/includes-symlinks/MANIFEST.txt | 31 +++++++++ .../dashboards/connections.json | 29 ++++++++ .../dashboards/extra/memory.json | 5 ++ .../manager/testdata/includes-symlinks/extra | 1 + .../testdata/includes-symlinks/plugin.json | 55 +++++++++++++++ .../testdata/includes-symlinks/symlink_to_txt | 1 + .../testdata/includes-symlinks/text.txt | 8 +++ .../symbolic-file-links/plugin/MANIFEST.txt | 32 --------- .../symbolic-file-links/plugin/lib/somelib.so | 1 - .../plugin/lib/somelib.so.1 | 0 .../symbolic-file-links/plugin/plugin.json | 16 ----- pkg/plugins/models.go | 1 - pkg/util/filepath.go | 2 +- 16 files changed, 215 insertions(+), 92 deletions(-) create mode 100644 pkg/plugins/manager/testdata/includes-symlinks/MANIFEST.txt create mode 100644 pkg/plugins/manager/testdata/includes-symlinks/dashboards/connections.json create mode 100644 pkg/plugins/manager/testdata/includes-symlinks/dashboards/extra/memory.json create mode 120000 pkg/plugins/manager/testdata/includes-symlinks/extra create mode 100644 pkg/plugins/manager/testdata/includes-symlinks/plugin.json create mode 120000 pkg/plugins/manager/testdata/includes-symlinks/symlink_to_txt create mode 100644 pkg/plugins/manager/testdata/includes-symlinks/text.txt delete mode 100644 pkg/plugins/manager/testdata/symbolic-file-links/plugin/MANIFEST.txt delete mode 120000 pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so delete mode 100644 pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so.1 delete mode 100644 pkg/plugins/manager/testdata/symbolic-file-links/plugin/plugin.json diff --git a/pkg/plugins/manager/manager.go b/pkg/plugins/manager/manager.go index aa24766d487..0cc4af9b7dc 100644 --- a/pkg/plugins/manager/manager.go +++ b/pkg/plugins/manager/manager.go @@ -530,7 +530,7 @@ func (s *PluginScanner) walker(currentPath string, f os.FileInfo, err error) err return fmt.Errorf("filepath.Walk reported an error for %q: %w", currentPath, err) } - if f.Name() == "node_modules" || f.Name() == "Chromium.app" { + if f.Name() == "node_modules" { return util.ErrWalkSkipDir } @@ -577,12 +577,6 @@ func (s *PluginScanner) loadPlugin(pluginJSONFilePath string) error { } pluginCommon.PluginDir = filepath.Dir(pluginJSONFilePath) - pluginCommon.Files, err = collectPluginFilesWithin(pluginCommon.PluginDir) - if err != nil { - s.log.Warn("Could not collect plugin file information in directory", "pluginID", pluginCommon.Id, "dir", pluginCommon.PluginDir) - return err - } - signatureState, err := getPluginSignatureState(s.log, &pluginCommon) if err != nil { s.log.Warn("Could not get plugin signature state", "pluginID", pluginCommon.Id, "err", err) @@ -726,25 +720,6 @@ func (pm *PluginManager) GetPluginMarkdown(pluginId string, name string) ([]byte return data, nil } -// gets plugin filenames that require verification for plugin signing -func collectPluginFilesWithin(rootDir string) ([]string, error) { - var files []string - err := filepath.Walk(rootDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if !info.IsDir() && info.Name() != "MANIFEST.txt" { - file, err := filepath.Rel(rootDir, path) - if err != nil { - return err - } - files = append(files, filepath.ToSlash(file)) - } - return nil - }) - return files, err -} - func (pm *PluginManager) StaticRoutes() []*plugins.PluginStaticRoute { return pm.staticRoutes } diff --git a/pkg/plugins/manager/manager_test.go b/pkg/plugins/manager/manager_test.go index 0a275cf7518..46f789f7c55 100644 --- a/pkg/plugins/manager/manager_test.go +++ b/pkg/plugins/manager/manager_test.go @@ -371,7 +371,7 @@ func TestPluginManager_Init(t *testing.T) { assert.Nil(t, pm.plugins[("test")]) }) - t.Run("With back-end plugin with a lib dir that has symbolic links", func(t *testing.T) { + t.Run("With plugin that contains symlink file + directory", func(t *testing.T) { origAppURL := setting.AppUrl t.Cleanup(func() { setting.AppUrl = origAppURL @@ -379,23 +379,25 @@ func TestPluginManager_Init(t *testing.T) { setting.AppUrl = defaultAppURL pm := createManager(t, func(pm *PluginManager) { - pm.Cfg.PluginsPath = "testdata/symbolic-file-links" + pm.Cfg.PluginsPath = "testdata/includes-symlinks" }) err := pm.Init() require.NoError(t, err) - // This plugin should be properly registered, even though it has a symbolicly linked file in it. require.Empty(t, pm.scanningErrors) - const pluginID = "test" - assert.NotNil(t, pm.plugins[pluginID]) - assert.Equal(t, "datasource", pm.plugins[pluginID].Type) - assert.Equal(t, "Test", pm.plugins[pluginID].Name) - assert.Equal(t, pluginID, pm.plugins[pluginID].Id) - assert.Equal(t, "1.0.0", pm.plugins[pluginID].Info.Version) - assert.Equal(t, plugins.PluginSignatureValid, pm.plugins[pluginID].Signature) - assert.Equal(t, plugins.GrafanaType, pm.plugins[pluginID].SignatureType) - assert.Equal(t, "Grafana Labs", pm.plugins[pluginID].SignatureOrg) - assert.False(t, pm.plugins[pluginID].IsCorePlugin) - assert.NotNil(t, pm.plugins[("test")]) + + const pluginID = "test-app" + p := pm.GetPlugin(pluginID) + + assert.NotNil(t, p) + assert.NotNil(t, pm.GetApp(pluginID)) + assert.Equal(t, pluginID, p.Id) + assert.Equal(t, "app", p.Type) + assert.Equal(t, "Test App", p.Name) + assert.Equal(t, "1.0.0", p.Info.Version) + assert.Equal(t, plugins.PluginSignatureValid, p.Signature) + assert.Equal(t, plugins.GrafanaType, p.SignatureType) + assert.Equal(t, "Grafana Labs", p.SignatureOrg) + assert.False(t, p.IsCorePlugin) }) t.Run("With back-end plugin that is symlinked to plugins dir", func(t *testing.T) { diff --git a/pkg/plugins/manager/manifest.go b/pkg/plugins/manager/manifest.go index a9b94ca10e4..1d5118c1d55 100644 --- a/pkg/plugins/manager/manifest.go +++ b/pkg/plugins/manager/manifest.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "encoding/json" "errors" + "fmt" "io" "io/ioutil" "net/url" @@ -210,9 +211,17 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin } if manifest.isV2() { + pluginFiles, err := pluginFilesRequiringVerification(plugin) + if err != nil { + log.Warn("Could not collect plugin file information in directory", "pluginID", plugin.Id, "dir", plugin.PluginDir) + return plugins.PluginSignatureState{ + Status: plugins.PluginSignatureInvalid, + }, err + } + // Track files missing from the manifest var unsignedFiles []string - for _, f := range plugin.Files { + for _, f := range pluginFiles { if _, exists := manifestFiles[f]; !exists { unsignedFiles = append(unsignedFiles, f) } @@ -234,3 +243,60 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin SigningOrg: manifest.SignedByOrgName, }, nil } + +// gets plugin filenames that require verification for plugin signing +// returns filenames as a slice of posix style paths relative to plugin directory +func pluginFilesRequiringVerification(plugin *plugins.PluginBase) ([]string, error) { + var files []string + err := filepath.Walk(plugin.PluginDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if info.Mode()&os.ModeSymlink == os.ModeSymlink { + symlinkPath, err := filepath.EvalSymlinks(path) + if err != nil { + return err + } + + symlink, err := os.Stat(symlinkPath) + if err != nil { + return err + } + + // skip symlink directories + if symlink.IsDir() { + return nil + } + + // verify that symlinked file is within plugin directory + p, err := filepath.Rel(plugin.PluginDir, symlinkPath) + if err != nil { + return err + } + if strings.HasPrefix(p, ".."+string(filepath.Separator)) { + return fmt.Errorf("file '%s' not inside of plugin directory", p) + } + } + + // skip directories and MANIFEST.txt + if info.IsDir() || info.Name() == "MANIFEST.txt" { + return nil + } + + // verify that file is within plugin directory + file, err := filepath.Rel(plugin.PluginDir, path) + if err != nil { + return err + } + if strings.HasPrefix(file, ".."+string(filepath.Separator)) { + return fmt.Errorf("file '%s' not inside of plugin directory", file) + } + + files = append(files, filepath.ToSlash(file)) + + return nil + }) + + return files, err +} diff --git a/pkg/plugins/manager/testdata/includes-symlinks/MANIFEST.txt b/pkg/plugins/manager/testdata/includes-symlinks/MANIFEST.txt new file mode 100644 index 00000000000..aa455a68b3c --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/MANIFEST.txt @@ -0,0 +1,31 @@ + +-----BEGIN PGP SIGNED MESSAGE----- +Hash: SHA512 + +{ + "manifestVersion": "2.0.0", + "signatureType": "grafana", + "signedByOrg": "grafana", + "signedByOrgName": "Grafana Labs", + "plugin": "test-app", + "version": "1.0.0", + "time": 1622547655175, + "keyId": "7e4d0c6a708866e7", + "files": { + "symlink_to_txt": "9f32c171bf78a85d5cb77a48ab44f85578ee2942a1fc9f9ec4fde194ae4ff048", + "plugin.json": "c59a51bf6d7ecd7a99608ccb99353390c8b973672a938a0247164324005c0caf", + "dashboards/connections.json": "bea86da4be970b98dc4681802ab55cdef3441dc3eb3c654cb207948d17b25303", + "dashboards/extra/memory.json": "7c042464941084caa91d0a9a2f188b05315a9796308a652ccdee31ca4fbcbfee", + "text.txt": "9f32c171bf78a85d5cb77a48ab44f85578ee2942a1fc9f9ec4fde194ae4ff048" + } +} +-----BEGIN PGP SIGNATURE----- +Version: OpenPGP.js v4.10.1 +Comment: https://openpgpjs.org + +wqEEARMKAAYFAmC2HMcACgkQfk0ManCIZuddZgIJAVsIWg93v20C9baQhKth +G9f1PY900AnjVNK/494/qdR7UrBWDH0LWboVIkcALEawo6oLMUzyuMIPknCy +QW45HwlQAgjRW2Wm1uxcFN6164M5UHAR9+a2mHrpva18bG6ELYliLRFsuEnj +WG0SOCjmUoSG/qN6iREuLxii5xK4Levu158kCg== +=Mpyz +-----END PGP SIGNATURE----- diff --git a/pkg/plugins/manager/testdata/includes-symlinks/dashboards/connections.json b/pkg/plugins/manager/testdata/includes-symlinks/dashboards/connections.json new file mode 100644 index 00000000000..042dc4baec5 --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/dashboards/connections.json @@ -0,0 +1,29 @@ +{ + "__inputs": [ + { + "name": "DS_NAME", + "type": "datasource", + "pluginId": "graphite" + } + ], + + "uid": "1MHHlVjzz", + "title": "Nginx Connections", + "revision": 25, + "schemaVersion": 11, + "tags": ["tag1", "tag2"], + "number_array": [1,2,3,10.33], + "boolean_true": true, + "boolean_false": false, + + "rows": [ + { + "panels": [ + { + "type": "graph", + "datasource": "${DS_NAME}" + } + ] + } + ] +} diff --git a/pkg/plugins/manager/testdata/includes-symlinks/dashboards/extra/memory.json b/pkg/plugins/manager/testdata/includes-symlinks/dashboards/extra/memory.json new file mode 100644 index 00000000000..983b994b40e --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/dashboards/extra/memory.json @@ -0,0 +1,5 @@ +{ + "title": "Nginx Memory", + "revision": 2, + "schemaVersion": 11 +} diff --git a/pkg/plugins/manager/testdata/includes-symlinks/extra b/pkg/plugins/manager/testdata/includes-symlinks/extra new file mode 120000 index 00000000000..e06802a5626 --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/extra @@ -0,0 +1 @@ +dashboards/extra \ No newline at end of file diff --git a/pkg/plugins/manager/testdata/includes-symlinks/plugin.json b/pkg/plugins/manager/testdata/includes-symlinks/plugin.json new file mode 100644 index 00000000000..cc82141690c --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/plugin.json @@ -0,0 +1,55 @@ +{ + "type": "app", + "name": "Test App", + "id": "test-app", + + "staticRoot": ".", + + "pages": [ + { "name": "Live stream", "component": "StreamPageCtrl", "role": "Editor"}, + { "name": "Log view", "component": "LogsPageCtrl", "role": "Viewer"} + ], + + "css": { + "dark": "css/dark.css", + "light": "css/light.css" + }, + + "info": { + "description": "Official Grafana Test App & Dashboard bundle", + "author": { + "name": "Test Inc.", + "url": "http://test.com" + }, + "keywords": ["test"], + "logos": { + "small": "img/logo_small.png", + "large": "img/logo_large.png" + }, + "screenshots": [ + {"name": "img1", "path": "img/screenshot1.png"}, + {"name": "img2", "path": "img/screenshot2.png"} + ], + "links": [ + {"name": "Project site", "url": "http://project.com"}, + {"name": "License & Terms", "url": "http://license.com"} + ], + "version": "1.0.0", + "updated": "2015-02-10" + }, + + "includes": [ + {"type": "dashboard", "name": "Nginx Connections", "path": "dashboards/connections.json"}, + {"type": "dashboard", "name": "Nginx Memory", "path": "dashboards/memory.json"}, + {"type": "panel", "name": "Nginx Panel"}, + {"type": "datasource", "name": "Nginx Datasource"} + ], + + "dependencies": { + "grafanaVersion": "3.x.x", + "plugins": [ + {"type": "datasource", "id": "graphite", "name": "Graphite", "version": "1.0.0"}, + {"type": "panel", "id": "graph", "name": "Graph", "version": "1.0.0"} + ] + } +} diff --git a/pkg/plugins/manager/testdata/includes-symlinks/symlink_to_txt b/pkg/plugins/manager/testdata/includes-symlinks/symlink_to_txt new file mode 120000 index 00000000000..031720c5f78 --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/symlink_to_txt @@ -0,0 +1 @@ +text.txt \ No newline at end of file diff --git a/pkg/plugins/manager/testdata/includes-symlinks/text.txt b/pkg/plugins/manager/testdata/includes-symlinks/text.txt new file mode 100644 index 00000000000..3cc2a73f7dd --- /dev/null +++ b/pkg/plugins/manager/testdata/includes-symlinks/text.txt @@ -0,0 +1,8 @@ +eafafewfewfwef +fawfwfwefewfwaef +fawewfewfewfewfe +wafewafewfewafewaf +efewafeawfewafewf +ewafewafewfeawfawf +weafweafewfefewafewa +fewafweafwafeaf diff --git a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/MANIFEST.txt b/pkg/plugins/manager/testdata/symbolic-file-links/plugin/MANIFEST.txt deleted file mode 100644 index 673baa0906b..00000000000 --- a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/MANIFEST.txt +++ /dev/null @@ -1,32 +0,0 @@ - ------BEGIN PGP SIGNED MESSAGE----- -Hash: SHA512 - -{ - "manifestVersion": "2.0.0", - "signatureType": "grafana", - "signedByOrg": "grafana", - "signedByOrgName": "Grafana Labs", - "rootUrls": [ - "http://localhost:3000/" - ], - "plugin": "test", - "version": "1.0.0", - "time": 1623327375936, - "keyId": "7e4d0c6a708866e7", - "files": { - "plugin.json": "2bb467c0bfd6c454551419efe475b8bf8573734e73c7bab52b14842adb62886f", - "lib/somelib.so.1": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", - "lib/somelib.so": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" - } -} ------BEGIN PGP SIGNATURE----- -Version: OpenPGP.js v4.10.1 -Comment: https://openpgpjs.org - -wqEEARMKAAYFAmDCApAACgkQfk0ManCIZuc+JAIJAVpL9/0Q1vnQyB+0MaOJ -oklt6Kw7/8OyL0oOj3lG0eW3qkJSOUfdM7Si2VW/BudruyUQovpU3EXr00/A -oR1SZtoLAgdNyc9KldvPNV95XAsNz8jjBzn12G2Qer4/Vgjzpl5wvn2WvZ1l -/NFR8rwHVhvMyGe7c1PJ6Gt6KpbQZ5j20j1NMA== -=GIHH ------END PGP SIGNATURE----- diff --git a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so b/pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so deleted file mode 120000 index 9c281a4a7d5..00000000000 --- a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so +++ /dev/null @@ -1 +0,0 @@ -somelib.so.1 \ No newline at end of file diff --git a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so.1 b/pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so.1 deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/plugin.json b/pkg/plugins/manager/testdata/symbolic-file-links/plugin/plugin.json deleted file mode 100644 index 31e38a2be85..00000000000 --- a/pkg/plugins/manager/testdata/symbolic-file-links/plugin/plugin.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "type": "datasource", - "name": "Test", - "id": "test", - "backend": true, - "executable": "test", - "state": "alpha", - "info": { - "version": "1.0.0", - "description": "Test", - "author": { - "name": "Will Browne", - "url": "https://willbrowne.com" - } - } -} diff --git a/pkg/plugins/models.go b/pkg/plugins/models.go index e3210643609..9eb104d20c3 100644 --- a/pkg/plugins/models.go +++ b/pkg/plugins/models.go @@ -71,7 +71,6 @@ type PluginBase struct { PluginDir string `json:"-"` DefaultNavUrl string `json:"-"` IsCorePlugin bool `json:"-"` - Files []string `json:"-"` SignatureType PluginSignatureType `json:"-"` SignatureOrg string `json:"-"` diff --git a/pkg/util/filepath.go b/pkg/util/filepath.go index f59a71107f8..46b15732391 100644 --- a/pkg/util/filepath.go +++ b/pkg/util/filepath.go @@ -46,7 +46,7 @@ func Walk(path string, followSymlinks bool, detectSymlinkInfiniteLoop bool, walk // If symlinkPathsFollowed is not nil, then we need to detect infinite loop. func walk(path string, info os.FileInfo, resolvedPath string, symlinkPathsFollowed map[string]bool, walkFn WalkFunc) error { if info == nil { - return errors.New("Walk: Nil FileInfo passed") + return errors.New("walk: Nil FileInfo passed") } err := walkFn(resolvedPath, info, nil) if err != nil {