Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use Imagick in server, and don't recomend it #13099

Open
enoch85 opened this issue Dec 16, 2018 · 27 comments

Comments

@enoch85
Copy link
Member

commented Dec 16, 2018

A few days ago it was brought up to my attention that using Imagick could have very negative effects on security. The Nextcloud snap decided to not using it due to that fact, and I've now mitigated the same threat(s) as well by not using it in the Nextcloud VM.

Here are the discussion regarding the decision in the Nextcloud snap, and I think it totally makes sense not to use it in the Nextcloud Server as well.

The situation now though is that it's recomended and the setup checks will inform the user that the package is missing. As Nextcloud is advertising it's secure, then why use a package that is prune to a lot of CVEs in the past?

Regarding alternatives I think this post sums it up quite well.

Please consider removing the recommendation in future versions, and please also consider replacing the use of Imagick with something better and more secure.

@kesselb

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2018

Ref #12821

I see the concerns and could imagine to move the warning into the theming app that some feature are not working because imagick is not present.

@skjnldsv @juliushaertl looks like @nextcloud/vm and @nextcloud/snap not going to add imagick. @nextcloud/docker added imagick a few days ago.

Edit: There is already a warning that some things does not work:

<?php if (!$_['canThemeIcons']) { ?>

@skjnldsv

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

Please consider removing the recommendation in future versions, and please also consider replacing the use of Imagick with something better and more secure.

Nothing allow us to properly convert images types (especially svg) with php other than imagick (that I'm aware of) unfortunately.

@kesselb

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2018

Apart from this favicon svg generation the theming app works fine without imagick?

@skjnldsv

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

@danielkesselberg avatars will be not as great looking without imagick.
Previews too :)

@J0WI

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

How about the performance without imagick?
This does also affect the gallery app: https://github.com/nextcloud/gallery#supporting-more-media-types

@cyphar

This comment has been minimized.

Copy link

commented Dec 17, 2018

From what I've read, most of the ImageMagick CVEs come from individual filters or filetypes that we probably don't care about -- is there a way to whitelist policy.xml so that we only allow the few formats that we want to support? In addition, checking that the magic header of files is actually correct (for the formats we want to support) would protect against most malicious files.

ImageMagick (and GraphicsMagick to a lesser extent) have had quite a large number of CVEs, mostly due to the sheer amount of formats and features that users need to process images. I would say a good first step would be to figure out a whitelist hardening configuration that we can use across the board, and then we can evaluate switching away from ImageMagick if that's not sufficient.

@J0WI

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

@cyphar

This comment has been minimized.

Copy link

commented Dec 17, 2018

There are also a couple of ways we could restrict ImageMagick through seccomp or by putting it inside an empty network namespace (and a mount namespace which has everything mounted-over except /lib64 and the file that is being accessed -- though this will require a bit of work since it requires having some sort of unveil feature).

@enoch85

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

@skjnldsv So something like https://github.com/flyimg/flyimg wouldn't work?

@skjnldsv

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

@enoch85 that would require shell_exec. Yes, we could rely on external software (like inkscape for example). But this is not really recommended to do on php. @rullzer ?

EDIT: sorry, I thought it was another software. Yes, we can use an external docker as well. We actually have a PoC somewhere for that. But this is a really heavy dependency and this would not scale to every setup. Also, most people don't use docker :/

@kyrofa

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Thanks for this, @enoch85. It's probably no surprise that I completely agree on this issue. In the snap it's not even possible for people to use it, so folks will just see the warning forever and be unable to do anything. So at the very least, packagers should be able to disable this warning without triggering an integrity failure. Even better, Nextcloud should just stop suggesting it be installed if it's not. If one doesn't miss the functionality it provides, all it does is make the general populous less secure. Best yet: find an alternative so everyone can enjoy the functionality without trading security for it.

@skjnldsv

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Best yet: find an alternative so everyone can enjoy the functionality without trading security for it.

Let's be clear here, we all agree 😝
Having another php lib that could do the job would be ideal, but I don't have anything else to suggest unfortunately. I'm open to suggestion though :)

