WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 7 months ago

#24251 reopened enhancement

Reconsider SVG inclusion to get_allowed_mime_types

Reported by: JustinSainton Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Upload Keywords: early
Focuses: Cc:

Description

There are some who think SVG should be included in core as an allowed mime type. Makes fine enough sense to me, since there is a good argument for it, and we have support for WordPerfect documents...so there's that.

Related: #20990

Attachments (3)

24251.diff (445 bytes) - added by JustinSainton 5 years ago.
24251-poc-kses.diff (15.0 KB) - added by pollett 4 years ago.
Filter SVG using KSES PoC
24251.2.diff (15.0 KB) - added by lukecavanagh 7 months ago.
Patch refresh

Download all attachments as: .zip

Change History (81)

@JustinSainton
5 years ago

#1 @JustinSainton
5 years ago

  • Keywords has-patch dev-feedback added

#2 @JustinSainton
5 years ago

  • Component changed from General to Upload

#3 @adampickering
5 years ago

SVG support should be a core feature. With the use of SVG’s now in MP6, WordPress logo using an SVG image and the rise of retina/hiDPI monitors I think its a must.

Currently if you try to upload a SVG image using the media uploader you will see an error warning.

#4 @mordauk
5 years ago

  • Cc pippin@… added

#5 @DrewAPicture
5 years ago

  • Cc xoodrew@… added

+1

#6 @melchoyce
5 years ago

  • Cc melissachoyce@… added

#7 @markoheijnen
5 years ago

Let's add this in 3.7 so we can decide how SVG's should be dealt with. Only adding it as a file format is just to limited for me.
If it's an image then we should try to have it including as an image

#8 @chriscct7
5 years ago

I would point out there are security issues with SVGs that need to be dealt with before SVG's become a security loophole upon incorportation into WordPress:

Ref[1]: List of security issues with SVG by the W3C
Ref[2]: The SVG that called a person

#9 @chriscct7
5 years ago

  • Cc chriscct7@… added

#10 @adampickering
5 years ago

@nacin cited security concerns with SVG which is a XML format. He said on IRC

nacin: XML = bad
nacin: almost always very bad

When developing a solution for this we'd have to make sure WordPress is doing checks on a defined schema

#11 @JustinSainton
5 years ago

Indeed - there are extensive security concerns to be aware of (Remote execution, unsafe redirects, etc.) - but these are not unsolveable issues. https://github.com/clones/html5lib/blob/master/python/src/html5lib/sanitizer.py takes an interesting approach.

I've seen other systems do the equivalent of applying a type of wp_kses_post() to the content of the SVG file.

#12 @nacin
5 years ago

There are also XXE vulnerabilities to be weary of.

There is an ALLOW_UNFILTERED_UPLOADS constant. There are also plugins that enable users to add types. Given how much would be required to make sure these are safe, this is a wontfix for now (and probably for a long while).

#13 @retlehs
4 years ago

  • Cc retlehs added

#14 @SergeyBiryukov
4 years ago

#24659 was marked as a duplicate.

#15 @SergeyBiryukov
4 years ago

#24659 also mentions SVGZ and WEBA.

#16 @nacin
4 years ago

#26435 was marked as a duplicate.

#17 @johnbillion
4 years ago

  • Keywords needs-patch added; has-patch removed

As mentioned above, 24251.diff on its own is not enough. For SVG file upload support to go into core we'll need some sort of sanitisation for SVG files akin to KSES.

#18 @JustinSainton
4 years ago

I'm likely the furthest thing from an expert on the subject of security, especially with regards to XXE. But, I would think something along these lines, checking for these elements on the whitelist, along with using libxml_disable_entity_loader(true); would get us a lot closer to a more secure solution. But smarter minds (nacin, markjaquith, _duck, mdwaffe) should certainly prevail.

@pollett
4 years ago

Filter SVG using KSES PoC

#19 @pollett
4 years ago

  • Cc wptrac@… added

Attached a proof of concept for using kses to filter SVG files for upload.
Tags will likely need much tweaking and how it's hooked into the upload could do with some thought, but might provoke some ideas.

#21 @SergeyBiryukov
3 years ago

#29521 was marked as a duplicate.

#22 follow-up: @LewisCowles
3 years ago

#31258 has a patch I submitted combining upload support with media gallery (including grid) support, feel free to check it out.

