self-hosted/joplin
1
0
Fork 0
mirror of https://github.com/laurent22/joplin.git synced 2025-01-11 18:24:43 +02:00
Code Issues Releases Activity

Desktop: Security: Prevent XSS and potential RCE when using a special HTML tag

Browse Source
This commit is contained in:
Laurent Cozic 2023-05-11 14:17:37 +01:00
parent b26bc9ed5f
commit 19bdda25c6
3 changed files with 33 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
packages/app-cli/tests/md_to_html/sanitize_14.html Normal file
View File

@ -0,0 +1 @@
<a href="#" class="jop-noMdConv">XSS</a>

1
packages/app-cli/tests/md_to_html/sanitize_14.md Normal file
View File

@ -0,0 +1 @@
<a data-from-md="" href="javascript:top.require('child_process').execSync('open -a Calculator')">XSS</a>

39
packages/renderer/htmlUtils.ts
View File

@ -144,6 +144,11 @@ class HtmlUtils {
.replace(/</g, '&lt;'); .replace(/</g, '&lt;');
} }
private isAcceptedUrl(url: string): boolean {
url = url.toLowerCase();
return url.startsWith('https://') || url.startsWith('http://') || url.startsWith('mailto://');
}
public sanitizeHtml(html: string, options: any = null) { public sanitizeHtml(html: string, options: any = null) {
options = Object.assign({}, { options = Object.assign({}, {
// If true, adds a "jop-noMdConv" class to all the tags. // If true, adds a "jop-noMdConv" class to all the tags.
@ -182,8 +187,12 @@ class HtmlUtils {
const parser = new htmlparser2.Parser({ const parser = new htmlparser2.Parser({
onopentag: (name: string, attrs: any) => { onopentag: (name: string, attrs: Record<string, string>) => {
tagStack.push(name.toLowerCase()); // Note: "name" and attribute names are always lowercase even
// when the input is not. So there is no need to call
// "toLowerCase" on them.
tagStack.push(name);
if (disallowedTags.includes(currentTag())) { if (disallowedTags.includes(currentTag())) {
disallowedTagDepth++; disallowedTagDepth++;
@ -202,11 +211,25 @@ class HtmlUtils {
// regular harmless attribute that starts with "on" will also // regular harmless attribute that starts with "on" will also
// be removed. // be removed.
// 0: https://developer.mozilla.org/en-US/docs/Web/Events // 0: https://developer.mozilla.org/en-US/docs/Web/Events
for (const name in attrs) { for (const attrName in attrs) {
if (!attrs.hasOwnProperty(name)) continue; if (!attrs.hasOwnProperty(attrName)) continue;
if (name.length <= 2) continue; if (attrName.length <= 2) continue;
if (name.toLowerCase().substr(0, 2) !== 'on') continue; if (attrName.substr(0, 2) !== 'on') continue;
delete attrs[name]; delete attrs[attrName];
}
if (name === 'a') {
// Make sure that only non-acceptable URLs are filtered out.
// In particular we want to exclude `javascript:` URLs.
if ('href' in attrs && !this.isAcceptedUrl(attrs['href'])) {
attrs['href'] = '#';
}
// We need to clear any such attribute, otherwise it will
// make any arbitrary link open within the application.
if ('data-from-md' in attrs) {
delete attrs['data-from-md'];
}
} }
if (options.addNoMdConvClass) { if (options.addNoMdConvClass) {
@ -222,7 +245,7 @@ class HtmlUtils {
// attribute. It doesn't always happen and it seems to depend on // attribute. It doesn't always happen and it seems to depend on
// what else is in the note but in any case adding the "href" // what else is in the note but in any case adding the "href"
// fixes it. https://github.com/laurent22/joplin/issues/5687 // fixes it. https://github.com/laurent22/joplin/issues/5687
if (name.toLowerCase() === 'a' && !attrs['href']) { if (name === 'a' && !attrs['href']) {
attrs['href'] = '#'; attrs['href'] = '#';
} }