@kyrofa

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

I'm glad we agree on that, but I'm also realistic: I don't know of an alternative off the top of my head either. While we look for one, can Nextcloud please stop complaining if imagick is not installed?

@bpcurse

This comment has been minimized.

Copy link

commented Dec 18, 2018

How about a temporary solution that shows this special warning as one that can be confirmed as read and then be permanently hidden?

@Gatak

This comment has been minimized.

Copy link

commented Dec 19, 2018

Is it just SVG that GD didn't support? How about https://github.com/meyfa/php-svg?

@J0WI

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

I'm not sure if a less common extensions with only few/one maintainers more secure (and more reliable for e.g. compatibility) than ImageMagick.

There are indeed more filetypes, but:

Technically Nextcloud can also generate the previews of other file types such as PDF, SVG or various office documents. Due to security concerns those providers have been disabled by default and are considered unsupported. While those providers are still available, we discourage enabling them, and they are not documented.

https://docs.nextcloud.com/server/stable/admin_manual/configuration_files/previews_configuration.html
https://github.com/nextcloud/gallery#supporting-more-media-types

@LEDfan

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

@enoch85

So something like https://github.com/flyimg/flyimg wouldn't work?

It seems like that's more like an API/microservice to manipulate images, but under the hood it's also using ImageMagick https://github.com/flyimg/flyimg#technology-stack:

Image manipulation: ImageMagick

@juliushaertl

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

I'm glad we agree on that, but I'm also realistic: I don't know of an alternative off the top of my head either. While we look for one, can Nextcloud please stop complaining if imagick is not installed?

It is not a hard error message, but a warning, so I see no reason why, we should hide this. It just informs admins that they are missing a dependency that might cause some features to not work properly. Regarding the security concerns, if previews for those files are not enabled we don't pass user provided files to imagemagick as far as I know. The theming app is limited to admins, so there is no attack vector here, since the admin is considered as trusted anyway.

@kyrofa

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

As soon as imagick is available it can be used by any application, no? That's the big problem here. It's easy to use incorrectly and it's easy to install third-party apps that do so.

@Gatak

This comment has been minimized.

Copy link

commented Feb 9, 2019

How about GraphicsMagick, a fork of IM. Does it have the same security problems?

http://www.graphicsmagick.org/

From their website:
Here are some reasons to prefer GraphicsMagick over ImageMagick:

GM is more efficient so it gets the job done faster using fewer resources.
GM is much smaller and lighter (3-5X smaller installation footprint).
GM is used to process billions of files at the world's largest photo sites (e.g. Flickr and Etsy).
GM does not conflict with other installed software.
GM suffers from fewer security issues and exploits.

@juliushaertl

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

GraphicsMagick seems to be API compatible to imagemagick, so it could be a drop in replacement in any setup as far as I can tell.

@kesselb

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

https://pecl.php.net/package/gmagick there is no stable release 😞

@J0WI

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

The imagemagick/GraphicsMagick binary is only required for extended file types like SVG:
nextcloud/docker#594 (comment)
Is php-imagick also that worse?

@Gatak

This comment has been minimized.

Copy link

commented Feb 11, 2019

php-imagick is just a php wrapper for the binary Imagemagick.

@nachoparker

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

I am getting better results in terms of speed with Imagick.

This might contribute to the conversation

https://ownyourbits.com/2019/06/29/understanding-and-improving-nextcloud-previews/

I wonder how we can assess how much of a risk Imagick is and how much we can mitigate, since it does perform better according to my testing, which is quite important for low end devices.

@enoch85

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

While we look for one, can Nextcloud please stop complaining if imagick is not installed?

Would be very nice for NC 17. Just saying. :)

@enoch85

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Stumbled across this: https://docs.nextcloud.com/server/stable/Nextcloud_Server_Administration_Manual.pdf (4.10.3)

EDIT: It actually says in the Nextcloud manual that you should "Disable preview image generation".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.