Can I just also make the point (from my post on #31973)

Sanitizing authorized user upload and input is a problem! It is not a feature, nor a benefit; it is most certainly not an improvement! It's like not letting a kid outside because they could hurt themselves or others, based on the fact they have hands, and hands can hold sharp pointy things...

I hope you can understand my passion and the point I am trying to make.

The W3C & Slideshare linked btw, are also not reasons to not include SVG in WP core. I Have been stopping clients slamming hard-coded CSS and JS into their code since 2010, through explanation that it is not good for organisation and messes up their pages. It is a feature, not a bug, that SVG's with script tags can be uploaded, and it should be incumbent on site owners and contributors to see that authorized accessors of their installation of WordPress does not do such things.

Bugs are softwae behaving in a way that is not expected. When I try to upload an SVG file, I expect it to upload. If it is invalid it is unreasonable of the author or distributor to think this is a WordPress issue. If I type script tags int an SVG and WordPress removes them without me asking it to, it is therefore unexpected and a bug!

#23 in reply to: ↑ 22 ; follow-up: @iandunn
2 years ago

Replying to LewisCowles:

It is a feature, not a bug, that SVG's with script tags can be uploaded, and it should be incumbent on site owners and contributors to see that authorized accessors of their installation of WordPress does not do such things.

By that logic, Core should also allow users to upload PHP scripts.

The majority of site admins aren't going to be aware of SVG security issues, let alone know how to protect against them.

If you want to take on the responsibility of handling the security issues, then there's nothing stopping you from just enabling SVG uploads via a simple filter.

Bugs are softwae behaving in a way that is not expected. When I try to upload an SVG file, I expect it to upload.

Users will also (rightfully) expect WordPress to be secure by default; they won't expect that, in order to not get hacked, they have to learn about esoteric security issues, and then go through their site disabling things that are on by default.

#24 in reply to: ↑ 23 @jimmy.smutek
2 years ago

Replying to iandunn:

If you want to take on the responsibility of handling the security issues, then there's nothing stopping you from just enabling SVG uploads via a simple filter.

Unless you also want to upload SVG via customizer, for example, to allow the option to upload an SVG logo.

Even when SVG mime types are added, customizer still will not allow SVG, right? (See also, #29521)

#25 follow-up: @LewisCowles
2 years ago

Hello @jimmy.smutek & @iandunn,

For clarity @jimmy.smutek I have a plugin written that accomplishes adding SVG including allowing upload and selection via customiser, it was the first thing I did with a view to SVG support. If you google CSS-Tricks SVG you will find the post quickly with a link to the gist.

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.

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).

In short what this is, represents a series of ill-informed people, supposing obscure security risks, which can be mitigated with even the most half-witted security policies, whilst WP allows users to use charsets with much more realistic potential risk to a WP install.

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.

There is another plugin, I am told will be updated using some of my code that also handles the SVG sanitization process for paranoid users. If you have CPU cycles to waste and untrustworthy users, feel free to use it. Personally, I think detection as part of a compliance framework, and autiting + remittance is a better fix for potential security issues.

#26 follow-up: @LewisCowles
2 years ago

Oh one other thing @iandunn, the core not only allows download and upload of PHP scripts (although not to the media gallery), but editing of plugins and themes, despite it being a horrible security risk, and encouraging poor habits!

#27 in reply to: ↑ 26 @chriscct7
2 years ago

  • Keywords dev-feedback needs-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Severity changed from minor to normal
  • Status changed from new to closed

Replying to LewisCowles:

Oh one other thing @iandunn, the core not only allows download and upload of PHP scripts (although not to the media gallery), but editing of plugins and themes, despite it being a horrible security risk, and encouraging poor habits!

The massive difference between plugins and theme upload, and image upload, is permissions. While yes, you can upload a plugin which contains insecure PHP, users can only do that with very specific, high level permissions, which by default are not granted to new users, only to people who are managing the site. Editing a plugin/theme isn't a security risk, as its restricted to just users who have administrator or above permissions. And if you want to turn that off, WordPress provides many ways to do that (constants, removing permissions from roles, removing ability to view page without a custom capability, etc)

However, the media manager is used by those who can add content to the site (like authors), who might not have the ability to upload plugins (the author or contributor role).

SVG file security isn't some obscure bug. There's multiple, well known SVG vulnerabilities. It isn't a theory, an obscurity, or an unknown. There are well over 8,000 logged CVE's that have to do with all sorts of fun and obscure SVG file security vulnerabilities.

The things that SVGs let you do may be a feature and are there by design, but that doesn't make them any less of a security risk. SVGs are inherently dangerous by design. As a CMS or as an application layer, WordPress's job is to ensure a level of security on a site. SVGs are simply too dangerous to allow. That being said, if you'd like to try to mitigate all of the security issues from SVGs and allow them to be uploaded on your site, you're more than welcome to try and write and release a plugin to do so. WordPress has filters on the allowed mime types that you can utilize to do this.

Replying to LewisCowles:

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).

And that's a terrible example, as you cannot upload HTML, Javascript or CSS files from the media manager. Nor can you edit themes or plugins without having administrator level access to the site.

Replying to LewisCowles:

Bugs are softwae behaving in a way that is not expected. When I try to upload an SVG file, I expect it to upload. If it is invalid it is unreasonable of the author or distributor to think this is a WordPress issue. If I type script tags int an SVG and WordPress removes them without me asking it to, it is therefore unexpected and a bug!

And when I turn on my computer, I expect my toaster to walk over to my bread and instantly (an automatically) make me toast. Since it does not do that, that's a software bug in my toaster.

A software bug is where the software does something that which is unintended of it's design. It has nothing to do with user expectations. Just because, as a user, I expect a car should fly over buildings to get me from A to B faster, it doesn't mean that the onboard navigation software will do that, or be designed to do that. I might really really want it to do that (I do) and I might really hate that it doesn't do that (I do) but that doesn't make it a software bug. The software does exactly what it is intended to do, which is navigation.

WordPress allows certain files types to be uploaded via the media manager, and does not allow any files to be uploaded that aren't of those types. Blocking the upload of files that are not allowed is not a bug, it is the design of the software, and contrary to what users may or may not think, it is doing exactly what it is designed to due (and therefore by definition not a bug).

WordPress needs to be secure, by default, for all users. SVG uploads via the media manager will not be permitted in core in their current document declaration iteration.

Tagging as wontfix, for the same reasons nacin pointed out 2 years ago, and will probably be true for many more years.

#28 @LewisCowles
2 years ago

What an assinine response. Most of the CVE's are browser specific, version specific, OS specific and / or library specific where the same exists between browsers; not to mention ARE A FRONT-END ISSUE, NOT A WORDPRESS CODEBASE ISSUE!!! i.e. http://www.cvedetails.com/cve/CVE-2014-1745/, which is a CVE caused by a TAG... 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...) What you report to think is rational, is like eating with your hands because knives and forks are pointy, and might stab or slice, because there are thousands of cases every year of that happening...

