#6269 improve img srcset sanitation checks

extension/js/common/message-renderer.ts

@@ -102,6 +102,9 @@ export class MessageRenderer {
         if (contentIdAttachment) {
           inlineCIDAttachments.add(contentIdAttachment);
           currentNode.setAttribute('src', `data:${contentIdAttachment.type};base64,${contentIdAttachment.getData().toBase64Str()}`);
+          // a CID-backed inline image must not carry other resource-loading attributes (eg srcset) that
+          // could reference an external URL and trigger a remote request outside of the consent flow
+          currentNode.removeAttribute('srcset');
         }
       }
     });

extension/js/common/platform/xss.ts

@@ -196,14 +196,24 @@ export class Xss {
       if (node.tagName === 'IMG') {
         const img = node as HTMLImageElement; // Narrow type to HTMLImageElement
         const src = img.getAttribute('src');
+        const srcset = img.getAttribute('srcset');
+        // an image is remote when `src` or any `srcset` candidate references an external URL; `srcset`
+        // (and protocol-relative `//` URLs) must be covered too, otherwise they could be used to trigger
+        // a remote request that bypasses the remote-image consent flow
+        let remoteSrc: string | undefined;
+        if (src && Xss.isRemoteUrl(src)) {
+          remoteSrc = src;
+        } else if (srcset) {
+          remoteSrc = Xss.getRemoteUrlFromSrcset(srcset);
+        }
         if (imgHandling === 'IMG-DEL') {
           img.remove(); // just skip images
-        } else if (!src) {
-          img.remove(); // src that exists but is null is suspicious
-        } else if (imgHandling === 'IMG-KEEP' && checkValidURL(src)) {
+        } else if (!src && !srcset) {
+          img.remove(); // an image without any source is suspicious
+        } else if (imgHandling === 'IMG-KEEP' && remoteSrc) {
           // replace remote image with remote_image_container
           const remoteImgEl = `
-        <div class="remote_image_container" data-src="${Xss.escape(src)}" data-test="remote-image-container">
+        <div class="remote_image_container" data-src="${Xss.escape(remoteSrc)}" data-test="remote-image-container">
           <span>Authenticity of this remote image cannot be verified.</span>
         </div>`;
           Xss.replaceElementDANGEROUSLY(img, remoteImgEl); // xss-safe-value
@@ -360,6 +370,34 @@ export class Xss {
     return style.cssText;
   };
 
+  /**
+   * Check whether a URL would be fetched by the browser from a remote origin.
+   * Besides plain http(s) URLs this also covers protocol-relative URLs (eg `//attacker.example/x.png`),
+   * which the browser resolves against the current page's protocol but which `checkValidURL` misses.
+   */
+  private static isRemoteUrl = (url: string): boolean => {
+    const trimmed = url.trim();
+    return checkValidURL(trimmed) || trimmed.startsWith('//');
+  };
+
+  /**
+   * Return the first remote URL among the candidates of an `srcset` attribute.
+   * Each comma-separated candidate is a URL optionally followed by a descriptor (eg `1x`, `2x`, `640w`).
+   */
+  private static getRemoteUrlFromSrcset = (srcset: string): string | undefined => {
+    for (const candidate of srcset.split(',')) {
+      const trimmed = candidate.trim();
+      if (!trimmed) {
+        continue;
+      }
+      const url = trimmed.split(/\s+/)[0];
+      if (Xss.isRemoteUrl(url)) {
+        return url;
+      }
+    }
+    return undefined;
+  };
+
   /**
    * allow href links that have same origin as our extension + cid + inline image
    */

test/source/tests/browser-unit-tests/unit-Xss.js

@@ -79,3 +79,104 @@ BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeKeepBasicTags strips url() in CSS`);
   }
   return 'pass';
 })();
+
+BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeKeepBasicTags sends remote srcset on cid image to consent flow (IMG-KEEP)`);
+(async () => {
+  const dirty = `<img src="cid:known-cid" srcset="https://attacker.example/track.png 1x">message body`;
+  const clean = Xss.htmlSanitizeKeepBasicTags(dirty, 'IMG-KEEP');
+  const doc = new DOMParser().parseFromString(clean, 'text/html');
+  const imgs = Array.from(doc.querySelectorAll('img'));
+  for (const img of imgs) {
+    if (/^https?:/i.test(img.getAttribute('src') || '') || /^https?:/i.test(img.getAttribute('srcset') || '')) {
+      throw Error(`remote-loading img survived sanitization: ${clean}`);
+    }
+  }
+  const containers = doc.querySelectorAll('.remote_image_container');
+  if (containers.length !== 1) {
+    throw Error(`expected exactly one remote_image_container placeholder but got ${clean}`);
+  }
+  if (containers[0].getAttribute('data-src') !== 'https://attacker.example/track.png') {
+    throw Error(`unexpected data-src "${containers[0].getAttribute('data-src')}" in ${clean}`);
+  }
+  return 'pass';
+})();
+
+BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeKeepBasicTags sends multi-candidate remote srcset to consent flow (IMG-KEEP)`);
+(async () => {
+  const dirty = `<img src="cid:known-cid" srcset="cid:known-cid 1x, https://attacker.example/track.png 2x, https://attacker.example/track2.png 640w">message body`;
+  const clean = Xss.htmlSanitizeKeepBasicTags(dirty, 'IMG-KEEP');
+  const doc = new DOMParser().parseFromString(clean, 'text/html');
+  for (const img of Array.from(doc.querySelectorAll('img'))) {
+    if (/^https?:/i.test(img.getAttribute('src') || '') || /^https?:/i.test(img.getAttribute('srcset') || '')) {
+      throw Error(`remote-loading img survived sanitization: ${clean}`);
+    }
+  }
+  const containers = doc.querySelectorAll('.remote_image_container');
+  if (containers.length !== 1) {
+    throw Error(`expected exactly one remote_image_container placeholder but got ${clean}`);
+  }
+  return 'pass';
+})();
+
+BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeKeepBasicTags sends srcset-only remote image to consent flow (IMG-KEEP)`);
+(async () => {
+  const dirty = `<img srcset="https://attacker.example/track.png 1x, https://attacker.example/track2.png 2x">message body`;
+  const clean = Xss.htmlSanitizeKeepBasicTags(dirty, 'IMG-KEEP');
+  const doc = new DOMParser().parseFromString(clean, 'text/html');
+  for (const img of Array.from(doc.querySelectorAll('img'))) {
+    if (/^https?:/i.test(img.getAttribute('src') || '') || /^https?:/i.test(img.getAttribute('srcset') || '')) {
+      throw Error(`remote-loading img survived sanitization: ${clean}`);
+    }
+  }
+  const containers = doc.querySelectorAll('.remote_image_container');
+  if (containers.length !== 1) {
+    throw Error(`expected exactly one remote_image_container placeholder but got ${clean}`);
+  }
+  return 'pass';
+})();
+
+BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeKeepBasicTags sends protocol-relative remote src to consent flow (IMG-KEEP)`);
+(async () => {
+  const dirty = `<img src="//attacker.example/track.png">message body`;
+  const clean = Xss.htmlSanitizeKeepBasicTags(dirty, 'IMG-KEEP');
+  const doc = new DOMParser().parseFromString(clean, 'text/html');
+  for (const img of Array.from(doc.querySelectorAll('img'))) {
+    if (img.getAttribute('src') || img.getAttribute('srcset')) {
+      throw Error(`remote-loading img survived sanitization: ${clean}`);
+    }
+  }
+  const containers = doc.querySelectorAll('.remote_image_container');
+  if (containers.length !== 1) {
+    throw Error(`expected exactly one remote_image_container placeholder but got ${clean}`);
+  }
+  return 'pass';
+})();
+
+BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeKeepBasicTags keeps local data srcset images untouched (IMG-KEEP)`);
+(async () => {
+  const dirty = `<img src="data:image/png;base64,AAAA" srcset="data:image/png;base64,AAAA 1x, data:image/png;base64,BBBB 2x">message body`;
+  const clean = Xss.htmlSanitizeKeepBasicTags(dirty, 'IMG-KEEP');
+  const doc = new DOMParser().parseFromString(clean, 'text/html');
+  const containers = doc.querySelectorAll('.remote_image_container');
+  if (containers.length !== 0) {
+    throw Error(`local image unexpectedly converted to a placeholder: ${clean}`);
+  }
+  const imgs = Array.from(doc.querySelectorAll('img'));
+  if (imgs.length !== 1) {
+    throw Error(`expected the local image to be preserved: ${clean}`);
+  }
+  if (!imgs[0].getAttribute('srcset')) {
+    throw Error(`local srcset was dropped although it is not remote: ${clean}`);
+  }
+  return 'pass';
+})();
+
+BROWSER_UNIT_TEST_NAME(`Xss.htmlSanitizeAndStripAllTags leaks no remote srcset URL into text (IMG-TO-PLAIN-TEXT)`);
+(async () => {
+  const dirty = `<img src="cid:known-cid" srcset="https://attacker.example/track.png 1x">message body`;
+  const text = Xss.htmlUnescape(Xss.htmlSanitizeAndStripAllTags(dirty, '\n', false));
+  if (text.includes('https://attacker.example') || text.includes('srcset')) {
+    throw Error(`remote srcset URL leaked into plain text: ${text}`);
+  }
+  return 'pass';
+})();