Sanitize OR add Content-Security-Policy for SVGs

Adding related issue.

+1 for this idea. It's important to maintain strong security around SVG, but also important to allow reasonable, legitimate use of the format.

A look at https://content-security-policy.com raises two questions for me:

1) What will our security policy for SVGs look like? My feeling is that we should initially lock it down completely in core and either open it up as needed in the future, or allow site builders to override and loosen it if they need to. One possibility is to make this a configuration setting in the Image module, but not introduce a UI to change (i.e., developers will be able to mess with it easily but the casual site builder or author won't).

2) What are we going to do about older browsers that don't support the CSP header, or only partially support it?

Content Security Policy is supported by IE 10+ (though as X-Content-Security-Policy in IE 10 and 11), so the only browser left out is IE9, which is no longer supported as of April 2017 (see also #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x)

Came here from #2650260. About @phenaproxima's questions, I'd say:

1) Scripts should not be allowed to run in SVG files. Plain and simple. I think this alone would fix all security concerns with SVG. Or is there another attack vector in SVGs except script execution we need to care about?

2) We should check whether IE10/11 properly support the Script blocking. Per 1) I guess we don't need any other features. For even older browsers, I would honestly say: Not our problem. It might sound a bit extreme at first, but, looking more closely: Those browsers are usually no longer supported by their respective manufacturers for a long time. And they are possibly running on Operating systems that are no longer supported as well. Those people don't get security updates. Not for their browser, not for their OS. And they don't care. They probably by now have more security holes in their environment than Swiss cheese. I highly doubt that it's now the SVG upload that'll break the camels back. BTW: As detailed in #2650260, I highly favor an option in "settings.php" to enable SVG upload support in general. With an added warning about the implications if you do this (e.g. people have to make sure .htaccess is actually respected etc.). Why not leave it then up to the site administrators if they want to allow this? For example, we have LOTS of Drupal websites here, where there is only a tiny audience of editors (or even only a single one) who can actually log in at all. For users without any credentials, there's not much the SVG can steal anyway.

I'd rather see that we take the best from SVG Formatter (Source) and SVG Sanitizer (Source) that both rely on darylldoyle/svg-sanitizer (Packagist: enshrined/svg-sanitize).

Am i improving or hijacking this? Feel free to revert if you think the latter.

Why not both? I suggest CSP as a good first step regardless and an svg formatter/sanitizer as another good idea.

Yeah, I'd also say both can't hurt. Though I somewhat fear the complexity introduced by a decent sanitizer and the wrong illusion of security it might give if it doesnt work 100%.

BTW, I just found this while researching the topic:

https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

Interesting stuff in there IMHO, we might want to check. Especially the:

background-image: url('javascript:alert(8);')

On a very cursory glance, the sanitizer doesnt seem to sanitize stylesheets at all yet. I might be wrong, though...

Quickly tested: On http://svg.enshrined.co.uk/ i quickly entered a style="background-image: url('javascript:alert(8);')" which (was not sanitized away and) came to my browser in an unwrapped xml element, but did not have any alertbox effects.

From https://www.w3.org/wiki/SVG_Security#SVG_as_image
> The SVG document is not allowed to fetch any resources.
> Scripts must not be executed.

Are we done then?

EDIT: Maybe for browser viewing, but not for the User-saves-and-views-SVG attack described here.

I think we are not done, because this whole issue is not about SVGs that are loaded as part of another webpage (e.g. IMG src tag). They usually don't pose a security risk exactly for the reasons you cited. The problem is about calling SVGs directly.

I'm not sure whether the style attacks and other attacks mentioned in the OWASP doc really don't work in all recent browsers. It certainly isnt sufficient to just test it in some...

I'd also really, realy like to see SVG support in Drupal, but I think we definitvely have to tread forward extra-careful here, because we are opening a security can of worms. If we don't watch out, we inadvertently create bad security holes...

What I'm currently missing is the reasoning behind why the sanitizer is really sufficient. Which vectors exist? And how are they catched by the sanitizer. Just taking the sanitizer and just hoping everything would be alright is a bit to easy, I guess.

Yes i also won't bet too much on the sanitizer, it may be a good start though.