On your point on toasters, that is a false analogy. I Have not said it is reasonable to assume the WordPress uploader should do anything but upload valid WWW media files. Also out of the remit of the uploader should be checking licenses, and other such minutia I have not mentioned... It would be fair to assume that it should not upload files known to not be supported in targetted browsers, but SVG has very good support in modern browsers. 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.

A more correct analogy would be, when I put something that fits in the toaster, have the toaster powered on, and pull down the lever on the toaster; I expect it to turn on, and create heat, subject to the working condition of the heating elements. I do not expect it to respond only to specific brands of bread, or to check the chemical consistency to stop me heating up plastics, or toxic matter.

LESSON?
The toaster manufacturers go to very litte effort to stop me doing what I like with my toaster, because it is REASONABLE to assume that only a special kind of idiot would misuse the toaster. They also don't have cut-off switches for electrical shorts etc, because you shouldn't be fiddling with the toaster! (See https://www.youtube.com/watch?v=oz0Dln1_QnA)

To be honest I am not even pushing for SVG on your platform any more, I have given up on it, along with the desire to give anything back to the Core. From the TRAC discussions I have been involved in, I am less than impressed with the rest of WordPress, as it also suffers this hyperbolic nonsense, which does not seem restricted to media file support, and I have witnessed failing, despite all the red tape...

Bottom line I have a plugin that works, and I make sure anyone that reads the comments in the code, or asks why they need a plugin for a basic file-format, knows it exists only because of a fringe of paranoid academia, that is prevalent in design by comittee.

#29 @Kelderic
2 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

The argument here seems to be whether scripts and in general insecure SVGs should be allowed. However, what is the objection to allowing SVGs but sanitizing them, as in one of the attached patches? If the SVG is changed, the user could be notified with something like "Image upload was successful. However, there was potentially insecure code inside the image which has been removed. Please check your image to see if it still appears correctly. For more information, see [Link]"?

#30 in reply to: ↑ 25 @iandunn
2 years ago

Replying to 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.

Drupal and 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.


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.


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.


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 "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.


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.


Replying to 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 Lewis's plugin and the others in the repository and most don't have any sanitization; the one that does uses 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.

ProcessWire is 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 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.

#31 @jorbin
2 years ago

  • Resolution set to maybelater
  • Status changed from reopened to closed

What an assinine response.

Please be respectful when discussing issues.

Discussion can (and should) continue with the ticket closed. Until there exists a well tested and maintained library for svg sanitation, nothing is going to change here. As @iandunn correctly points out:

Mario Heiderich, one of the researchers who popularized the security issues, tried writing a sanitizer and found it to be harder than even he imagined.

#32 @iandunn
2 years ago

Actually, it looks like after giving up on SVGPurifier, Mario and some others took another shot at it with DOMPurify, which looks really nice, but is client-side, so not something we could use it in this context.

#33 follow-up: @enshrined
2 years ago

Hey All,

Not sure if this is of any interest to anyone, but after seeing DOMPurify, I set about trying to duplicate it's functionality in PHP. I've only put it together in a few hours tonight, but if anyone fancies running some tests on it and letting me know your thoughts then maybe we can get something that will help out this issue.

The repo is currently at: https://github.com/darylldoyle/svg-sanitizer

#34 in reply to: ↑ 33 ; follow-up: @iandunn
2 years ago

Replying to enshrined:

after seeing DOMPurify, I set about trying to duplicate it's functionality in PHP

That looks like a great start. Kudos :)

I think one of the main reasons that DomPurify chose a client-side approach, is that there's no good way for PHP to sanitize concatenated strings that build a malicious payload, etc, so I'm guessing you'll have to just completely gut anything that could possibly embed JavaScript. The Image That Called Me has a run-down of a lot of the vectors, but there may be more.

I'd recommend reaching out to Mario Heiderich for some advice, since he could give you details on why they switched from a PHP approach to a client-side one, among tons of other wisdom. I e-mailed him a couple weeks back and he was very friendly and helpful. He's on Twitter and his e-mail is in this slide deck.

His paper on the topic is also available if you're looking for a little light reading ;)

#35 in reply to: ↑ 34 @enshrined
2 years ago

Replying to iandunn:

Thanks for the info, I'll have a read through and try and get in contact with him for a discussion!

Will update here if I have any luck!

#36 @enshrined
2 years ago

Hi all,

Just a quick update on progress.

I read through Mario's papers and then got in contact with him. He's given me a few pointers which have now been addressed but what would be really lovely, is if anyone has some spare time, if they could try and run some tests on it to see if they can get anything through.

There is a smoke test available at: http://svg.enshrined.co.uk/

If anyone would like some more info on it feel free to shout and I'll talk you through it so far.

email: daryll (at) enshrined (dot) co (dot) uk

This ticket was mentioned in Slack in #core by enshrined. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by enshrined. View the logs.


2 years ago

#39 @enshrined
2 years ago

I've just put up a plugin as a sort of proof of concept of how we can securely allow SVG uploads: https://wordpress.org/plugins/safe-svg/

This is based upon the svg-sanitizer library I've been working on (see earlier comments), which at the moment seems to be doing pretty well at sanitizing attack vectors including XXE and XSS attacks in SVG files.

It hooks into wp_handle_upload_prefilter and sanitizes the data before being written to uploads. If the file cannot be sanitized, usually due to a badly formatted XML file, it will return an error to the user saying so and not upload the file.

