Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTML injection in panel links (drilldown) #17718

Closed
torkelo opened this issue Jun 24, 2019 · 3 comments · Fixed by #17731
Closed

HTML injection in panel links (drilldown) #17718

torkelo opened this issue Jun 24, 2019 · 3 comments · Fixed by #17731
Assignees
Milestone

Comments

@torkelo
Copy link
Member

torkelo commented Jun 24, 2019

You can inject image tags in panel drilldown links (via Title & url fields).

There is no script injection as this already sanitized.

But for these fields there is no need to have html here.

Problem is here:
https://github.com/grafana/grafana/blob/master/public/app/features/panel/panel_ctrl.ts#L269

Think using escape function when building the html there would solve it.

Here is where you can replicate it
Screen Shot 2019-06-24 at 10 13 52

@torkelo torkelo added this to the 6.2.5 milestone Jun 24, 2019
@torkelo torkelo added this to Bugs & Investigations in Frontend Platform Backlog Jun 24, 2019
@dprokop
Copy link
Member

dprokop commented Jun 24, 2019

@torkelo we need to update the new Data links as well to make sure HTML is escaped.

@nluedtke
Copy link

nluedtke commented Jul 1, 2019

This was assigned CVE-2019-13068.

@SchoolGuy
Copy link

Since SUSE Storage 5 is shipping Grafana 4, I created at work a patch which may be of help. This patch is a highly modified version of the PR #17731 and can be applied to the Grafana 4.6.x branch. I tested it with v4.6.5.

diff --git a/package.json b/package.json
index 78b4e6818c..8c09610fd2 100644
--- a/package.json
+++ b/package.json
@@ -124,6 +124,7 @@
     "rxjs": "^5.4.3",
     "tether": "^1.4.0",
     "tether-drop": "https://github.com/torkelo/drop",
-    "tinycolor2": "^1.4.1"
+    "tinycolor2": "^1.4.1",
+    "xss": "1.0.6"
   }
 }
diff --git a/public/app/core/utils/text.ts b/public/app/core/utils/text.ts
index e69de29bb2..b68547fc96 100644
--- a/public/app/core/utils/text.ts
+++ b/public/app/core/utils/text.ts
@@ -0,0 +1,34 @@
+import xss from 'xss';
+
+const XSSWL = Object.keys(xss.whiteList).reduce((acc, element) => {
+  acc[element] = xss.whiteList[element].concat(['class', 'style']);
+  return acc;
+}, {});
+
+const sanitizeXSS = new xss.FilterXSS({
+  whiteList: XSSWL,
+});
+
+/**
+ * Returns string safe from XSS attacks.
+ *
+ * Even though we allow the style-attribute, there's still default filtering applied to it
+ * Info: https://github.com/leizongmin/js-xss#customize-css-filter
+ * Whitelist: https://github.com/leizongmin/js-css-filter/blob/master/lib/default.js
+ */
+export function sanitize(unsanitizedString: string): string {
+  try {
+    return sanitizeXSS.process(unsanitizedString);
+  } catch (error) {
+    console.log('String could not be sanitized', unsanitizedString);
+    return unsanitizedString;
+  }
+}
+
+export function escapeHtml(str: string): string {
+  return String(str)
+    .replace(/&/g, '&')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/"/g, '&quot;');
+}
diff --git a/public/app/features/panel/panel_ctrl.ts b/public/app/features/panel/panel_ctrl.ts
index 34574bdb47..5856c8ef05 100644
--- a/public/app/features/panel/panel_ctrl.ts
+++ b/public/app/features/panel/panel_ctrl.ts
@@ -2,6 +2,7 @@ import config from 'app/core/config';
 import _ from 'lodash';
 import $ from 'jquery';
 import {profiler} from 'app/core/profiler';
+import { escapeHtml } from 'app/core/utils/text';
 import Remarkable from 'remarkable';
 
 const TITLE_HEIGHT = 25;
@@ -258,13 +259,15 @@ export class PanelCtrl {
     var interpolatedMarkdown = templateSrv.replace(markdown, this.panel.scopedVars);
     var html = '<div class="markdown-html">';
 
-    html += new Remarkable().render(interpolatedMarkdown);
+    const md = new Remarkable().render(interpolatedMarkdown);
+    html += config.disableSanitizeHtml ? md : sanitize(md);
 
     if (this.panel.links && this.panel.links.length > 0) {
       html += '<ul>';
       for (let link of this.panel.links) {
         var info = linkSrv.getPanelLinkAnchorInfo(link, this.panel.scopedVars);
-        html += '<li><a class="panel-menu-link" href="' + info.href + '" target="' + info.target + '">' + info.title + '</a></li>';
+        html += '<li><a class="panel-menu-link" href="' + escapeHtml(info.href);
+        html += '" target="' + escapeHtml(info.target) + '">' + escapeHtml(info.title) + '</a></li>';
       }
       html += '</ul>';
     }
diff --git a/public/app/plugins/panel/text/module.ts b/public/app/plugins/panel/text/module.ts
index 5f453aea15..cf73f3cffd 100644
--- a/public/app/plugins/panel/text/module.ts
+++ b/public/app/plugins/panel/text/module.ts
@@ -1,6 +1,7 @@
 ///<reference path="../../../headers/common.d.ts" />
 
 import _ from 'lodash';
+import { escapeHtml } from 'app/core/utils/text';
 import {PanelCtrl} from 'app/plugins/sdk';
 
 export class TextPanelCtrl extends PanelCtrl {
@@ -48,12 +49,8 @@ export class TextPanelCtrl extends PanelCtrl {
   }
 
   renderText(content) {
-    content = content
-    .replace(/&/g, '&amp;')
-    .replace(/>/g, '&gt;')
-    .replace(/</g, '&lt;')
-    .replace(/\n/g, '<br/>');
-    this.updateContent(content);
+    const safeContent = escapeHtml(content).replace(/\n/g, '<br/>');
+    this.updateContent(safeContent);
   }
 
   renderMarkdown(content) {
diff --git a/public/sass/base/_fonts.scss b/public/sass/base/_fonts.scss
index fb360ba587..250521e1d6 100644
--- a/public/sass/base/_fonts.scss
+++ b/public/sass/base/_fonts.scss
@@ -1,5 +1,5 @@
-@import "base/font_awesome";
-@import "base/grafana_icons";
+@import "font_awesome";
+@import "grafana_icons";
 
 /* Open sans fonts*/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants