Plugins: Use file extension allowlist when serving plugin assets inst…

…ead of checking for UNIX executable (grafana#37688) * explicitly check for plugin binary * remove check completely * resolve conflicts * allow module + logos * add tests * simplify * rework to allowlist * add case * remove old stuff * simplify * add case insensitive test
felipevallejo · Aug 9, 2021 · e0315da · e0315da
1 parent fe50031
commit e0315da
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 9 deletions.
diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go
@@ -21,6 +21,14 @@ 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")
@@ -292,9 +300,9 @@ func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) {
 		return
 	}
 
-	if shouldExclude(fi) {
+	if accessForbidden(fi.Name()) {
 		c.JsonApiErr(403, "Plugin file access forbidden",
-			fmt.Errorf("access is forbidden to executable plugin file %s", pluginFilePath))
+			fmt.Errorf("access is forbidden to plugin file %s", pluginFilePath))
 		return
 	}
 
@@ -441,12 +449,13 @@ func translatePluginRequestErrorToAPIError(err error) response.Response {
 	return response.Error(500, "Plugin request failed", err)
 }
 
-func shouldExclude(fi os.FileInfo) bool {
-	normalizedFilename := strings.ToLower(fi.Name())
-
-	isUnixExecutable := fi.Mode()&0111 == 0111
-	isWindowsExecutable := strings.HasSuffix(normalizedFilename, ".exe")
-	isScript := strings.HasSuffix(normalizedFilename, ".sh")
+func accessForbidden(pluginFilename string) bool {
+	ext := filepath.Ext(pluginFilename)
 
-	return isUnixExecutable || isWindowsExecutable || isScript
+	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
@@ -0,0 +1,89 @@
+package api
+
+import (
+	"testing"
+)
+
+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{
+
+				filename: "module.js",
+			},
+			accessForbidden: false,
+		},
+		{
+			name: "logos are not forbidden",
+			t: testCase{
+
+				filename: "logo.svg",
+			},
+			accessForbidden: false,
+		},
+		{
+			name: "JPGs are not forbidden",
+			t: testCase{
+				filename: "img/test.jpg",
+			},
+			accessForbidden: false,
+		},
+		{
+			name: "JPEGs are not forbidden",
+			t: testCase{
+				filename: "img/test.jpeg",
+			},
+			accessForbidden: false,
+		},
+		{
+			name: "ext case is ignored",
+			t: testCase{
+				filename: "scripts/runThis.SH",
+			},
+			accessForbidden: true,
+		},
+		{
+			name: "no file ext is forbidden",
+			t: testCase{
+				filename: "scripts/runThis",
+			},
+			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)
+			}
+		})
+	}
+}