If people are still interested in this issue and getting it fixed, testing this plugin and giving me any feedback you have would be amazing!

#40 @jorbin
2 years ago

  • Keywords early added
  • Resolution maybelater deleted
  • Status changed from closed to reopened

This proof of concept is certainly interesting. As this was closed due to concerns about the lack of a library for svg sanitization, I think that we should reconsider things in 4.4.

In the meantime, please check out the plugin linked above and the tests which are at https://github.com/darylldoyle/svg-sanitizer.

#41 follow-up: @chriscct7
2 years ago

Note the library in comment:20 is for proof of concept. As it's 5.3+, its not usable in core. Further, it's not a mature (in terms of development) or complete sanitizer.

If WordPress were to ever allow SVGs, the sanitize library would not only need to work well, it would also need to be thoroughly tested, in large scale production environments. Literally by design, SVGs are designed to be insecure. Just as we continue to find new MySQL vulnerabilities (not with WordPress specifically but with MySQL in general), SVGs continue to have entirely new vectors found.

The second something like SVGs were to get into WordPress core, our library would be scrutinized, poked and prodded for security holes.

Also there would be a significant presence to using a library that another large scale company uses in production, thus guaranteeing it's current development but also removing core team from having to maintain yet another library, like for example the Dropbox zxcvbn library.

Version 0, edited 2 years ago by chriscct7 (next)

#42 in reply to: ↑ 41 ; follow-up: @enshrined
2 years ago

Note the library in comment:20 is for proof of concept. As it's 5.3+, its not usable in core. Further, it's not a mature (in terms of development) or complete sanitizer.

I agree, this isn't at all mature as it's only been around a few weeks. It's also only aimed at sanitizing SVGs as I didn't see much reason to implement any more when tools like htmlpurifier are already established in that area.

If WordPress were to ever allow SVGs, the sanitize library would not only need to work well, it would also need to be thoroughly tested, in large scale production environments. Literally by design, SVGs are designed to be insecure. Just as we continue to find new MySQL vulnerabilities (not with WordPress specifically but with MySQL in general), SVGs continue to have entirely new vectors found.

The second something like SVGs were to get into WordPress core, our library would be scrutinized, poked and prodded for security holes.

I agree, I'd hate to see something rushed into core, only to open up major security holes and you're right, a library such as this one would have to be continually maintained and updated to keep it secure.

My thinking around it is that a lot of developers seem to allow SVGs into WordPress anyway, eased by articles like https://css-tricks.com/snippets/wordpress/allow-svg-through-wordpress-media-uploader/ which show how to allow them, without even a mention of the security risks. I'm guessing that a lot of developers that allow SVG uploads aren't aware of the risks of SVGs and therefore some sanitization is better than none, even if you still have to manually allow SVGs as you do now.

Also there would be a significant presence to using a library that another large scale company uses in production, thus guaranteeing it's current development but also removing core team from having to maintain yet another library, like for example the Dropbox zxcvbn library.

I'm not aware of any established SVG sanitization libraries out there but please do let me know if you've seen one. Wikimedia have a version baked into their uploads handler (below) which I'll pull apart at some point but from the looks of it, it's very regex based. I'll try and get in contact with someone R.E. that though to see why they did it that way

https://git.wikimedia.org/raw/mediawiki%2Fcore.git/eba9321b2b75823f8e9797398f44944e8a05389a/includes%2Fupload%2FUploadBase.php

#43 in reply to: ↑ 42 ; follow-up: @chriscct7
2 years ago

Replying to enshrined:

I'm not aware of any established SVG sanitization libraries out there but please do let me know if you've seen one. Wikimedia have a version baked into their uploads handler (below) which I'll pull apart at some point but from the looks of it, it's very regex based. I'll try and get in contact with someone R.E. that though to see why they did it that way

https://git.wikimedia.org/raw/mediawiki%2Fcore.git/eba9321b2b75823f8e9797398f44944e8a05389a/includes%2Fupload%2FUploadBase.php

From my understanding of Wikimedia's system, they actually upload the SVG and then after sanitizing it they convert it to a PNG, thus solving the security issues, but also in the process losing all the benefits of having an SVG in the first place.

From https://www.mediawiki.org/wiki/Manual:Image_administration

MediaWiki supports SVG image rendering: if enabled, SVG images can be used like other image files — they will automatically be rendered as a PNG file and thumbnailed as needed on the fly.

#44 @valendesigns
2 years ago

This is awesome! Finally some movement on the SVG front, even if it is only a plugin for now. We at the very least have a minimum level of security for those that want to add SVG support to their sites. I'm very interested to see how this all plays out.

Last edited 2 years ago by valendesigns (previous) (diff)

#45 in reply to: ↑ 43 ; follow-up: @enshrined
2 years ago

Replying to chriscct7:

From my understanding of Wikimedia's system, they actually upload the SVG and then after sanitizing it they convert it to a PNG, thus solving the security issues, but also in the process losing all the benefits of having an SVG in the first place.

From https://www.mediawiki.org/wiki/Manual:Image_administration

MediaWiki supports SVG image rendering: if enabled, SVG images can be used like other image files — they will automatically be rendered as a PNG file and thumbnailed as needed on the fly.

Yeah that seems correct, from what I can see they remove any script elements and then just convert to multiple PNGs of different sizes. They then serve the PNGs as the images on-site and just offer the SVG as a direct download from the media page.

#46 in reply to: ↑ 45 @chriscct7
2 years ago

Replying to enshrined:

Replying to chriscct7:

