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.

Sign up for GitHub

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

Chore: Use DOMPurify to sanitize strings rather than js-xss #62787

Merged
merged 1 commit into from
Mar 16, 2023
Merged

Chore: Use DOMPurify to sanitize strings rather than js-xss #62787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/dashboards-suite/dashboard-templating.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ e2e.scenario({
`Server:pipe = A'A"A|BB\\B|CCC`,
`Server:distributed = A'A"A,Server=BB\\B,Server=CCC`,
`Server:csv = A'A"A,BB\\B,CCC`,
`Server:html = A'A"A, BB\\B, CCC`,
`Server:html = A'A"A, BB\\B, CCC`,
`Server:json = ["A'A\\"A","BB\\\\B","CCC"]`,
`Server:percentencode = %7BA%27A%22A%2CBB%5CB%2CCCC%7D`,
`Server:singlequote = 'A\\'A"A','BB\\B','CCC'`,
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,6 @@
"dangerously-set-html-content": "1.0.9",
"date-fns": "2.29.3",
"debounce-promise": "3.1.2",
"dompurify": "^2.4.1",
"emotion": "11.0.0",
"eventemitter3": "5.0.0",
"fast-deep-equal": "^3.1.3",
Expand Down
4 changes: 3 additions & 1 deletion packages/grafana-data/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"@types/d3-interpolate": "^3.0.0",
"d3-interpolate": "3.0.1",
"date-fns": "2.29.3",
"dompurify": "^2.4.3",
"eventemitter3": "5.0.0",
"fast_array_intersect": "1.1.0",
"history": "4.10.1",
Expand All @@ -55,7 +56,7 @@
"tinycolor2": "1.6.0",
"tslib": "2.5.0",
"uplot": "1.6.24",
"xss": "1.0.14"
"xss": "^1.0.14"
},
"devDependencies": {
"@grafana/tsconfig": "^1.2.0-rc1",
Expand All @@ -67,6 +68,7 @@
"@testing-library/react": "12.1.4",
"@testing-library/react-hooks": "8.0.1",
"@testing-library/user-event": "14.4.3",
"@types/dompurify": "^2",
"@types/history": "4.7.11",
"@types/jest": "29.2.3",
"@types/jquery": "3.5.16",
Expand Down
10 changes: 9 additions & 1 deletion packages/grafana-data/src/text/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
export * from './string';
export * from './markdown';
export * from './text';
import { escapeHtml, hasAnsiCodes, sanitize, sanitizeUrl, sanitizeTextPanelContent } from './sanitize';
import {
escapeHtml,
hasAnsiCodes,
sanitize,
sanitizeUrl,
sanitizeTextPanelContent,
sanitizeSVGContent,
} from './sanitize';

export const textUtil = {
escapeHtml,
hasAnsiCodes,
sanitize,
sanitizeTextPanelContent,
sanitizeUrl,
sanitizeSVGContent,
};
12 changes: 12 additions & 0 deletions packages/grafana-data/src/text/markdown.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ describe('Markdown wrapper', () => {
expect(str).toBe('<script>alert()</script>');
});

it('should only escape (and not remove) code blocks inside markdown', () => {
const inlineCodeBlock = renderMarkdown('This is a piece of code block: `<script>alert()</script>`');
expect(inlineCodeBlock.trim()).toBe(
'<p>This is a piece of code block: <code>&lt;script&gt;alert()&lt;/script&gt;</code></p>'
);

const multilineCodeBlock = renderMarkdown('This is a piece of code block: ```<script>alert()</script>```');
expect(multilineCodeBlock.trim()).toBe(
'<p>This is a piece of code block: <code>&lt;script&gt;alert()&lt;/script&gt;</code></p>'
);
});

it('should sanitize content in text panel by default', () => {
const str = renderTextPanelMarkdown('<script>alert()</script>');
expect(str).toBe('&lt;script&gt;alert()&lt;/script&gt;');
Expand Down
4 changes: 2 additions & 2 deletions packages/grafana-data/src/text/markdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { marked } from 'marked';

import { sanitize, sanitizeTextPanelContent } from './sanitize';
import { sanitizeTextPanelContent } from './sanitize';

let hasInitialized = false;

Expand Down Expand Up @@ -37,7 +37,7 @@ export function renderMarkdown(str?: string, options?: RenderMarkdownOptions): s
return html;
}

return sanitize(html);
return sanitizeTextPanelContent(html);
}

export function renderTextPanelMarkdown(str?: string, options?: RenderMarkdownOptions): string {
Expand Down
19 changes: 18 additions & 1 deletion packages/grafana-data/src/text/sanitize.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { sanitizeTextPanelContent } from './sanitize';
import { sanitizeTextPanelContent, sanitizeUrl, sanitize } from './sanitize';

describe('Sanitize wrapper', () => {
it('should allow whitelisted styles in text panel', () => {
Expand All @@ -10,3 +10,20 @@ describe('Sanitize wrapper', () => {
);
});
});

describe('sanitizeUrl', () => {
it('sanitize javascript urls', () => {
const url = 'javascript:alert(document.domain)';
const str = sanitizeUrl(url);
expect(str).toBe('about:blank');
});
});

// write test to sanitize xss payloads using the sanitize function
describe('sanitize', () => {
it('should sanitize xss payload', () => {
const html = '<script>alert(1)</script>';
const str = sanitize(html);
expect(str).toBe('');
});
});
43 changes: 31 additions & 12 deletions packages/grafana-data/src/text/sanitize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
import { sanitizeUrl as braintreeSanitizeUrl } from '@braintree/sanitize-url';
import DOMPurify from 'dompurify';
import * as xss from 'xss';

const XSSWL = Object.keys(xss.whiteList).reduce<xss.IWhiteList>((acc, element) => {
acc[element] = xss.whiteList[element]?.concat(['class', 'style']);
return acc;
}, {});

const sanitizeXSS = new xss.FilterXSS({
whiteList: XSSWL,
});

const sanitizeTextPanelWhitelist = new xss.FilterXSS({
whiteList: XSSWL,
css: {
Expand All @@ -34,21 +31,29 @@ const sanitizeTextPanelWhitelist = new xss.FilterXSS({
});

/**
* 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
* Return a sanitized string that is going to be rendered in the browser to prevent XSS attacks.
* Note that sanitized tags will be removed, such as "<script>".
* We don't allow form, pre, or input elements.
*/
export function sanitize(unsanitizedString: string): string {
try {
return sanitizeXSS.process(unsanitizedString);
return DOMPurify.sanitize(unsanitizedString, {
USE_PROFILES: { html: true },
FORBID_TAGS: ['form', 'input', 'pre'],
});
} catch (error) {
console.error('String could not be sanitized', unsanitizedString);
return unsanitizedString;
return escapeHtml(unsanitizedString);
}
}

/**
* Returns string safe from XSS attacks to be used in the Text panel plugin.
*
* 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 sanitizeTextPanelContent(unsanitizedString: string): string {
try {
return sanitizeTextPanelWhitelist.process(unsanitizedString);
Expand All @@ -58,14 +63,28 @@ export function sanitizeTextPanelContent(unsanitizedString: string): string {
}
}

// Returns sanitized SVG, free from XSS attacks to be used when rendering SVG content.
export function sanitizeSVGContent(unsanitizedString: string): string {
return DOMPurify.sanitize(unsanitizedString, { USE_PROFILES: { svg: true, svgFilters: true } });
}

// Return a sanitized URL, free from XSS attacks, such as javascript:alert(1)
export function sanitizeUrl(url: string): string {
return braintreeSanitizeUrl(url);
}

// Returns true if the string contains ANSI color codes.
export function hasAnsiCodes(input: string): boolean {
return /\u001b\[\d{1,2}m/.test(input);
}

// Returns a string with HTML entities escaped.
export function escapeHtml(str: string): string {
return String(str).replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;').replace(/"/g, '&quot;');
return String(str)
.replace(/&/g, '&amp;')
.replace(/</g, '&lt;')
.replace(/>/g, '&gt;')
.replace(/'/g, '&#39;')
.replace(/\//g, '&#47;')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryantxu why escape forward slash here? I don't see any other example of HTML escape doing that, and the browser does seem to do it either in (tested with this):

function escapeHtml(html){
var text = document.createTextNode(html);
var p = document.createElement('p');
p.appendChild(text);
return p.innerHTML;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torkelo (answering since this was my PR) -- This is part of security enhancement, forward slash could be considered unsafe as it could be used like this <img/src/onerror=alert()> which breaks out of context. Other things such as paths, comments and schemas (file://) also use forward slash which can be used when exploiting vulnerabilities.

Is encoding forward slash causing issues?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KristianGrafana no, just curious why it was added, could not find it in any other escapeHtml implementation (incl browser impl)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torkelo Understandable. It's a good question and it simply boils down to the context. In most implementations, / is not escaped as it has little to no impact of providing XSS protection. In odd corner cases escaping / might be helpful, but honestly, it might be a good idea to try to follow standards here.

.replace(/"/g, '&quot;');
}
5 changes: 3 additions & 2 deletions public/app/core/components/SVG/SanitizedSVG.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as DOMPurify from 'dompurify';
import React from 'react';
import SVG, { Props } from 'react-inlinesvg';

import { textUtil } from '@grafana/data';

export const SanitizedSVG = (props: Props) => {
return <SVG {...props} cacheRequests={true} preProcessor={getCleanSVG} />;
};
Expand All @@ -11,7 +12,7 @@ let cache = new Map<string, string>();
function getCleanSVG(code: string): string {
let clean = cache.get(code);
if (!clean) {
clean = DOMPurify.sanitize(code, { USE_PROFILES: { svg: true, svgFilters: true } });
clean = textUtil.sanitizeSVGContent(code);
cache.set(code, clean);
}
return clean;
Expand Down
2 changes: 1 addition & 1 deletion public/app/features/templating/template_srv.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ describe('templateSrv', () => {
{ type: 'query', name: 'test', current: { value: '<script>alert(asd)</script>' } },
]);
const target = _templateSrv.replace('$test', {}, 'html');
expect(target).toBe('&lt;script&gt;alert(asd)&lt;/script&gt;');
expect(target).toBe('&lt;script&gt;alert(asd)&lt;&#47;script&gt;');
});
});

Expand Down
5 changes: 2 additions & 3 deletions public/app/plugins/panel/geomap/style/markers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import * as DOMPurify from 'dompurify';
import { Fill, RegularShape, Stroke, Circle, Style, Icon, Text } from 'ol/style';
import tinycolor from 'tinycolor2';

import { Registry, RegistryItem } from '@grafana/data';
import { Registry, RegistryItem, textUtil } from '@grafana/data';
import { config } from '@grafana/runtime';
import { getPublicOrAbsoluteUrl } from 'app/features/dimensions';

Expand Down Expand Up @@ -248,7 +247,7 @@ async function prepareSVG(url: string, size?: number): Promise<string> {
return res.text();
})
.then((text) => {
text = DOMPurify.sanitize(text, { USE_PROFILES: { svg: true, svgFilters: true } });
text = textUtil.sanitizeSVGContent(text);

const parser = new DOMParser();
const doc = parser.parseFromString(text, 'image/svg+xml');
Expand Down
15 changes: 8 additions & 7 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4927,6 +4927,7 @@ __metadata:
"@testing-library/react-hooks": 8.0.1
"@testing-library/user-event": 14.4.3
"@types/d3-interpolate": ^3.0.0
"@types/dompurify": ^2
"@types/history": 4.7.11
"@types/jest": 29.2.3
"@types/jquery": 3.5.16
Expand All @@ -4941,6 +4942,7 @@ __metadata:
"@types/tinycolor2": 1.4.3
d3-interpolate: 3.0.1
date-fns: 2.29.3
dompurify: ^2.4.3
esbuild: 0.16.17
eventemitter3: 5.0.0
fast_array_intersect: 1.1.0
Expand All @@ -4967,7 +4969,7 @@ __metadata:
tslib: 2.5.0
typescript: 4.8.4
uplot: 1.6.24
xss: 1.0.14
xss: ^1.0.14
peerDependencies:
react: ^16.8.0 || ^17.0.0
react-dom: ^16.8.0 || ^17.0.0
Expand Down Expand Up @@ -18575,10 +18577,10 @@ __metadata:
languageName: node
linkType: hard

"dompurify@npm:^2.4.1":
version: 2.4.3
resolution: "dompurify@npm:2.4.3"
checksum: b440981f2a38cada2085759cc3d1e2f94571afc34343d011a8a6aa1ad91ae6abf651adbfa4994b0e2283f0ce81f7891cdb04b67d0b234c8d190cb70e9691f026
"dompurify@npm:^2.4.3":
version: 2.4.5
resolution: "dompurify@npm:2.4.5"
checksum: d6d3c3b320f15cdb5b26aa1902c3275a3ab2c3705a9df4420bb94691d7c4df67959ec7b91e486c308320791b0ee000456f042734c45d76721e61c2768eac706e
languageName: node
linkType: hard

Expand Down Expand Up @@ -22254,7 +22256,6 @@ __metadata:
dangerously-set-html-content: 1.0.9
date-fns: 2.29.3
debounce-promise: 3.1.2
dompurify: ^2.4.1
emotion: 11.0.0
esbuild: 0.16.17
esbuild-loader: 2.21.0
Expand Down Expand Up @@ -40045,7 +40046,7 @@ __metadata:
languageName: node
linkType: hard

"xss@npm:1.0.14":
"xss@npm:^1.0.14":
version: 1.0.14
resolution: "xss@npm:1.0.14"
dependencies:
Expand Down