fix(security): scan <style> CSS past a CDATA-hidden </style>

scripts/lib/svg-active-content.mjs

@@ -210,11 +210,74 @@ const RESOURCE_ELEMENTS = new Set([
 const TAG_PATTERN =
   /<\s*([A-Za-z_][-A-Za-z0-9_.:]*)((?:"[^"]*"|'[^']*'|[^>"'])*)>/g;
 
-const STYLE_BLOCK_PATTERN = new RegExp(
-  `<\\s*${NS_PREFIX}style\\b(?:"[^"]*"|'[^']*'|[^>"'])*>([\\s\\S]*?)<\\s*/\\s*${NS_PREFIX}style\\s*>`,
+const STYLE_OPEN_TAG = new RegExp(
+  `<\\s*${NS_PREFIX}style\\b((?:"[^"]*"|'[^']*'|[^>"'])*)>`,
   'gi',
 );
 
+/** Sticky, so the close tag can be tested in place without slicing. */
+const STYLE_CLOSE_TAG = new RegExp(`<\\s*/\\s*${NS_PREFIX}style\\s*>`, 'iy');
+
+const CDATA_OPEN = '<![CDATA[';
+const CDATA_CLOSE = ']]>';
+
+/**
+ * The CSS text of every `<style>` element in `source`.
+ *
+ * A single lazy `<style ...>([\s\S]*?)</style>` regex cannot express this. In
+ * XML -- which is how a browser parses a standalone `.svg` -- a `</style>`
+ * inside a CDATA section is ordinary character data, so it does not close the
+ * element; the lazy match stops there anyway and every declaration after it
+ * goes unscanned, in a file that is well-formed and renders. That hid a live
+ * `url(https://host/...)` fetch from findRemoteReferences, which is the gate
+ * that keeps a visitor's IP, User-Agent and Referer from reaching a host the
+ * diagram's author chose.
+ *
+ * CDATA sections are therefore consumed whole before `<` is given any
+ * meaning. A `<style>` that never closes is read to the end of the document:
+ * that is what an HTML parser does, and in XML it is a fatal error, so
+ * scanning the remainder is the conservative reading under either grammar.
+ * A self-closing `<style/>` has no content and is skipped.
+ *
+ * @param {string} source - SVG file contents.
+ * @returns {string[]}
+ */
+function styleBlockContents(source) {
+  const blocks = [];
+  STYLE_OPEN_TAG.lastIndex = 0;
+  let open;
+  while ((open = STYLE_OPEN_TAG.exec(source))) {
+    const contentStart = open.index + open[0].length;
+    if ((open[1] ?? '').trimEnd().endsWith('/')) {
+      STYLE_OPEN_TAG.lastIndex = contentStart;
+      continue;
+    }
+
+    let cursor = contentStart;
+    let contentEnd = -1;
+    while (cursor < source.length) {
+      if (source.startsWith(CDATA_OPEN, cursor)) {
+        const close = source.indexOf(CDATA_CLOSE, cursor + CDATA_OPEN.length);
+        cursor = close === -1 ? source.length : close + CDATA_CLOSE.length;
+        continue;
+      }
+      if (source[cursor] === '<') {
+        STYLE_CLOSE_TAG.lastIndex = cursor;
+        if (STYLE_CLOSE_TAG.test(source)) {
+          contentEnd = cursor;
+          break;
+        }
+      }
+      cursor += 1;
+    }
+
+    const end = contentEnd === -1 ? source.length : contentEnd;
+    blocks.push(source.slice(contentStart, end));
+    STYLE_OPEN_TAG.lastIndex = end;
+  }
+  return blocks;
+}
+
 /** A CSS `url(...)` target, quoted or bare. Covers `@font-face` `src` too. */
 const CSS_URL_PATTERN = /url\(\s*(?:"([^"]*)"|'([^']*)'|([^)\s"']*))\s*\)/gi;
 
@@ -308,8 +371,8 @@ export function findRemoteReferences(source) {
     }
   }
 
-  for (const block of String(source).matchAll(STYLE_BLOCK_PATTERN)) {
-    for (const target of cssTargets(block[1] || '')) {
+  for (const block of styleBlockContents(String(source))) {
+    for (const target of cssTargets(block)) {
       findings.add(
         `references a remote resource in a <style> block: ${describeTarget(target)}`,
       );

tests/svg-active-content.test.mjs

@@ -538,16 +538,68 @@ test('an unquoted href is read as a value, not skipped', () => {
 });
 
 test('an empty <style> block does not break the block scan that follows it', () => {
-  // STYLE_BLOCK_PATTERN captures its body lazily, so an empty block yields an
-  // empty capture. Nothing may be reported for the block itself, and the rest
-  // of the document still has to be scanned.
+  // The block scan reads from the end of the open tag to the close tag, so an
+  // empty block yields empty CSS. Nothing may be reported for the block
+  // itself, and the rest of the document still has to be scanned.
   const svg =
     '<svg xmlns="http://www.w3.org/2000/svg"><style></style><image href="https://evil.example/after.png"/></svg>';
   assert.deepEqual(findRemoteReferences(svg), [
     'references a remote resource in <image> href: https://evil.example/after.png',
   ]);
 });
 
+test('a </style> inside a CDATA section does not end the <style> block', () => {
+  // A browser parses a standalone .svg as XML, where a `</style>` inside a
+  // CDATA section is character data and does not close the element. A lazy
+  // `<style ...>([\s\S]*?)</style>` match stops there anyway, so every
+  // declaration after it went unscanned in a file that is well-formed and
+  // renders -- a live remote fetch hidden from the gate that keeps a
+  // visitor's IP, User-Agent and Referer off a host the diagram chose.
+  const svg =
+    '<svg xmlns="http://www.w3.org/2000/svg"><style type="text/css"><![CDATA[' +
+    '/* </style> */ rect{fill:url(https://evil.example/track.svg#g)}' +
+    ']]></style><rect width="10" height="10"/></svg>';
+  assert.deepEqual(findRemoteReferences(svg), [
+    'references a remote resource in a <style> block: https://evil.example/track.svg#g',
+  ]);
+});
+
+test('a <style> block after a CDATA-hiding one is still scanned', () => {
+  // The scan resumes at the real close tag, not at the decoy, so a second
+  // block cannot be skipped by hiding a `</style>` in the first.
+  const svg =
+    '<svg xmlns="http://www.w3.org/2000/svg">' +
+    '<style><![CDATA[/* </style> */ a{fill:url(https://a.example/1.svg#g)}]]></style>' +
+    '<style>b{fill:url(https://b.example/2.svg#g)}</style></svg>';
+  assert.deepEqual(findRemoteReferences(svg), [
+    'references a remote resource in a <style> block: https://a.example/1.svg#g',
+    'references a remote resource in a <style> block: https://b.example/2.svg#g',
+  ]);
+});
+
+test('an unclosed <style> block is scanned to the end of the document', () => {
+  // An HTML parser runs an unclosed <style> to EOF and XML rejects the file
+  // outright, so reading the remainder is the conservative answer under
+  // either grammar. Stopping at a missing close tag reported nothing at all.
+  const svg =
+    '<svg xmlns="http://www.w3.org/2000/svg"><defs><style type="text/css">' +
+    'rect{fill:url(https://evil.example/late.svg#g)}';
+  assert.deepEqual(findRemoteReferences(svg), [
+    'references a remote resource in a <style> block: https://evil.example/late.svg#g',
+  ]);
+});
+
+test('a self-closing <style/> swallows no content', () => {
+  // `<style/>` has no body; treating what follows it as CSS would report a
+  // local `url(#id)` reference as remote-adjacent noise and, worse, would
+  // make the scan's idea of where CSS ends depend on later markup.
+  const svg =
+    '<svg xmlns="http://www.w3.org/2000/svg"><style /><image href="https://evil.example/after.png"/></svg>';
+  assert.deepEqual(findRemoteReferences(svg), [
+    'references a remote resource in <image> href: https://evil.example/after.png',
+  ]);
+});
+
 test('a long remote target is truncated so one URL cannot flood the output', () => {
   // A data-bearing URL can be arbitrarily long, and these findings are printed
   // one per line by the import and validation gates.