From my understanding of Wikimedia's system, they actually upload the SVG and then after sanitizing it they convert it to a PNG, thus solving the security issues, but also in the process losing all the benefits of having an SVG in the first place.

From https://www.mediawiki.org/wiki/Manual:Image_administration

MediaWiki supports SVG image rendering: if enabled, SVG images can be used like other image files — they will automatically be rendered as a PNG file and thumbnailed as needed on the fly.

Yeah that seems correct, from what I can see they remove any script elements and then just convert to multiple PNGs of different sizes. They then serve the PNGs as the images on-site and just offer the SVG as a direct download from the media page.

Yeah they're just sanitizing it enough so that there isn't any security concerns when running the SVG to PNG converter

#47 @SergeyBiryukov
2 years ago

  • Milestone set to Awaiting Review

This ticket was mentioned in Slack in #feature-respimg by georgestephanis. View the logs.


2 years ago

This ticket was mentioned in Slack in #feature-respimg by enshrined. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


22 months ago

#51 follow-up: @Ninos Ego
21 months ago

Think this feature should be implemented in v4.5, otherwise feature #33755 is mostly useless.. See also my comment on that ticket:
https://core.trac.wordpress.org/ticket/33755#comment:115

#52 in reply to: ↑ 51 @enshrined
21 months ago

Replying to Ninos Ego:

Think this feature should be implemented in v4.5, otherwise feature #33755 is mostly useless.. See also my comment on that ticket:
https://core.trac.wordpress.org/ticket/33755#comment:115

As stated earlier in this ticket, the library within the POC is not well enough tested to even be considered for core inclusion. Not to mention it's only compatible with PHP 5.3+, seeing as WP still supports 5.2.4 that is also a major blocker.

For now, the best thing that could happen is for people to help test the above library or if you know one that's already tested and maintained, mention that here. Until a well tested, well maintained SVG sanitisation library is found, I can't see this ticket progressing any more and for good reasons.

#53 follow-up: @Ninos Ego
21 months ago

Can someone create a patch or plugin so we can test this lib in wordpress? I would love to test it out and also try to enhance the compatibility with php 5.2.

BTW: You really want to create png-thumbnails of an svg? What's the reason here? For me it just makes sense for the site icon (here I would disable svg images) :)

#54 follow-ups: @LewisCowles
21 months ago

This thread is like the highlight-section of why there is such high frequency anti-wp sentiment amongst most IT professionals... IT took virtually no time at all to build the PoC WP plugin to allow uploads of SVG; and display in gallery. Then WP released an update and the plugin had to be modified.

I tested this morning; WP does not protect against me uploading a text-file renamed to .png, so there is probably very little to stop me uploading a malicious payload in any format. Let's stop the arbitrary, and frankly deluded fixation on the content-management system sanitizing anything, let alone SVG files; issue some simple guidance that you shouldn't just upload things to the internet and move on with SVG support.

You've had some really fantastic updates and releases since I first came to this thread; the addition of taxonomy meta-data, better user-password system (although I can still maliciously inject an MD5 password to override); and a REST API have been fantastic, I don't understand why this is such an issue when others have provided working code.

#55 @markoheijnen
21 months ago

This is really not the reason why there is an anti-wp sentiment. This is a nice feature but not a feature that lot's of people want to see in WordPress. Most of the WordPress users don't really know what SVG's are. They will upload a JPG or PNG instead. Since WordPress powers a lot of the web, things need to be thought carefully.

I personally haven't had the change to look into it but I really would love to. I will be looking at things around uploading media for 4.6 and have put this on my personal roadmap.

#56 in reply to: ↑ 54 @DrewAPicture
21 months ago

Replying to LewisCowles:

Let's stop the arbitrary, and frankly deluded fixation on the content-management system sanitizing anything, let alone SVG files; issue some simple guidance that you shouldn't just upload things to the internet and move on with SVG support.

Let's stop the arbitrary and frankly insulting hyberbole. We promote civility here on Core Trac and appreciate constructive criticism from anyone who wishes to contribute. I would encourage you to mindful of this in the future when contributing to this community.

I don't understand why this is such an issue when others have provided working code.

As @markoheijnen alluded in comment:55, there are lot of considerations in play, most notably that WordPress is built user-first. It's easy enough to make assumptions as developers that everyone will know the right thing to do or not do. It's another thing entirely to make that assumption for a largely non-technical user base, all of whom place their trust in the project leaders to make decisions that will adequately balance maximized benefit and minimized risk.

#57 in reply to: ↑ 54 ; follow-up: @chriscct7
21 months ago

Replying to LewisCowles:

I tested this morning; WP does not protect against me uploading a text-file renamed to .png, so there is probably very little to stop me uploading a malicious payload in any format.

That's not comparable to sanitized SVG upload. A PNG file, on render or access, does not run scripts. An unsanitized SVG can contain JavaScript or trigger remotely run code. There's quite a few different ways SVG files can cause malicious output. A good overview of some these issues is: https://www.blackhat.com/docs/us-14/materials/us-14-DeGraaf-SVG-Exploiting-Browsers-Without-Image-Parsing-Bugs.pdf

However, as those slides were presented 2 years ago, several new attack vectors found over the last 2 years are omitted, as well as possibilities arising from the new SVG 2.0 spec.

so there is probably very little to stop me uploading a malicious payload in any format

This would be a security bug. If you find or know a way to do this, please email security@ wordpress.org so it can be fixed.

IT took virtually no time at all to build the PoC WP plugin to allow uploads of SVG. Then WP released an update and the plugin had to be modified.

The plugin didn't fully sanitize SVGs at the time it was uploaded. Moreover, the new SVG 2.0 also adds more places for JS to be placed in an SVG file that the plugin doesn't account for.

