From 774c20772b245ce6542a32cd46fe629b40ee457f Mon Sep 17 00:00:00 2001
From: Laurent Cozic <laurent@cozic.net>
Date: Mon, 25 Apr 2022 17:17:54 +0100
Subject: [PATCH] Security: Fixed disallowed tag XSS

---
 .../app-cli/tests/md_to_html/sanitize_11.html |  0
 .../app-cli/tests/md_to_html/sanitize_11.md   |  1 +
 packages/renderer/htmlUtils.ts                | 21 +++++++++++++++----
 3 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 packages/app-cli/tests/md_to_html/sanitize_11.html
 create mode 100644 packages/app-cli/tests/md_to_html/sanitize_11.md

diff --git a/packages/app-cli/tests/md_to_html/sanitize_11.html b/packages/app-cli/tests/md_to_html/sanitize_11.html
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/packages/app-cli/tests/md_to_html/sanitize_11.md b/packages/app-cli/tests/md_to_html/sanitize_11.md
new file mode 100644
index 0000000000..dca473d3a9
--- /dev/null
+++ b/packages/app-cli/tests/md_to_html/sanitize_11.md
@@ -0,0 +1 @@
+<iframe  src=""><svg><style><img src="" onerror=this.onerror=confirm('vulnerable_to_XSS')
\ No newline at end of file
diff --git a/packages/renderer/htmlUtils.ts b/packages/renderer/htmlUtils.ts
index 546239cdb6..61759bfbf7 100644
--- a/packages/renderer/htmlUtils.ts
+++ b/packages/renderer/htmlUtils.ts
@@ -155,6 +155,11 @@ class HtmlUtils {
 			return tagStack[tagStack.length - 1];
 		};
 
+		// When we encounter a disallowed tag, all the other tags within it are
+		// going to be skipped too. This is necessary to prevent certain XSS
+		// attacks. See sanitize_11.md
+		let disallowedTagDepth = 0;
+
 		// The BASE tag allows changing the base URL from which files are
 		// loaded, and that can break several plugins, such as Katex (which
 		// needs to load CSS files using a relative URL). For that reason
@@ -164,14 +169,20 @@ class HtmlUtils {
 		// "link" can be used to escape the parser and inject JavaScript.
 		// Adding "meta" too for the same reason as it shouldn't be used in
 		// notes anyway.
-		const disallowedTags = ['script', 'iframe', 'frameset', 'frame', 'object', 'base', 'embed', 'link', 'meta', 'noscript', 'button', 'form', 'input', 'select', 'textarea', 'option', 'optgroup'];
+		const disallowedTags = [
+			'script', 'iframe', 'frameset', 'frame', 'object', 'base',
+			'embed', 'link', 'meta', 'noscript', 'button', 'form',
+			'input', 'select', 'textarea', 'option', 'optgroup',
+		];
 
 		const parser = new htmlparser2.Parser({
 
 			onopentag: (name: string, attrs: any) => {
 				tagStack.push(name.toLowerCase());
 
-				if (disallowedTags.includes(currentTag())) return;
+				if (disallowedTags.includes(currentTag())) disallowedTagDepth++;
+
+				if (disallowedTagDepth) return;
 
 				attrs = Object.assign({}, attrs);
 
@@ -214,7 +225,7 @@ class HtmlUtils {
 			},
 
 			ontext: (decodedText: string) => {
-				if (disallowedTags.includes(currentTag())) return;
+				if (disallowedTagDepth) return;
 
 				if (currentTag() === 'style') {
 					// For CSS, we have to put the style as-is inside the tag because if we html-entities encode
@@ -231,7 +242,9 @@ class HtmlUtils {
 
 				if (current === name.toLowerCase()) tagStack.pop();
 
-				if (disallowedTags.includes(current)) return;
+				if (disallowedTags.includes(current)) disallowedTagDepth--;
+
+				if (disallowedTagDepth) return;
 
 				if (this.isSelfClosingTag(name)) return;
 				output.push(`</${name}>`);