WordPress / performance

Performance plugin from the WordPress Performance Group, which is a collection of standalone performance modules.
https://wordpress.org/plugins/performance-lab/
GNU General Public License v2.0
345 stars 94 forks source link

SVG uploads #427

Closed mxbclang closed 1 month ago

mxbclang commented 2 years ago

High-level feature

Allow uploading of regular SVGs without scripts and provide an SVG preview in the Media Library.

Technical details

mxbclang commented 2 years ago

:wave: @masteradhoc @alaca is working on research and reviewing the existing approaches for this one. He can also work on the implementation, but of course we'd love to get your input and thoughts as we move forward. Thank you again for your work on this!

masteradhoc commented 2 years ago

@bethanylang thanks! @alaca would be glad to work together on this. contact me over twitter or slack to discuss how to move forward :)

michael-sumner commented 2 years ago

We will need to make sure that any script injection is taken into consideration. Especially considering the attacks in Web3 and the adoption of WordPress websites having Web3 integration.

A common one was the OpenSea.io hack by sending people free NFT's as SVG. Which would then pop up a request that asked the user to "confirm" logging into their crypto wallet, and then another "confirm" to complete the transaction of the NFT. It turns out that the "transaction" drained the wallet of funds by using JavaScript in the NFT... if the user would have read carefully.

The mass adoption of SVG module in WordPress could have JavaScript removed to avoid script injections, and offer an option to "Allow JavaScript in SVG uploads" with a clear "Warning:" sign.

erikyo commented 2 years ago

@michael-sumner Hope this fortinet post could provide some initial info about the svg security concerns.

However, I believe that using php for this does not provide 100% security for us, since the php XML handling is subject to xml injections itself. The best one in this case is DOMpurifier that is an industry standard but, also in this case, it would not provide 100% security in the WordPress environment because images can be uploaded in many ways and could bypass this control.

My personal solution (I made a plugin of that) was to create a block so that:

EDIT: this repo could be a very good starting point to test svg uploads

adamsilverstein commented 2 years ago

One possible security solution would be to only enable SVG embedding as images (not objects) - when the SVG is implemented as an image tag or CSS background image source, the browser will not execute any JavaScript embedded inside the SVG, reducing any security concerns.

alaca commented 2 years ago

Embedding SVG as an image will help prevent running JavaScript, but that will resolve only half of the problem. The worrying part of enabling uploads of SVG files is that SVG can potentially carry many types of malicious payloads and not just JavaScript. For example, anything from HTML Injection, XML Entity Processing, Denial of Service attack, and many more. Luckily, the svg-sanitizer prevents all attacks mentioned above.

Most of the plugins listed on WordPress.org that are adding the functionality to upload SVG to WordPress are using the same library to sanitize SVG files. Some of the plugins have 900.000 active installs, which means that this library is "battle-tested". I'd suggest using the same approach but simplified and rewritten to follow WPCS. Also, all existing tests can be reused.

spacedmonkey commented 2 years ago

It worth noting that svg-sanitizer library is only PHP 7.0+. WordPress core still supports 5.6. Either this library needs to be backported to php 5.6 or will have to be php 7+

jeffpaul commented 1 year ago

@spacedmonkey see https://github.com/darylldoyle/svg-sanitizer/issues/80#issuecomment-1242284071 for info on how we could get 5.6 compat added back to the svg-sanitizer library.

adamsilverstein commented 1 year ago

It worth noting that svg-sanitizer library is only PHP 7.0+. WordPress core still supports 5.6. Either this library needs to be backported to php 5.6 or will have to be php 7+

Another possibility would be enabling SVG only for sites that have 7.0+; we add conditional support for other formats based on server capabilities as well (eg. PDF, WebP).

felixarntz commented 1 year ago

Strong +1 to what @adamsilverstein; we also discussed this a few weeks back in one of the weekly performance chats, and everyone was aligned that this would be a good feature to simply only offer for sites on PHP 7+. It can easily be built in a way that SVG uploads are not allowed for those WordPress sites still running on PHP 5.6.

I've clarified that in the issue description now.

mxbclang commented 1 year ago

Additional discussion: https://github.com/WordPress/wporg-mu-plugins/issues/300

spacedmonkey commented 1 year ago

Seems like this there is movement on making PHP 7.2 min version. Which would be extremely helpful - https://core.trac.wordpress.org/ticket/57345

swissspidy commented 1 month ago

Since we moved away from modules to standalone plugins, and there are already plugins like https://wordpress.org/plugins/safe-svg/ utilizing SVG Sanitizer, I don't see value in creating yet another plugin for SVGs. It could be worked on directly in core (in https://core.trac.wordpress.org/ticket/24251). Also, SVGs are not a priority for the performance team.

I suggest closing this issue here in favor of https://core.trac.wordpress.org/ticket/24251