Last edited 21 months ago by chriscct7 (previous) (diff)

#58 follow-ups: @LewisCowles
21 months ago

@DrewAPicture none of what I have said is hyperbole; it is all documented facts, references to easy to find information with anyone using this TRAC (which I have assumed they know how to use), and the wider internet.

PHP 5.2's last release was announced on January 6, 2011 http://nl1.php.net/archive/2011.php#id2011-01-06-1, that is over five years ago! Scrolling up two entries on that link demonstrates the product was EOL in 2011 as far as PHP were concerned. Any stack running that is so old, it will likely also be vulnerable at the server level to a host of vulnerabilities; many much more serious than a dodgy SVG file. The language you use would suppose this is akin to pretending santa exists to kids; but it's not, it's much more like pretending all strangers are nice people a child should trust. Sadly times change; how you educate, and how much you educate users need to change too.

As I'm much less interested in opinions than facts, and keen not to be drawn into some cyclical nonsense about tone of conversation; please check at PHP.net, pay attention to release dates and the change log from 5.2.8 (given by someone else in this thread), until the present 5.6.x version (or 5.7.x version when that becomes available). http://php.net/ChangeLog-5.php#5.2.9 . The technically minded will notice many problems ranging from the annoying to unacceptable.

I would like to apologize to anyone who has found some of what I ave said "insulting". Please be assured my interest in contributing to your project with code is no-longer existent. My only contributions to any thread which I am subscribed to now, is to ensure that it presents a technically accurate account of decisions, which may well highlight problems with this project, but should overall help to educate. I think there are only a few threads, but feel free to ban me if you wish to enforce all users operate sheerly on opinion and trust, rather than facts.

It is terrible to not be able to feel differently; it is terrible to have to use these words; But to call misrepresented facts, or exaggerations, and non-standard practices negligent is not an insult. What it does represent is the opinion of a professional (not just one), who has worked in software for over 13 years (me), based upon the current, and past code-base of this project; some of it's own official announcements; based upon some of the advice I see contributors, including your last post, giving to an audience described in your own words as "a largely non-technical user base, all of whom place their trust in the project leaders". It's unacceptable, much more so than colourful use of language.

It's this very aspect that has me both concerned and professionally shocked. It's this that you probably think is rude; but I have to say, disagreeing with someone, or believing their work is negligent based upon it's failed conformance to established industry practices is not rude or insulting, unless untrue or misrepresented. There have been professional talks at PHP community events for years encouraging people to use more recent versions of PHP runtime software; it's a core competency in operations and server-admin to ensure that updates and especially patches are applied, and that software is upgraded when needed; and a post from IRCMaxwell; a PHP core contributor including the same sentiment I am expressing on PHP can be found here http://blog.ircmaxell.com/2014/12/on-php-version-requirements.html.

I think it would be far more rude for me to congratulate, jeer, or promote ignorance to my peers of what I understand to be widely understood best practices, and whilst I do not expect them to understand much of the technical expertise I posses; especially not those that are "largely non-technical". To suggest that it's unreasonable to allow a file-type > 75% of the internet can access without restriction, that the primary server OS linux uses, that is present in many themes as a potential for representing iconography is much more hyperbolic. (For this fact, I'm using the WordPress official we are on 25% of the web figures, Fontawesome.io as an icon-font example).

