Fix HTML filter bypasses in Markdown preview, and harden further (#1694)

Author: charmander

assets/js/defang.js

@@ -14,10 +14,14 @@ const allowedTags = new Set([
     'A', 'IMG',
 ]);
 
-const allowedAttributes = new Set([
+export const allowedAttributes = new Set([
     'title', 'alt', 'colspan', 'rowspan', 'start', 'type',
 ]);
 
+export const conditionallyAllowedAttributes = [
+    'href', 'src', 'style', 'class',
+];
+
 const allowedSchemes = new Set([
     'http:', 'https:', 'mailto:', 'irc:', 'ircs:', 'magnet:',
 ]);
@@ -66,22 +70,48 @@ export const tryGetCleanHref = s => {
     return u.pathname;
 };
 
-const defang = (node, isBody) => {
-    for (let i = node.childNodes.length; i--;) {
-        const child = node.childNodes[i];
+// clobbering-safe access
+const NP = Node.prototype;
+const npGetter = k => Object.getOwnPropertyDescriptor(NP, k).get;
+const getFirstChild = npGetter('firstChild');
+const getNextSibling = npGetter('nextSibling');
 
-        if (child.nodeType === 1) {
-            defang(child, false);
+const defang = (node, isBody) => {
+    for (let child = getFirstChild.call(node); child;) {
+        // `child` may be removed (and possibly replaced with multiple nodes) either by recursive `defang` or `default` case
+        const nextSibling = getNextSibling.call(child);
+
+        switch (child.nodeType) {
+            case 1:
+                defang(child, false);
+                break;
+
+            case 3:
+                // text nodes are always allowed
+                break;
+
+            default:
+                // NOTE: also covers clobbered `nodeType`
+                NP.removeChild.call(node, child);
         }
+
+        child = nextSibling;
     }
 
-    if (!isBody && !allowedTags.has(node.nodeName)) {
-        while (node.hasChildNodes()) {
-            node.parentNode.insertBefore(node.firstChild, node);
+    // NOTE: also covers clobbered `nodeName`, and no `allowedTags` elements support named properties, so `hasAttribute` won't even throw
+    if (!isBody && (!allowedTags.has(node.nodeName) || node.hasAttribute('is'))) {
+        // n.b. merging this with the previous loop would be easy to get dangerously wrong
+        const childNodes = document.createDocumentFragment();
+
+        for (let child = getFirstChild.call(node); child;) {
+            const nextSibling = getNextSibling.call(child);
+            childNodes.appendChild(child);
+            child = nextSibling;
         }
 
-        node.parentNode.removeChild(node);
+        Element.prototype.replaceWith.call(node, childNodes);
     } else {
+        // no `allowedTags` elements support named properties, so `node`'s properties are unclobbered in this branch
         for (let i = node.attributes.length; i--;) {
             const attribute = node.attributes[i];
             let cleanHref;

assets/js/scripts.js

@@ -1,6 +1,6 @@
 /* global marked */
 import autosize_ from 'autosize';
-import defang from './defang.js';
+import defang, {allowedAttributes, conditionallyAllowedAttributes} from './defang.js';
 import {byClass} from './dom.js';
 import initEmbed from './embed.js';
 import {forEach, some} from './util/array-like.js';
@@ -222,10 +222,36 @@ const loadMarked = () => {
     document.body.appendChild(markedScript);
 };
 
+// Clobbering-safe access. A document *from `DOMParser` specifically* doesn't seem to have named property access on Chromium 146, but nothing in the DOM/HTML/`DOMParser` specs distinguish that case as far as I can tell; anyway, it does have named property access on Firefox 149.
+const getBody = Object.getOwnPropertyDescriptor(Document.prototype, 'body').get;
+
+/**
+ * Parses a string of HTML into a partially sanitized container element if the environment supports the HTML Sanitizer API, and an unsanitized one otherwise.
+ *
+ * This partial sanitization removes attributes that are never allowed on any element (especially `name` and `id`, preventing clobbering) and anything that can execute scripts (try not to reintroduce such things in `weasylMarkdown`) as defense in depth. Unfortunately, `<script>`, `<iframe>`, and `<object>` elements are completely removed, instead of behaving like `defang`; configuring these as `replaceWithChildrenElements` doesn't work with "safe" functions. `javascript:` links are also removed instead of getting the `replaceBadLinks` treatment.
+ */
+let parseHtml;
+
+if (Document.parseHTML) {
+    // some of this configuration is already implied by the use of a "safe" function like `parseHTML`, but it's nice to be explicit
+    const presanitizer = new Sanitizer({
+        // NOTE: does not remove `is`
+        attributes: [...allowedAttributes, ...conditionallyAllowedAttributes],
+        comments: false,
+    });
+    presanitizer.removeUnsafe();
+
+    parseHtml = html => Document.parseHTML(html, {sanitizer: presanitizer}).body;
+} else {
+    parseHtml = html => {
+        const doc = new DOMParser().parseFromString(html, 'text/html');
+        return getBody.call(doc);
+    };
+}
+
 const renderMarkdown = (content, container) => {
-    const markdown = marked(content, markdownOptions);
-    const sanitizeDocument = new DOMParser().parseFromString(markdown, 'text/html');
-    const fragment = sanitizeDocument.body;
+    const renderedMarkdown = marked(content, markdownOptions);
+    const fragment = parseHtml(renderedMarkdown);
 
     weasylMarkdown(fragment);
     defang(fragment, true);

assets/js/weasyl-markdown.js

@@ -25,6 +25,7 @@ const replaceBadLinks = fragment => {
     });
 };
 
+// XXX: Properties of `fragment` and `child` can be clobbered here. For now, they just break the preview or user links at worst, which is "fine".
 const addUserLinks = fragment => {
     for (let i = 0; i < fragment.childNodes.length; i++) {
         const child = fragment.childNodes[i];
@@ -92,7 +93,7 @@ const weasylMarkdown = fragment => {
     const links = fragment.getElementsByTagName('a');
 
     forEach(links, link => {
-        const href = link.getAttribute('href');
+        const href = link.getAttribute('href') || '';
         const i = href.indexOf(':');
         const scheme = href.substring(0, i);
         const user = href.substring(i + 1);
@@ -130,19 +131,20 @@ const weasylMarkdown = fragment => {
     const images = fragment.querySelectorAll('img');
 
     forEach(images, image => {
-        const src = image.getAttribute('src');
+        const src = image.getAttribute('src') || '';
         const i = src.indexOf(':');
         const scheme = src.substring(0, i);
         const link = document.createElement('a');
 
+        image.replaceWith(link);
+
         if (scheme === 'user') {
             const user = src.substring(i + 1);
             image.className = 'user-icon';
             image.src = '/~' + user + '/avatar';
 
             link.href = '/~' + user;
 
-            image.parentNode.replaceChild(link, image);
             link.appendChild(image);
 
             if (image.alt) {
@@ -157,10 +159,8 @@ const weasylMarkdown = fragment => {
                 image.title = '';
             }
         } else {
-            link.href = image.src;
+            link.href = image.src;  // XXX: reintroduction point for `javascript:` URLs after presanitizer
             link.appendChild(document.createTextNode(image.alt || image.src));
-
-            image.parentNode.replaceChild(link, image);
         }
     });