CVE-2024-56510

@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+### Security
+
+- Improper neutralization of HTML sanitization by comments may lead to XSS (by [@Ry0taK](https://github.com/Ry0taK))
+
 ### Changed
 
 - Upgrade Marpit to [v3.1.2](https://github.com/marp-team/marpit/releases/v3.1.2) ([#390](https://github.com/marp-team/marp-core/pull/390))

@@ -42,201 +42,204 @@ const srcSetSanitizer = (value: string): string => {
   return value
 }
 
-export const defaultHTMLAllowList = {
-  a: {
-    ...globalAttrs,
-    href: webUrlSanitizer,
-    name: true, // deprecated attribute, but still useful in Marp for making stable anchor link
-    rel: true,
-    target: true,
-  },
-  abbr: globalAttrs,
-  address: globalAttrs,
-  article: globalAttrs,
-  aside: globalAttrs,
-  audio: {
-    ...globalAttrs,
-    autoplay: true,
-    controls: true,
-    loop: true,
-    muted: true,
-    preload: true,
-    src: webUrlSanitizer,
-  },
-  b: globalAttrs,
-  bdi: globalAttrs,
-  bdo: globalAttrs,
-  big: globalAttrs,
-  blockquote: {
-    ...globalAttrs,
-    cite: webUrlSanitizer,
-  },
-  br: globalAttrs,
-  caption: globalAttrs,
-  center: globalAttrs, // deprecated
-  cite: globalAttrs,
-  code: globalAttrs,
-  col: {
-    ...globalAttrs,
-    align: true,
-    valign: true,
-    span: true,
-    width: true,
-  },
-  colgroup: {
-    ...globalAttrs,
-    align: true,
-    valign: true,
-    span: true,
-    width: true,
-  },
-  dd: globalAttrs,
-  del: {
-    ...globalAttrs,
-    cite: webUrlSanitizer,
-    datetime: true,
-  },
-  details: {
-    ...globalAttrs,
-    open: true,
-  },
-  div: globalAttrs,
-  dl: globalAttrs,
-  dt: globalAttrs,
-  em: globalAttrs,
-  figcaption: globalAttrs,
-  figure: globalAttrs,
-  // footer: globalAttrs, // Inserted by Marpit directives so disallowed to avoid confusion
-  h1: globalAttrs,
-  h2: globalAttrs,
-  h3: globalAttrs,
-  h4: globalAttrs,
-  h5: globalAttrs,
-  h6: globalAttrs,
-  // header: globalAttrs, // Inserted by Marpit directives so disallowed to avoid confusion
-  hr: globalAttrs,
-  i: globalAttrs,
-  img: {
-    ...globalAttrs,
-    align: true, // deprecated attribute, but still useful in Marp for aligning image
-    alt: true,
-    decoding: true,
-    height: true,
-    loading: true,
-    src: imageUrlSanitizer,
-    srcset: srcSetSanitizer,
-    title: true,
-    width: true,
-  },
-  ins: {
-    ...globalAttrs,
-    cite: webUrlSanitizer,
-    datetime: true,
-  },
-  kbd: globalAttrs,
-  li: {
-    ...globalAttrs,
-    type: true,
-    value: true,
-  },
-  mark: globalAttrs,
-  nav: globalAttrs,
-  ol: {
-    ...globalAttrs,
-    reversed: true,
-    start: true,
-    type: true,
-  },
-  p: globalAttrs,
-  picture: globalAttrs,
-  pre: globalAttrs,
-  source: {
-    height: true,
-    media: true,
-    sizes: true,
-    src: imageUrlSanitizer,
-    srcset: srcSetSanitizer,
-    type: true,
-    width: true,
-  },
-  q: {
-    ...globalAttrs,
-    cite: webUrlSanitizer,
-  },
-  rp: globalAttrs,
-  rt: globalAttrs,
-  ruby: globalAttrs,
-  s: globalAttrs,
-  section: globalAttrs,
-  small: globalAttrs,
-  span: globalAttrs,
-  sub: globalAttrs,
-  summary: globalAttrs,
-  sup: globalAttrs,
-  strong: globalAttrs,
-  strike: globalAttrs,
-  table: {
-    ...globalAttrs,
-    width: true,
-    border: true,
-    align: true,
-    valign: true,
-  },
-  tbody: {
-    ...globalAttrs,
-    align: true,
-    valign: true,
-  },
-  td: {
-    ...globalAttrs,
-    width: true,
-    rowspan: true,
-    colspan: true,
-    align: true,
-    valign: true,
-  },
-  tfoot: {
-    ...globalAttrs,
-    align: true,
-    valign: true,
-  },
-  th: {
-    ...globalAttrs,
-    width: true,
-    rowspan: true,
-    colspan: true,
-    align: true,
-    valign: true,
-  },
-  thead: {
-    ...globalAttrs,
-    align: true,
-    valign: true,
-  },
-  time: {
-    ...globalAttrs,
-    datetime: true,
-  },
-  tr: {
-    ...globalAttrs,
-    rowspan: true,
-    align: true,
-    valign: true,
-  },
-  u: globalAttrs,
-  ul: globalAttrs,
-  video: {
-    ...globalAttrs,
-    autoplay: true,
-    controls: true,
-    loop: true,
-    muted: true,
-    playsinline: true,
-    poster: imageUrlSanitizer,
-    preload: true,
-    src: webUrlSanitizer,
-    height: true,
-    width: true,
-  },
-  wbr: globalAttrs,
-} as const satisfies HTMLAllowList
+export const defaultHTMLAllowList: HTMLAllowList = Object.assign(
+  Object.create(null),
+  {
+    a: {
+      ...globalAttrs,
+      href: webUrlSanitizer,
+      name: true, // deprecated attribute, but still useful in Marp for making stable anchor link
+      rel: true,
+      target: true,
+    },
+    abbr: globalAttrs,
+    address: globalAttrs,
+    article: globalAttrs,
+    aside: globalAttrs,
+    audio: {
+      ...globalAttrs,
+      autoplay: true,
+      controls: true,
+      loop: true,
+      muted: true,
+      preload: true,
+      src: webUrlSanitizer,
+    },
+    b: globalAttrs,
+    bdi: globalAttrs,
+    bdo: globalAttrs,
+    big: globalAttrs,
+    blockquote: {
+      ...globalAttrs,
+      cite: webUrlSanitizer,
+    },
+    br: globalAttrs,
+    caption: globalAttrs,
+    center: globalAttrs, // deprecated
+    cite: globalAttrs,
+    code: globalAttrs,
+    col: {
+      ...globalAttrs,
+      align: true,
+      valign: true,
+      span: true,
+      width: true,
+    },
+    colgroup: {
+      ...globalAttrs,
+      align: true,
+      valign: true,
+      span: true,
+      width: true,
+    },
+    dd: globalAttrs,
+    del: {
+      ...globalAttrs,
+      cite: webUrlSanitizer,
+      datetime: true,
+    },
+    details: {
+      ...globalAttrs,
+      open: true,
+    },
+    div: globalAttrs,
+    dl: globalAttrs,
+    dt: globalAttrs,
+    em: globalAttrs,
+    figcaption: globalAttrs,
+    figure: globalAttrs,
+    // footer: globalAttrs, // Inserted by Marpit directives so disallowed to avoid confusion
+    h1: globalAttrs,
+    h2: globalAttrs,
+    h3: globalAttrs,
+    h4: globalAttrs,
+    h5: globalAttrs,
+    h6: globalAttrs,
+    // header: globalAttrs, // Inserted by Marpit directives so disallowed to avoid confusion
+    hr: globalAttrs,
+    i: globalAttrs,
+    img: {
+      ...globalAttrs,
+      align: true, // deprecated attribute, but still useful in Marp for aligning image
+      alt: true,
+      decoding: true,
+      height: true,
+      loading: true,
+      src: imageUrlSanitizer,
+      srcset: srcSetSanitizer,
+      title: true,
+      width: true,
+    },
+    ins: {
+      ...globalAttrs,
+      cite: webUrlSanitizer,
+      datetime: true,
+    },
+    kbd: globalAttrs,
+    li: {
+      ...globalAttrs,
+      type: true,
+      value: true,
+    },
+    mark: globalAttrs,
+    nav: globalAttrs,
+    ol: {
+      ...globalAttrs,
+      reversed: true,
+      start: true,
+      type: true,
+    },
+    p: globalAttrs,
+    picture: globalAttrs,
+    pre: globalAttrs,
+    source: {
+      height: true,
+      media: true,
+      sizes: true,
+      src: imageUrlSanitizer,
+      srcset: srcSetSanitizer,
+      type: true,
+      width: true,
+    },
+    q: {
+      ...globalAttrs,
+      cite: webUrlSanitizer,
+    },
+    rp: globalAttrs,
+    rt: globalAttrs,
+    ruby: globalAttrs,
+    s: globalAttrs,
+    section: globalAttrs,
+    small: globalAttrs,
+    span: globalAttrs,
+    sub: globalAttrs,
+    summary: globalAttrs,
+    sup: globalAttrs,
+    strong: globalAttrs,
+    strike: globalAttrs,
+    table: {
+      ...globalAttrs,
+      width: true,
+      border: true,
+      align: true,
+      valign: true,
+    },
+    tbody: {
+      ...globalAttrs,
+      align: true,
+      valign: true,
+    },
+    td: {
+      ...globalAttrs,
+      width: true,
+      rowspan: true,
+      colspan: true,
+      align: true,
+      valign: true,
+    },
+    tfoot: {
+      ...globalAttrs,
+      align: true,
+      valign: true,
+    },
+    th: {
+      ...globalAttrs,
+      width: true,
+      rowspan: true,
+      colspan: true,
+      align: true,
+      valign: true,
+    },
+    thead: {
+      ...globalAttrs,
+      align: true,
+      valign: true,
+    },
+    time: {
+      ...globalAttrs,
+      datetime: true,
+    },
+    tr: {
+      ...globalAttrs,
+      rowspan: true,
+      align: true,
+      valign: true,
+    },
+    u: globalAttrs,
+    ul: globalAttrs,
+    video: {
+      ...globalAttrs,
+      autoplay: true,
+      controls: true,
+      loop: true,
+      muted: true,
+      playsinline: true,
+      poster: imageUrlSanitizer,
+      preload: true,
+      src: webUrlSanitizer,
+      height: true,
+      width: true,
+    },
+    wbr: globalAttrs,
+  } as const satisfies HTMLAllowList,
+)

@@ -1,5 +1,6 @@
 import selfClosingTags from 'self-closing-tags'
 import { FilterXSS, friendlyAttrValue, escapeAttrValue } from 'xss'
+import type { SafeAttrValueHandler, IWhiteList } from 'xss'
 import { MarpOptions } from '../marp'
 
 const selfClosingRegexp = /\s*\/?>$/
@@ -14,82 +15,103 @@ const xhtmlOutFilter = new FilterXSS({
   allowList: {},
 })
 
+// Prevent breaking JavaScript special characters such as `<` and `>` by HTML
+// escape process only if the entire content of HTML block is consisted of
+// script tag (The case of matching the case 1 of https://spec.commonmark.org/0.31.2/#html-blocks,
+// with special condition for <script> tag)
+//
+// For cases like https://spec.commonmark.org/0.31.2/#example-178, which do not
+// end the HTML block with `</script>`, that will not exclude from sanitizing.
+//
+const scriptBlockRegexp =
+  /^<script(?:>|[ \t\f\n\r][\s\S]*?>)([\s\S]*)<\/script>[ \t\f\n\r]*$/i
+
+const scriptBlockContentUnexpectedCloseRegexp = /<\/script[>/\t\f\n\r ]/i
+
+const isValidScriptBlock = (htmlBlockContent: string) => {
+  const m = htmlBlockContent.match(scriptBlockRegexp)
+  return !!(m && !scriptBlockContentUnexpectedCloseRegexp.test(m[1]))
+}
+
 export function markdown(md): void {
   const { html_inline, html_block } = md.renderer.rules
 
-  const sanitizedRenderer =
-    (original: (...args: any[]) => string) =>
-    (...args) => {
-      const ret = original(...args)
-
-      // Pick comments
-      const splitted: string[] = []
-      let pos = 0
+  const fetchHtmlOption = (): MarpOptions['html'] => md.options.html
+  const fetchAllowList = (html = fetchHtmlOption()): IWhiteList => {
+    const allowList: IWhiteList = Object.create(null)
 
-      while (pos < ret.length) {
-        const startIdx = ret.indexOf('<!--', pos)
-        let endIdx = startIdx !== -1 ? ret.indexOf('-->', startIdx + 4) : -1
+    if (typeof html === 'object') {
+      for (const tag of Object.keys(html)) {
+        const attrs = html[tag]
 
-        if (endIdx === -1) {
-          splitted.push(ret.slice(pos))
-          break
+        if (Array.isArray(attrs)) {
+          allowList[tag] = attrs
+        } else if (typeof attrs === 'object') {
+          allowList[tag] = Object.keys(attrs).filter(
+            (attr) => attrs[attr] !== false,
+          )
         }
-
-        endIdx += 3
-        splitted.push(ret.slice(pos, startIdx), ret.slice(startIdx, endIdx))
-        pos = endIdx
       }
-
-      // Apply filter to each contents by XSS
-      const allowList = {}
-      const html: MarpOptions['html'] = md.options.html
-
-      if (typeof html === 'object') {
-        for (const tag of Object.keys(html)) {
-          const attrs = html[tag]
-
-          if (Array.isArray(attrs)) {
-            allowList[tag] = attrs
-          } else if (typeof attrs === 'object') {
-            allowList[tag] = Object.keys(attrs).filter(
-              (attr) => attrs[attr] !== false,
-            )
-          }
-        }
+    }
+    return allowList
+  }
+
+  const generateSafeAttrValueHandler =
+    (html = fetchHtmlOption()): SafeAttrValueHandler =>
+    (tag, attr, value) => {
+      let ret = friendlyAttrValue(value)
+
+      if (
+        typeof html === 'object' &&
+        html[tag] &&
+        !Array.isArray(html[tag]) &&
+        typeof html[tag][attr] === 'function'
+      ) {
+        ret = html[tag][attr](ret)
       }
 
-      const filter = new FilterXSS({
-        allowList,
-        onIgnoreTag: (_, rawHtml) => (html === true ? rawHtml : undefined),
-        safeAttrValue: (tag, attr, value) => {
-          let ret = friendlyAttrValue(value)
-
-          if (
-            typeof html === 'object' &&
-            html[tag] &&
-            !Array.isArray(html[tag]) &&
-            typeof html[tag][attr] === 'function'
-          ) {
-            ret = html[tag][attr](ret)
-          }
-
-          return escapeAttrValue(ret)
+      return escapeAttrValue(ret)
+    }
+
+  const sanitize = (ret: string) => {
+    const html = fetchHtmlOption()
+    const filter = new FilterXSS({
+      allowList: fetchAllowList(html),
+      onIgnoreTag: (_, rawHtml) => (html === true ? rawHtml : undefined),
+      safeAttrValue: generateSafeAttrValueHandler(html),
+    })
+
+    const sanitized = filter.process(ret)
+    return md.options.xhtmlOut ? xhtmlOutFilter.process(sanitized) : sanitized
+  }
+
+  md.renderer.rules.html_inline = (...args) => sanitize(html_inline(...args))
+  md.renderer.rules.html_block = (...args) => {
+    const ret = html_block(...args)
+    const html = fetchHtmlOption()
+
+    const scriptAllowAttrs = (() => {
+      if (html === true) return []
+      if (typeof html === 'object' && html['script'])
+        return fetchAllowList({ script: html.script }).script
+    })()
+
+    // If the entire content of HTML block is consisted of script tag when the
+    // script tag is allowed, we will not escape the content of the script tag.
+    if (scriptAllowAttrs && isValidScriptBlock(ret)) {
+      const scriptFilter = new FilterXSS({
+        allowList: { script: scriptAllowAttrs || [] },
+        allowCommentTag: true,
+        onIgnoreTagAttr: (_, name, value) => {
+          if (html === true) return `${name}="${escapeAttrValue(value)}"`
         },
+        escapeHtml: (s) => s,
+        safeAttrValue: generateSafeAttrValueHandler(html),
       })
 
-      return splitted
-        .map((part, idx) => {
-          if (idx % 2 === 1) return part
-
-          const sanitized = filter.process(part)
-
-          return md.options.xhtmlOut
-            ? xhtmlOutFilter.process(sanitized)
-            : sanitized
-        })
-        .join('')
+      return scriptFilter.process(ret)
     }
 
-  md.renderer.rules.html_inline = sanitizedRenderer(html_inline)
-  md.renderer.rules.html_block = sanitizedRenderer(html_block)
+    return sanitize(ret)
+  }
 }

@@ -391,44 +391,6 @@ describe('Marp', () => {
         expect($('footer > em')).toHaveLength(1)
       })
 
-      it('keeps raw HTML comments within valid HTML block', () => {
-        const { html: $script, comments: comments$script } = marp().render(
-          "<script><!--\nconst script = '<b>test</b>'\n--></script>",
-        )
-        expect($script).toContain("const script = '<b>test</b>'")
-        expect(comments$script[0]).toHaveLength(0)
-
-        // Complex comment
-        const complexComment = `
-<!--
-function matchwo(a,b)
-{
-
-  if (a < b && a < 0) then {
-    return 1;
-
-  } else {
-
-    return 0;
-  }
-}
-
-// ex
--->
-`.trim()
-        const { html: $complex } = marp().render(
-          `<script>${complexComment}</script>`,
-        )
-        expect($complex).toContain(complexComment)
-
-        // NOTE: Marpit framework will collect the comment block if the whole of HTML block was comment
-        const { html: $comment, comments: comments$comment } = marp().render(
-          "<!--\nconst script = '<b>test</b>'\n-->",
-        )
-        expect($comment).not.toContain("const script = '<b>test</b>'")
-        expect(comments$comment[0]).toHaveLength(1)
-      })
-
       it('sanitizes CDATA section', () => {
         // HTML Living Standard denys using CDATA in HTML context so must be sanitized
         const cdata = `
@@ -463,6 +425,66 @@ function matchwo(a,b)
           "<br class='normalize' />",
         )
       })
+
+      it('does not escape JavaScript special character within valid <script> HTML block', () => {
+        const { html: $script, comments: comments$script } = m.render(
+          "<script><!--\nconst script = '<b>test</b>'\n--></script>",
+        )
+        expect($script).toContain("const script = '<b>test</b>'")
+        expect(comments$script[0]).toHaveLength(0)
+
+        // Complex comment
+        const complexComment = `
+<!--
+function complex(a,b)
+{
+
+  if (a < b && a < 0) then {
+    return 1;
+
+  } else {
+
+    return 0;
+  }
+}
+
+// ex
+>
+`.trim()
+        const { html: $complex } = m.render(
+          `<script>${complexComment}</script>`,
+        )
+        expect($complex).toContain(complexComment)
+
+        // Case-insensitive tag names, attributes, and script without comment
+        const attrsAndScriptWithoutComment = `
+<SCRIPT
+  type="text/javascript"
+  data-script="true">
+    console.log(2 > 1 && 1 < 2)
+</Script>
+`.trim()
+        const { html: $attrAndScript } = m.render(attrsAndScriptWithoutComment)
+        expect($attrAndScript).toContain(
+          '<script type="text/javascript" data-script="true">',
+        )
+        expect($attrAndScript).toContain('console.log(2 > 1 && 1 < 2)')
+      })
+
+      it('does escape JavaScript special character if <script> HTML block has trailing contents', () => {
+        // ref: https://spec.commonmark.org/0.31.2/#example-178
+        const withTrailingContents = `
+<script>
+  console.log(2 > 1);
+</script> trailing <a href="https://example.com">link</a>
+`.trim()
+        const { html } = m.render(withTrailingContents)
+
+        expect(html).toContain('console.log(2 &gt; 1);')
+        expect(html).toContain(
+          '</script> trailing <a href="https://example.com">link</a>',
+        )
+      })
     })
 
     describe('with false', () => {
@@ -517,6 +539,79 @@ function matchwo(a,b)
           expect(html).toContain('<p id="sanitized"></p>')
         })
       })
+
+      describe('when <script> tag is allowed', () => {
+        const m = marp({ html: { script: ['type'] } })
+
+        it('does not escape JavaScript special character within valid <script> HTML block', () => {
+          const { html: $script, comments: comments$script } = m.render(
+            "<script><!--\nconst script = '<b>test</b>'\n--></script>",
+          )
+          expect($script).toContain("const script = '<b>test</b>'")
+          expect(comments$script[0]).toHaveLength(0)
+
+          // Complex comment
+          const complexComment = `
+  <!--
+  function complex(a,b)
+  {
+
+    if (a < b && a < 0) then {
+      return 1;
+
+    } else {
+
+      return 0;
+    }
+  }
+
+  // ex
+  >
+  `.trim()
+          const { html: $complex } = m.render(
+            `<script>${complexComment}</script>`,
+          )
+          expect($complex).toContain(complexComment)
+
+          // Case-insensitive tag names, attributes w/ filter, and script without comment
+          const attrsAndScriptWithoutComment = `
+<SCRIPT
+  type="text/javascript"
+  data-script="true">
+    console.log(2 > 1 && 1 < 2)
+</Script>
+`.trim()
+          const { html: $attrAndScript } = m.render(
+            attrsAndScriptWithoutComment,
+          )
+          expect($attrAndScript).toContain('<script type="text/javascript">')
+          expect($attrAndScript).toContain('console.log(2 > 1 && 1 < 2)')
+
+          // Including incorrect closing (may be malicious)
+          const { html: $incorrectClosing } = m.render(
+            '<script></script><b>bypass whitelist</b></script>',
+          )
+          expect($incorrectClosing).toContain(
+            '&lt;b&gt;bypass whitelist&lt;/b&gt;',
+          )
+        })
+
+        it('does escape JavaScript special character if <script> HTML block has trailing contents', () => {
+          // ref: https://spec.commonmark.org/0.31.2/#example-178
+          const withTrailingContents = `
+  <script>
+    console.log(2 > 1);
+  </script> trailing <a href="https://example.com">link</a>
+  `.trim()
+          const { html } = m.render(withTrailingContents)
+
+          expect(html).toContain('console.log(2 &gt; 1);')
+          expect(html).toContain(
+            // Follow allowlist
+            '</script> trailing &lt;a href="https://example.com"&gt;link&lt;/a&gt;',
+          )
+        })
+      })
     })
 
     describe("with markdown-it's xhtmlOut option as false", () => {