Worse still to suggest SVG, unlike other files deserves excessive scrutiny (I'd suggest you take a look at MediaWiki & OWASP who both use and support SVG on systems much more open than WordPress standard installs) is to misrepresent the understood and monitored state of the market. It's like the tobacco industry telling people that it's healthy to smoke their brand, but not competitors, it's simply untrue.

Instead I would suggest that you are using the fact I am presenting uncomfortable information to suggest I am insulting you or any other specific person. I am not, and if anyone thinks I am, then again I apologize to them. I would like to point out that I cannot do anything about how they interpret a stream of facts and suggestions, all to benefit a largely non-technical user-base to enhance their project, and re-iterate that my comments are not to insult, deride, or mislead through hyperbole or emotive language. I simply do not believe the responses received against SVG in core since the initial comment; are of the same quality and effort, or technical competence that the pro-SVG arguments demonstrate.

#59 in reply to: ↑ 58 @chriscct7
21 months ago

Replying to LewisCowles:

Worse still to suggest SVG, unlike other files deserves excessive scrutiny (I'd suggest you take a look at MediaWiki & OWASP who both use and support SVG

MediaWiki/OWASP (who uses MediaWiki) does not support SVG as a viewable filetype. As pointed out in comment:43, upon upload the SVGs are converted to pngs. No SVG is ever outputted on the frontend. You can read about their process (which basically uses a series of PHP libraries in case of fallback to get a copy of the SVG rendering and save it in the png on the fly and then serves the png) here: https://www.mediawiki.org/wiki/Manual:Image_administration#SVG

Last edited 21 months ago by chriscct7 (previous) (diff)

#60 in reply to: ↑ 58 @chriscct7
21 months ago

Replying to LewisCowles:

PHP 5.2's last release was announced on January 6, 2011 http://nl1.php.net/archive/2011.php#id2011-01-06-1, that is over five years ago!

Also I'm not sure why the PHP version matters at all unless I've missed something. The proof of concept doesn't sanitize all of the vectors pointed out, regardless of PHP version. It was tested on a copy of PHP 5.6. That it is also not PHP 5.2 compatible isn't the bigger issue, the larger issue is that it doesn't sanitize all of the vectors.

#61 in reply to: ↑ 53 @chriscct7
21 months ago

Replying to Ninos Ego:

You really want to create png-thumbnails of an svg? What's the reason here?

That wasn't suggested. It was an explanation of how "svg support" worked in Wikimedia/Wikipedia

#62 in reply to: ↑ 57 @LewisCowles
21 months ago

Replying to chriscct7:

Replying to LewisCowles:

I tested this morning; WP does not protect against me uploading a text-file renamed to .png, so there is probably very little to stop me uploading a malicious payload in any format.

That's not comparable to sanitized SVG upload. A PNG file, on render or access, does not run scripts. An unsanitized SVG can contain JavaScript or trigger remotely run code.

Actually as PNG's are binary, there have been numerous exploits including code at the end of the file, using a remote server to serve alternative binary content, they are every bit as vulnerable as an SVG file, albeit through alternative methodologies.

There's quite a few different ways SVG files can cause malicious output. A good overview of some these issues is: https://www.blackhat.com/docs/us-14/materials/us-14-DeGraaf-SVG-Exploiting-Browsers-Without-Image-Parsing-Bugs.pdf

Most of the examples in that PDF are now patched and were browser problems, not CMS problems, or SVG problems; Even the slides you show mentions that many of the problems are not specific to SVG, and affect the XML file format and HTML (See page 18/55 in your PDF).

However, as those slides were presented 2 years ago, several new attack vectors found over the last 2 years are omitted, as well as possibilities arising from the new SVG 2.0 spec.

so there is probably very little to stop me uploading a malicious payload in any format

This would be a security bug. If you find or know a way to do this, please email security@ wordpress.org so it can be fixed.

Every single "exploit" mentioned was either not a security bug, or has been remitted... Again, leading me to conclude that any facts heard are out-dated and nothing to do with any application system...

IT took virtually no time at all to build the PoC WP plugin to allow uploads of SVG. Then WP released an update and the plugin had to be modified.

The plugin didn't fully sanitize SVGs at the time it was uploaded. Moreover, the new SVG 2.0 also adds more places for JS to be placed in an SVG file that the plugin doesn't account for.

I do not agree that it is the place of a plugin to sanitize SVG; it adds needless complexity, which as said could be dealt with through education of users. Using this same logic that an upload could contain malicious code, we should ban plugins, themes, and user-input... It's disproportionate to the problem space and puts unfair strain on WP to expect it to roll it's own sanitization in-house when most will be irrelevant in 3-6 months when browser vendors patch.

Using a different domain to serve assets
Only uploading in-house, or purchased SVG files
Using CSP

All mitigate existing known SVG attacks; it really is that simple.

On other comments, the PHP comment is in response to the notion that it is reasonable that WordPress supports PHP 5.2 (it's not), I accept it is a side-note, but as it came up, I felt I had to address it.

Wikimedia absolutely does not block uploading, or access to SVG files; and WikiPedia serves a link to the original SVG file as a core function of the system to allow the upload of SVG files, and provide visitor access to said files. Don't try to twist this because they have a backwards-compatibility fix to show SVG's in browsers that do not support them (IE pre 2011 as per linked .pdf)

Whilst XSS in SVG could be annoying; a greedy regex could easily remove the risk (Again I think if someone uploads a file, your job is to upload it, not provide a virus / exploit remover in the CMS); it's like insisting a car comes with life-jacket in-case someone drives into a river.

#63 @markoheijnen
21 months ago

You can rant whatever you want but at least be fair in what you say. I get that you want WordPress to handle SVG's but you must know that only a small percentage of WordPress users will use it. Also uploading plugins & themes is only for admins and if we would allow SVG's then it also works for other roles with the upload capability. I can only recommend that you put your effort in writing a patch then ranting on this ticket. Who knows then it can be added in 4.6.

Also WordPress can run on PHP 5.2 but it doesn't recommend it. Forcing it to the user will result in a security risk for those users.

#64 @Ninos Ego
21 months ago

As first step why you guys don't just allow svg upload for users, which can e.g. change the page logo in customizer (-> administrators).

In the WWW svgs are mostly used for icons and logos (designer tasks). You're right if you say, that lots of users in web still don't use svgs, because svgs are vector-based images and so just thought for icons, logos, graphics and not for pictures. Customers mostly just handle pictures. Also svgs are not known by customers, because they are not widely supported atm :D

As stated before, the feature #33755 is mostly useless without supporting svgs (at least for administrators). More and more webdesigners use svgs for logos.

Please tell us how you want to handle svg images support in wordpress now and in future releases, so we can create patches which will hopely be merged in near future :)

#65 @markoheijnen
21 months ago

I guess we could integrate it as a feature you can select per media modal but to me (basic) sanitising is needed for any integration. I do have to say that this ticket is only about uploading SVG's and not handle them as images. Like #31258 also shows some issues with it. So if people want to create patches then they should try to integrate it as good as possible.

Also I think #33755 is more a feature for regular users who will use JPG or PNG. The users who do know what SVGs are will most likely use a custom theme where they will put the logo directly in the theme itself.

This ticket was mentioned in Slack in #design by otto42. View the logs.


14 months ago

#67 @hugobaeta
14 months ago

Hoping this ticket gets attention again. SVG has become a sort of standard for resolution independent vector graphics on the web, would be great if WordPress supported it (with the appropriate precautions for security, obviously). This conversation is beyond my technical knowledge, so just adding this +1 here to spark the conversation again :D

#68 @mor10
14 months ago

Reading all this, I'm left with a question that has not been answered yet:

What happens when an uploaded SVG either appears or is expected to appear as inline content?

When an SVG is uploaded into WordPress and used in a post or page, it will be presented as a referenced item just like other image files. However, a theme or plugin can employ a JavaScript tool like
SVGInjector (https://github.com/iconic/SVGInjector) to dynamically present the SVG as an inline element accessible through the DOM. This is after all what makes SVG unique: our ability to manipulate graphics using CSS and JavaScript.

In a Customizer context, this is not all that problematic: The theme / plugin developer can ensure the inline SVG is handled properly, and put security measures in place to prevent unexpected behavior. However, if the theme / plugin dev enables dynamic rewriting of referenced SVGs as inline items, that can also extend to post/page content. That in turn means a user who uploads an SVG to use in a post or page may end up introducing actual code into the page. This in turn can cause serious issues both with presentation (repeated classes and IDs, JavaScripts targeting multiple elements), and with security (as live code in the DOM, the SVG becomes a significant security risk).

The challenge here is that while we can, in theory, do a lot to sanitize the SVG as it enters the WordPress media library, we have no control over what happens to the SVG once it's presented on the front end because a theme or plugin can alter that behavior on the fly.

Two of the major selling points of the SVG, and two of the main reasons many users will want to upload SVGs, are a) the ability to manipulate inline SVGs using CSS and JavaScript, and b) the ability to reuse symbols nested in an inline SVG throughout the site. While these may appear fringe use cases, they represent two of the main purposes for this graphics format and we can expect more and more users to want to hook into these features. And since WordPress doesn't output SVGs as inline content by default, users will employ JS tools to dynamically place these files inline.

In my view, this is something we need to discuss and resolve before we can move forward. SVG is not an image file. It's a live code document that is meant to live inline in the DOM.

#69 follow-up: @enshrined
14 months ago

The challenge here is that while we can, in theory, do a lot to sanitize the SVG as it enters the WordPress media library, we have no control over what happens to the SVG once it's presented on the front end because a theme or plugin can alter that behavior on the fly.

There are ways that we can attempt to sanitise on the client side. Libraries such as DOMPurify https://github.com/cure53/DOMPurify by Mario Heiderich are well maintained and tested and could quite easily be enqueued along with other WordPress JS files.

I think the real issue with SVGs is not that we can't protect the user from malicious scripts running as we can't stop malicious JS running either, but more the fact that we can't protect the server from being attacked.

As you say, SVG isn't an image file it's a standalone XML application and a number of server side XML attacks are well documented, but as of yet there's no surefire way of sanitising them server side that I've seen. Unless this issue can be cracked, I see no way WordPress can ever allow SVG uploads by default.

You could maybe go the same way as allowing users with the unfiltered_html capability to upload SVG's but still I'd be cautious. If someone gets malicious unfiltered html on the page you may have some XSS attacks etc but if someone gets a malicious unsanitised SVG uploaded you could end up with XXE attacks or XML bombs affecting the server.

I honestly don't think this ticket can go much further until someone comes up with a well maintained and tested SVG sanitisation library.

#70 in reply to: ↑ 69 ; follow-up: @bjornjohansen
14 months ago

Replying to enshrined:

You could maybe go the same way as allowing users with the unfiltered_html capability to upload SVG's but still I'd be cautious.

A huge issue is that while users with the capability of inserting scripts will (hopefully) be aware that scripts may be malicious, and only insert scripts from trusted sources. They are in many (most?) cases not aware that SVGs are not images at all, but XML applications. Believing they are just images, they might not consider the source at all. SVG is the perfect Trojan Horse.

#71 in reply to: ↑ 70 @enshrined
14 months ago

Replying to bjornjohansen:

A huge issue is that while users with the capability of inserting scripts will (hopefully) be aware that scripts may be malicious, and only insert scripts from trusted sources. They are in many (most?) cases not aware that SVGs are not images at all, but XML applications. Believing they are just images, they might not consider the source at all. SVG is the perfect Trojan Horse.

Yep, I think this is the big issue with SVGs. People look at them as images because in a lot of cases they're used to create images, especially on the web.

Also, any sanitisation library will have to decide how tolerant to be, it's extremely hard to work out if embedded script is malicious or not from the server side and therefore you'd probably end up having to remove all JS which in turn, takes a way a huge benefit of SVGs.

#72 follow-up: @drrobotnik
10 months ago

Did anyone acknowledge @pollett proof of concept patch? Is KSES filtering not an option?

#73 in reply to: ↑ 72 @iandunn
9 months ago

Replying to drrobotnik:

Did anyone acknowledge @pollett proof of concept patch? Is KSES filtering not an option?

I think we'll need something much more much sophisticated than KSES, given the huge number of elements and attributes supported by SVG, as well as the complexity and wide variety of attack vectors.

I haven't tested it, but I'm guessing that a simple KSES approach would end up blocking way too may things to be useful for real-world SVGs. The plugin that @enshrined built (see comment:39) is probably much closer to a real-world solution, but I think there's still a lot of work to do before Core can be confident enough to merge it.

This ticket was mentioned in Slack in #core-media by enshrined. View the logs.


9 months ago

#75 @blobfolio
9 months ago

Wanted to mention a proof-of-concept plugin along these lines: https://github.com/Blobfolio/svgeez

@drrobotnik KSES is not fully XML-compliant nor extensible in the right ways to make it work with SVG. SVGeez employs DOMDocument-based parsing for environments supporting it, and a KSES-like fallback for those that don't.

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by enshrined. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by lukecavanagh. View the logs.


7 months ago

@lukecavanagh
7 months ago

Patch refresh

Note: See TracTickets for help on using tickets.