> The problem is about calling SVGs directly.

What do you mean? If we can box untrusted SVG in an img tag, why would we risk something else?

Note that the owasp article is from 2011, while the W3C wiki page is based on a chat in 2014.
(Which does not necessarily mean everything-is-bright-and-shiny now, but it gives historical context.)

I meant: When the user uploads an SVG, by default it ends up in sites/default/files. And then you can call:

http://www.goodandhonestsite.com/sites/default/files/evil.svg

It doesnt matter how you actually use the image in the site. The possibility to call it that way is immediately there after the users uploads the file.

Hmm I think the issue summary should be updated to indicate that the security issue is restricted to SVGs being embedded directly "inline" via an

@bkosborne: Well, the summary talks about uploads, specifically. And, just to be clear, it doesn't help if you actually use the uploaded SVG only inside an img tag. As stated, the problem is that the upload usually ends up in a public directory and the resulting image can as such be called via /sites/default/files/evil.svg. This, by itself, is already the security hole - no matter however the image will actually be used on the site later on. Perhaps you are already aware of this, I'd like to emphasize it again though, because the "...is restricted to..." part sounded *a bit* like "Hey, it only causes problems in rare cases".

I hear you, I appreciate the emphasis on security.

Here's a patch to try out, that adds a CSP header via the root .htaccess file.

#25: I don't grok the CSP header but modulo its correctness the patch looks good to me. If it's that easy we may well drop the (my) proposal to sanitize.

I just copied the header value from what Github uses on their CDN.

It's restrictive by default (default-src 'none'), which doesn't allow loading any external resources.
It then opens up exceptions for embedded raster image data (img-src data:), and inline style attributes (style-src 'unsafe-inline')

There's another Github issue about allowing inline font data: https://github.com/github/markup/issues/1164

----

An alternative to setting this in the root .htaccess would be to apply it to the .htaccess in the files directory, so that only uploaded SVG files receive the header.

Good findings! We might consider a code comment about this.

> There's another Github issue about allowing inline font data:

That makes a lot of sense to me.

> An alternative to setting this in the root .htaccess would be to apply it to the .htaccess in the files directory, so that only uploaded SVG files receive the header.

That's an interesting point to bikeshed about ;-). I'd tend to apply to all served files until we know a reason to not do.

Which brings me to another point: How do we tackle SVGs uploaded to private filesystem and served by Drupal...?

Umm, what about the following:

Let's say we have Drupal commerce shop or a Drupal forum site or whatever. IE11 is supported:

https://www.drupal.org/docs/system-requirements/browser-requirements

"The latest supported release of the latest major version of: Internet Explorer".

The latest major release of Internet Explorer is IE11 (because it's called Edge after that).

Apart from that, even if it wasn't officially supported... the website might happily accept forum or shop users using IE11 and perhaps they won't notice anything not working.

Now, let's upload an SVG which contains a malicious script. The script is under: sites/default/files/evil.svg.

Now, I go to a URL shortener website and create an alias for this: bitly.com/blabla

Now, I publish that URL on my twitter account and say: Hey, celebrity XY totally XXX (you know what).

IE11 user is logged into his shop. Clicks on the link. CSP doesn't work:

https://stackoverflow.com/questions/42937146/content-security-policy-doe...

Bam!

How do we avoid that? Do we even *want* to avoid that or ignore that risk?

IMHO, doing the sanitizing stuff is still useful in addition, after all. It creates a fallback for those cases.

It also creates a fallback if .htaccess doesn't work for some reason. (e.g. if AllowOverride is configured to false etc). What I think is dangerous about the above patch is that nobody will even notice if the webserver or browser doesn't support it. Everything will work just fine. But in fact the/some user(s) are then susceptible to evil cross-site scripting attacks.

This introduces a response subscriber to add a header to all BinaryFileResponse instances, which will protect private files.
CSP isn't relevant to most file types, but the subscriber still sets a restrictive policy (just default-src 'none'). Modules are able to override this header value through their own subscriber if necessary.

----

csp.module currently sets the same header value on private file responses as it does on page responses, but should probably be updated to send a different value like in this patch.