Skip to content

Webmail: block remote CSS fetches when remote images are off#222

Open
ahmedhesham6 wants to merge 2 commits into
oblien:mainfrom
ahmedhesham6:fix/webmail-css-remote-resource-blocking
Open

Webmail: block remote CSS fetches when remote images are off#222
ahmedhesham6 wants to merge 2 commits into
oblien:mainfrom
ahmedhesham6:fix/webmail-css-remote-resource-blocking

Conversation

@ahmedhesham6

Copy link
Copy Markdown
Contributor

Summary

With remote images off, processEmailContent rewrote only <img src>. The sanitizer allows <style> and style attributes, so CSS url(), @import, image-set() — and <img srcset> — all still fetched. Any one of them gave a sender a read receipt, timestamp, and IP from a reader the UI reported as protected.

This PR adds blockRemoteContent() in sanitize.ts as the single entry point for the preference and reduces the route to sanitize → block → return.

What it blocks

  • <img src> — remote sources become the existing 1×1 placeholder pixel; data:/cid: sources are left alone so inline attachments keep rendering.
  • <img srcset> — dropped whole when any candidate is remote, since srcset outranks src and would undo the rewrite. Candidates are parsed per the HTML rules (commas inside data: URLs are not separators).
  • CSS url() — rewritten to none, keeping the rest of the declaration valid; data:/cid: exempt.
  • CSS @import — dropped entirely, both url() and bare-string forms.
  • CSS image-set() — remote string arguments replaced with an inert data:,, since they fetch with no url( token.

Covers both <style> bodies and style attributes (with the entity encoding sanitize-html applies to attribute values handled correctly).

Hardening details

  • CSS comments are normalized to whitespace first, so url(/*x*/https://…) cannot split the token past the patterns.
  • CSS escapes of ASCII alphanumerics are decoded first, so \75 rl(…) and @\69 mport cannot dodge the patterns. Other escapes are untouched.
  • The comment and @import passes are hand-written linear scanners: the natural regexes backtrack quadratically on crafted input (/*a/*a…, repeated unclosed url(), which server-side is a denial-of-service lever.
  • One shared inline-scheme definition (data:/cid:) backs every exemption so the passes cannot drift.
  • Fast paths: mail with no style content, and CSS with no (, @, or \, skip the work and pass through byte-identical.

Blocking any of it sets the existing hasBlockedImages flag, so the current "remote content blocked" banner covers CSS too with no client change.

Notes

  • Regex-over-HTML is best-effort by design; a follow-up worth considering is rendering the read pane in a CSP-restricted iframe so the browser enforces no-fetch and this pass becomes defense-in-depth.
  • No new dependencies.

Closes #220

With remote images off, only <img src> was rewritten. The sanitizer
allows <style> and style attributes, so url(), @import, image-set()
and img srcset all still fetched — enough for a sender to collect a
read receipt, timestamp, and IP from a reader the UI reported as
protected.

blockRemoteContent() in sanitize.ts now covers every fetch surface in
one pass: remote img src becomes the placeholder pixel, srcset with
any remote candidate is dropped, and CSS loses url()/@import/
image-set() targets unless they are data: or cid:. CSS comments and
escapes are normalized first so split or escaped spellings cannot
smuggle a fetch past the patterns, and the comment/@import passes are
linear scanners because the natural regexes backtrack quadratically
on crafted input.

Closes oblien#220
@Hydralerne

Copy link
Copy Markdown
Member

thank you for your support
how ever, i found some issues in the pr

  • Bypasses (remote fetch survives, no banner)
  1. image-set() bare string containing ; { or } - HIGH
<style>a{background:image-set("http://evil.com/a;b" 1x)}</style> → unchanged, blocked=false

CSS_IMAGE_SET = /…image-set([^;{}]*/ truncates its span at the first ;/{/} - but those are ordinary characters inside a CSS string token, so the browser parses the whole URL and fetches. Worst part: the realistic data: fallback + remote sibling hits this every time (the ; in ;base64 truncates the span before the remote candidate):
image-set("data:image/png;base64,AAA" 1x, "http://evil.com/a" 2x) → remote 2x leaks

  1. image-set() via escaped hyphen - HIGH
<style>a{background:image\2dset("http://evil.com/x" 1x)}</style> → unchanged, blocked=false

CSS_IMAGE_SET keys on the literal substring image-set(; image\2dset( doesn't match, but the browser resolves \2d→- while building the ident and forms a real image-set( function. (The url() form is safe here - CSS_REMOTE_URL is ident-independent - so only the bare-string form leaks.)

  1. Decoy src= inside alt/title shields the real remote src - HIGH
    x src='data:,' → unchanged, blocked=false
    The src rewrite /(\bsrc\s*=\s*)("[^"]"|'[^']')/i is non-global, and \b matches the src= inside the alt value (the space before it is a word boundary). It rewrites that first (inline, so it's left as-is) and stops - the genuine src="https://evil.com/track" is never inspected. This is the nastiest one: it breaks the plain case, which is the whole original feature. (data-src is safe only because the allowlist strips it - this is the same class of bug the allowlist happened to cover, but alt/title are preserved verbatim.)

also Correctness (false positives / broken render)

  • D1 (med): inline image-set("data:a" 1x, "data:b" 2x) → mangled to image-set("data:a"data:,"data:b" 2x) and falsely blocked=true. CSS_REMOTE_STRING re-anchors on the closing quote of an exempt string. Destroys a legit zero-fetch retina background - violates the PR's own "inline data: survives" invariant.
  • D2 (low): content:"@import" (the literal substring in a string) → stripImports deletes to EOF, unbalancing the block; blocked=true false positive. stripImports doesn't skip string/url() contents.
  • D3 (low): fill:url(#grad) (same-document fragment ref, never fetches) → rewritten to none + blocked=true false banner.

suggested fix :

  • image-set: replace the [^;{}]* span regex with a quote-aware, balanced-paren scanner (like the existing importEnd) to find the function's true extent; and normalize the ident (decode escaped -, or match image-set allowing escapes) before comparing.
  • : stop regexing src out of the raw tag string - parse the tag's attributes (or at minimum require a real attribute boundary and make it global) so a src= inside alt/title can't shadow the real one.
  • false positives: exempt bare url(#fragment); make CSS_REMOTE_STRING not span across an exempt string's boundary; make stripImports skip string/url() contents.

@ahmedhesham6

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, will update the gaps

Bypasses (review on oblien#222):
- image-set() spans are now found by a quote-aware balanced-paren scan
  instead of a regex ended at ;/{/} — those are ordinary characters
  inside a string token, so the regex span truncated before a remote
  candidate (data:…;base64 fallback + remote 2x leaked every time).
- Escape decoding now includes \2d (-), so image\2dset( normalizes
  to image-set( and cannot dodge the ident match.
- <img> attributes are walked sequentially instead of regexing src=
  out of the raw tag, so a decoy src= inside an alt/title value can
  no longer shield the real remote src.

False positives:
- Fragment-only url(#grad) is exempt — a same-document lookup, not a
  fetch.
- stripImports skips string contents, so content:"@import" is no
  longer treated as an at-keyword.
- image-set strings are rewritten by a sequential token scan; the
  regex re-anchored on the closing quote of an exempt string and
  mangled all-inline sets like image-set("data:a" 1x, "data:b" 2x).
@ahmedhesham6

Copy link
Copy Markdown
Contributor Author

Thanks for the sharp review — all six reproduce on the previous revision, and all six are fixed in faa955d.

Bypasses:

  1. image-set() span truncation at ;/{/} — the span is now found by a quote-aware scan to the balancing paren (same approach as importEnd), so string contents can't end it early. Your data:…;base64 fallback + remote 2x case now blocks the remote candidate and keeps the data: one.
  2. image\2dset( — escape decoding now includes \2d (-) alongside alphanumerics, so the ident normalizes to image-set( before matching. Same reasoning as the alnum escapes: - means the same thing in an ident and in a string, so decoding is safe there too.
  3. Decoy src= in alt/titleblockImgTag no longer regexes src= out of the raw tag text. It walks the tag's attributes sequentially (name = value pairs), so only a real src/srcset attribute is inspected and a lookalike inside another attribute's value can't shadow it.

False positives:

  • D1 — image-set strings are rewritten by a sequential token scan instead of a regex, so it can't re-anchor on the closing quote of an exempt string; image-set("data:a" 1x, "data:b" 2x) now passes through byte-identical and unflagged.
  • D2stripImports skips string contents, so content:"@import" is left alone.
  • D3url(#fragment) is exempt in the lookahead; a same-document reference never fetches.

All previous behavior holds (34 checks including your five cases, the original issue repro, adversarial-input timing on the new scanners, and an end-to-end pass through the sanitizer).

@Hydralerne

Copy link
Copy Markdown
Member

perfect, gonna check last time then merge, ty

@Hydralerne

Copy link
Copy Markdown
Member

also are you on x ? would love to know your handle :"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Webmail: CSS url()/@import bypasses remote-image blocking, leaking a read receipt with images off

2 participants