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

@@ -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
 }

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) {

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
+}

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-----

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}"
+        }
+      ]
+    }
+  ]
+}

pkg/plugins/manager/testdata/includes-symlinks/dashboards/extra/memory.json

@@ -0,0 +1,5 @@
+{
+  "title": "Nginx Memory",
+  "revision": 2,
+  "schemaVersion": 11
+}

pkg/plugins/manager/testdata/includes-symlinks/extra

@@ -0,0 +1 @@
+dashboards/extra

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"}
+    ]
+  }
+}

pkg/plugins/manager/testdata/includes-symlinks/symlink_to_txt

@@ -0,0 +1 @@
+text.txt

pkg/plugins/manager/testdata/includes-symlinks/text.txt

@@ -0,0 +1,8 @@
+eafafewfewfwef
+fawfwfwefewfwaef
+fawewfewfewfewfe
+wafewafewfewafewaf
+efewafeawfewafewf
+ewafewafewfeawfawf
+weafweafewfefewafewa
+fewafweafwafeaf

pkg/plugins/manager/testdata/symbolic-file-links/plugin/MANIFEST.txt

@@ -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-----

pkg/plugins/manager/testdata/symbolic-file-links/plugin/lib/somelib.so

@@ -1 +0,0 @@
-somelib.so.1

pkg/plugins/manager/testdata/symbolic-file-links/plugin/plugin.json

@@ -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"
-    }
-  }
-}

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:"-"`
 

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 {