Page MenuHomePhabricator

Ensure DOMPurify meets our SVG sanitization requirements for Graphs
Closed, ResolvedPublic

Description

Please ensure that DOMPurify library will suffice as an independent sanitizer for SVG produced by the Vega library. This will allow SVG images with links and high DPI images as oppose to non-clickable PNGs for graphs.

Demo is running at https://cure53.de/purify

Event Timeline

Yurik created this task.Feb 1 2016, 11:23 AM
Yurik assigned this task to csteipp.
Yurik raised the priority of this task from to Needs Triage.
Yurik updated the task description. (Show Details)
Yurik added subscribers: Yurik, GWicke, mobrovac, Pchelolo.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptFeb 1 2016, 11:23 AM
Yurik updated the task description. (Show Details)
Yurik set Security to None.
Rillke added a subscriber: Rillke.Mar 11 2016, 2:15 AM
Bawolff added a comment.EditedMar 11 2016, 2:15 AM

If in addition to the DOMPurifier, the svgs were included as an <img> tag, and served from a separate domain, this would probably be significantly safer then what we currently do for user supplied svgs.

(Just to clarify, this is an unofficial personal opinion. Ive been doing some security stuff recently, but i dont do the reviews, so all this is just imho).

Yurik added a comment.Mar 11 2016, 2:19 AM

@Bawolff that sounds like a good idea. Are we ok to sacrifice performance for security for this?

@Bawolff that sounds like a good idea. Are we ok to sacrifice performance for security for this?

Why would serving from a different domain be slower? Are they currently inline?

Yurik added a comment.Mar 11 2016, 2:30 AM

@Rillke TLS handshake (connecting to https) takes time. When you connect to en.wikipedia.org, you want to download everything from it.

Bawolff added a comment.EditedMar 11 2016, 2:37 AM

@Rillke TLS handshake (connecting to https) takes time. When you connect to en.wikipedia.org, you want to download everything from it.

I don't think you need multiple handshakes for SPDY/HTTP/2 when using a cert that has both domains in ServerAltName, and for the HTTP/1.1 case, you often get better pipelining using multiple domains. Not 100% sure about that though

Yurik added a subscriber: BBlack.Mar 11 2016, 2:40 AM

@Rillke TLS handshake (connecting to https) takes time. When you connect to en.wikipedia.org, you want to download everything from it.

I don't think you need multiple handshakes for SPDY/HTTP/2 when using a cert that has both domains in ServerAltName, and for the HTTP/1.1 case, you often get better pipelining using multiple domains. Not 100% sure about that though

@BBlack would know :)

HTTP/2 (and SPDY) will coalesce if the IP is the same and they're SANs on the same cert. The multiple domains for HTTP/1.1 thing is just to increase parallelism for that case. Currently for our primary use case (text wiki traffic + images from upload.wm.o), they're on the same cert, but upload has a separate IP from the rest (and we don't have any near-term plans to merge the IPs, for a number of reasons).

Lacking context about what domainnames we're even talking about here, it's hard to say what's up with this specific situation.

I would also say, that for this particular use case, i think worrying about this is premature optimization. <img> tags do not block page rendering afaik (esp. If width/height is specified). And graph tags are only used on a small number of pages

csteipp moved this task from Backlog to Ready on the Security-Team board.Apr 7 2016, 12:16 AM

While the underlying library does a great job a preventing javascript execution for html and svgs, it fails on a couple of things:

  • It doesn't sanitize styles. There's a plugin in the demos folder that does a pretty good job. So we could make that work.
  • It happily allows non-javascript urls to automatically pull in data, which is a privacy issue for us. They have a few demos where they attempt to filter urls, but those fail on things like css attributes with url's in them.

So as is, DOMPurify won't work for us. DOMPurify with a the css and a custom plugin that we write to strip external sources, we could probably do.

csteipp moved this task from Ready to Waiting on the Security-Team board.May 9 2016, 6:22 PM

Is there anything left to do here? Is there still a desire to serve graphs as SVGs?

GWicke triaged this task as Normal priority.Oct 12 2016, 9:28 PM
GWicke edited projects, added Services (blocked); removed Services.
Yurik added a comment.Oct 12 2016, 9:30 PM

SVGs would allow us to serve much crispier graphs, as well as (eventually) to add URL links directly to the graph. Additionally, a number of font issues will be solved because the font rendering will be done by the browser.

EBjune closed this task as Resolved.Nov 29 2017, 6:44 PM
EBjune added a subscriber: EBjune.

Nothing else to do here

sbassett moved this task from Waiting to Done on the Security-Team board.Jun 11 2019, 6:03 PM