diff --git a/extension/js/common/message-renderer.ts b/extension/js/common/message-renderer.ts index 5b7c1270beb..cf7cf679964 100644 --- a/extension/js/common/message-renderer.ts +++ b/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'); } } }); diff --git a/extension/js/common/platform/xss.ts b/extension/js/common/platform/xss.ts index 20cc682b07d..0c95ba1094f 100644 --- a/extension/js/common/platform/xss.ts +++ b/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 = ` -
+
Authenticity of this remote image cannot be verified.
`; 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 */ diff --git a/test/source/tests/browser-unit-tests/unit-Xss.js b/test/source/tests/browser-unit-tests/unit-Xss.js index 8b95ac0bf8c..d307ad39a74 100644 --- a/test/source/tests/browser-unit-tests/unit-Xss.js +++ b/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 = `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 = `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 = `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 = `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 = `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 = `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'; +})();