[wp-trac] [WordPress Trac] #24251: Reconsider SVG inclusion to get_allowed_mime_types

Wed Jun 3 22:05:36 UTC 2015

#24251: Reconsider SVG inclusion to get_allowed_mime_types
---------------------------+-----------------------
 Reporter:  JustinSainton  |       Owner:
     Type:  enhancement    |      Status:  reopened
 Priority:  normal         |   Milestone:
Component:  Upload         |     Version:
 Severity:  normal         |  Resolution:
 Keywords:                 |     Focuses:
---------------------------+-----------------------

Comment (by iandunn):

 Replying to [comment:25 LewisCowles]:
 > Unlike my other plugins, I do not feel it is right that SVG is
 considered a site-specific / ecosystem-specific use-case. In 2015 it is an
 absolute horror that something, as widely used as WP, uses such old and
 limited file-format support.

 [https://api.drupal.org/api/drupal/includes%21file.inc/function/file_save_upload/7
 Drupal] and [https://github.com/joomla/joomla-
 cms/blob/ea05ad099fca9325d5d9e15d0f2bf34be0cb90c8/administrator/components/com_media/config.xml#L8
 Joomla] don't allow it either, but if you know of a popular CMS that has
 implemented a way of securely allowing SVG uploads -- or feel like writing
 it yourself -- then by all means link to the code, since that would help
 any effort to bring a similar mechanism to WP.

 [[br]]

 > For clarity with @iandunn, you clearly do not understand the supposed
 security risk being talked about fully. This is not a file-system bug or
 privilege escalation, or SQL hack. It affects more than just SVG as a
 domain, and as I understand it from the W3C, could affect many taggable
 file formats accepting script tags, or javascript, and data uri's, css
 files linking SVG from external resources could be a bigger risk (so HTML,
 xhtml, CSS and ironically JS, are also potential candidates for such
 hacks, and they are not banned).

 I'll be the first to admit that I don't fully understand all of the
 vulnerabilities and mitigations, but that's part of the problem. They're
 relatively new and not widely understood. Most developers still perceive
 SVGs as just images rather than dynamic XML applications. The protections
 against them are even newer and are frequently bypassed. I think it's
 sensible to wait until things settle down.

 Core does restrict HTML, CSS and JavaScript uploads to the point
 necessary. HTML can't be uploaded by Contributors and Authors (Editors and
 Admins can, because they have the `unfiltered_html` capability).

 JavaScript files can be uploaded by Authors, but they're not executed. If
 you visit them directly, you just see the raw source, and there's nothing
 that causes them to run inside wp-admin. CSS uploads aren't allowed at all
 (although I'm not sure there's any risk there, since they wouldn't be
 execute either).

 SVGs loaded as a CSS background image run in a different context than
 inline SVGs; they're not part of the DOM and can't execute JavaScript.

 [[br]]

 > Also because these are front-end hacks being spoken about, unless you
 are on a page with the linked SVG, allow users that are untrustworthy to
 use your WP, upload files etc, your visitors would not be at risk; it
 would be simple to mitigate, and it would not affect the backend, without
 significant intervention from an admin-level user.

 SVGs would presumably be shown in the media gallery previews like any
 other image, allowing an embedded script to silently steal an admin's auth
 cookie, etc.

 Even if they weren't shown in wp-admin, it's still a security issue for
 admins visiting the front end. It wouldn't be hard to social-engineer an
 admin to visit a page on their own site. They'll also execute when visited
 directly, unlike JavaScript.

 A lot of WordPress sites have untrusted users, even untrusted admins;
 think about a site like WordPress.com. Multisite even assumes that admins
 are untrusted, and removes the `unfiltered_html` capability from them.

 Allowing unsanitized SVGs would effectively be privilege escalation,
 giving `unfiltered_html` to Authors.

 [[br]]

 > I'm not even against the middle-ground of them being turned off with an
 admin option to enable them, but this is not something even considered in
 this polarized non-debate.

 I don't think that's a likely solution, since Core has a
 "[http://wordpress.org/about/philosophy decisions, not options]"
 philosophy.

 If you substitute filters for GUI options, though, that's pretty much the
 current state of things. It's off by default, but it's easy for someone to
 enable it if they want.

 [[br]]

 > Again, I suggest RESTRICT ALL USERS, which mitigates potential risk a
 lot better than banning a file-format, so ban your author that has the
 dodgy SVG, and it won't affect your users because you will have an editor!
 (Use the roles luke...)

 That might be possible, since Editors and Admins can already upload HTML
 files. They would still need to be sanitized within wp-admin, though, to
 enforce Core's policy against XSS there, and I wouldn't be surprised if
 there are other issues.

 [[br]]

 Replying to [comment:29 Kelderic]:
 > what is the objection to allowing SVGs but sanitizing them, as in one of
 the attached patches

 I would guess that that's the most promising way forward, but I don't
 think there's a solid sanitization library available right now.

 I looked at [https://gist.github.com/Lewiscowles1986/44f059876ec205dd4d27
 Lewis's plugin] and the others in the repository and most don't have any
 sanitization; [https://wordpress.org/plugins/scalable-vector-graphics-svg/
 the one that does] uses [https://github.com/alister-/SVG-Sanitizer a
 library] that appears to be old and unmaintained, doesn't have a large
 user base, doesn't have unit tests, and probably hasn't been audited by an
 expert.

 [https://processwire.com/ ProcessWire] is [http://www.flamingruby.com/blog
 /processwire-weekly-47/#3 using it] as the basis for their SVG
 sanitization, but I suspect that they're small enough that they don't get
 much scrutiny from researchers and attackers.

 Mario Heiderich, one of the researchers who popularized the security
 issues, tried writing a sanitizer and
 [https://security.stackexchange.com/a/30390/8467 found it to be harder]
 than even he imagined.

 Maybe we could build our own, and `24251-poc-kses.diff` is a step in that
 direction, but I think it will take a lot more work to prove it's
 reasonably secure.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/24251#comment:30>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform