nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.23k stars 4.05k forks source link

Replace Imagick with something better #13099

Closed enoch85 closed 2 years ago

enoch85 commented 5 years ago

EDIT (SEO): The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.

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.

EDIT 2: We now install Imaginary as a replacement for this in the Nextcloud VM.

kesselb commented 5 years ago

Ref https://github.com/nextcloud/server/pull/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: https://github.com/nextcloud/server/blob/3068f07ad98ab6ec0aa96027aa2ef45240e809a6/apps/theming/templates/settings-admin.php#L136

skjnldsv commented 5 years ago

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 commented 5 years ago

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

skjnldsv commented 5 years ago

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

J0WI commented 5 years ago

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

cyphar commented 5 years ago

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 commented 5 years ago

See also https://github.com/mkoppanen/imagick/issues/262

cyphar commented 5 years ago

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 commented 5 years ago

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

skjnldsv commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

Let's be clear here, we all agree :stuck_out_tongue_closed_eyes: 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 commented 5 years ago

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 commented 5 years ago

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

ghost commented 5 years ago

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

J0WI commented 5 years ago

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 commented 5 years ago

@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

juliusknorr commented 5 years ago

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 commented 5 years ago

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.

ghost commented 5 years ago

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.

juliusknorr commented 5 years ago

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 commented 5 years ago

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

J0WI commented 5 years ago

The imagemagick/GraphicsMagick binary is only required for extended file types like SVG: https://github.com/nextcloud/docker/issues/594#issuecomment-459859737 Is php-imagick also that worse?

ghost commented 5 years ago

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

nachoparker commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

neufeind commented 4 years ago

Often standard-webspaces have IM or GM, but not as a php-module. So imho having Nextcloud support IM and/or GM externally would be great if it's "really needed". And if it's available externally don't complain about a missing PHP-module. I don't think "loading everything into PHP" is a good solution. Maybe NC can use a wrapper-library?

pgnd commented 4 years ago

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

currently with NC 18b (18.0.0.4), setting in config.php

'enable_previews'         => 'false',

does NOT quiet/eliminate the warning in 'overview'; this still exists:

This instance is missing some recommended PHP modules. For improved performance and better compatibility it is highly recommended to install them.
    imagick
kyrofa commented 4 years ago

Last I checked that warning came from the theming app.

pgnd commented 4 years ago

@kyrofa saw your comment on other thread

The warning will go away if you disable the theming app. 

If that's the preferred remediation , cool. Can theming app be disabled in config.php? in sample config,

/**
 * If you are applying a theme to Nextcloud, enter the name of the theme here.
 * The default location for themes is ``nextcloud/themes/``.
 *
 * Defaults to the theming app which is shipped since Nextcloud 9
 */
'theme' => '',

but not clear how to correctly/safely DISABLE the app ...

pgnd commented 4 years ago

need to look in the right place!

sudo -u wwwrun php occ app:list | grep them
  - theming: 1.9.0
sudo -u wwwrun php occ app:disable theming
    theming disabled

does the trick.

no more warning about imagick in overview

neufeind commented 4 years ago

Can a decision on this be done? Wouldn't ImageMagick/GraphicsMagick as a separate binary be equally well suited for performance and compatibility? I think it's a good alternative instead of having all compiled into PHP itself.

ghost commented 4 years ago

Isn't the whole problem that ImageMagick has had some security issues, where by submitting false/crafted image files, some arbitrary code could be executed on the server?

In my opinion the risk assessment must be different for public, corporate or home/private users. I do not mind using IM since I only have NC for family and friends. But if I were hosting IM on a shared hosting, the situation might be different.

An alternative to IM might be to use GraphicsMagick, assuming there is a similar PHP module like PECL-Imagick, or to implement image processing in pure PHP. I think that kind of task would be very difficult.

neufeind commented 4 years ago

Well, or the fact that you wouldn't want to bloat your PHP with loading "everything" as a module straight into PHP. And all that ImageMagick is capable of (various formats, ...) is a huge pile of functionality. Why would you want to load that for each and every request, even when executing a small job on the CLI or something, where usually you wouldn't need that module? If it can be abstracted by some library to either use the pecl-functionality or exec the external binary or provide some fallback that would imho be fine. For various other systems there is IM or GM as a binary (for a CMS like TYPO3 for example) but not loaded as a module into PHP. (A similar case is having a huge functionality like PHP separate and hot loaded as a mod_php into your webserver.)

enoch85 commented 4 years ago

@neufeind Everything you said. :+1:

adrianvg commented 3 years ago

The warning is still seen on Nextcloud 20.0.4 Snap under Administration/Overview.

tmechen commented 3 years ago

Now there is an active warning in the admin panel when using the official docker image (:latest tag) which has no imagick:

Module php-imagick in this instance has no SVG support. For better compatibility it is recommended to install it.

Edit found some other issue: https://github.com/nextcloud/docker/issues/594#issuecomment-782585880

Sieboldianus commented 3 years ago

Isn't the whole problem that ImageMagick has had some security issues, where by submitting false/crafted image files, some arbitrary code could be executed on the server?

In my opinion the risk assessment must be different for public, corporate or home/private users. I do not mind using IM since I only have NC for family and friends. But if I were hosting IM on a shared hosting, the situation might be different.

An alternative to IM might be to use GraphicsMagick, assuming there is a similar PHP module like PECL-Imagick, or to implement image processing in pure PHP. I think that kind of task would be very difficult.

Not sure: Assume you clone some repo from Github, which includes a crafted JS SVG. Now you may have added .git to the ignore list in your Nextcloud, but the files itself may still sync to your home cloud. I am not a security expert, but it appears possible that a vulnerability in imagick's SVG JS processor is used to phone home private information, e.g. through DNS exfiltration.

sebalis commented 3 years ago

Are you sure #25866 is a duplicate of this issue? This one seems to be a long-term discussion about the pros and cons of using Imagick. I would like to see a smaller and quicker change – an option to not check for and warn about the non-presence of the SVG part of Imagick in /settings/admin/overview – I had hoped to see this in the next possible release of NC 21.

jkpirie commented 3 years ago

Hello all, Not wishing to muddy the waters on this issue but I have the warnings about missing php-imagick on my installation of Nextcloud 20 server in Linux Ubuntu 20.10. There is a twist for me though in that php-imagick IS installed but Nextcloud has decided to ignore it. Everything I have tried so far has failed apart from taking the advice here about turning off theming which removed the warning from Nextcloud security and setup. The only clue I have at present is that the version of php-imagic installed is reported as 8 whereas I am currently using php 7.4 therefore I wondered if perhaps Nextcloud 20 cannot utilise any php 8 modules. Finally, before anyone asks, I have no idea how php-imagic has turned up as version 8, I have removed and reinstalled it to no avail :) Things seem to be working fine however so this is more out of curiosity and a need to see the green tick than anything else :) Thanks for any insights.

James

j-be commented 3 years ago

@jkpirie I noticed the "SVG missing" today after upgrading to Nextcloud 21 on Ubuntu 20.04. For me, installing libmagickcore-6.q16-6-extra (found here) did the trick. That seems to be a recommended dependency of imagemagick in Ubuntu (see here), hence it may not be installed by default depending on your config (afaik you can tell apt whether to always install recommended dependencies, or not).

As for the "should you?" I'm not touching that discussion with a ten foot pole :smiley:

jkpirie commented 3 years ago

@j-be

As for the "should you?" I'm not touching that discussion with a ten foot pole smiley

Your advice is noted gratefully. You should also be aware that your ten foot pole comment had me snorting my morning coffee down my nose :)
As I said, I don't see any issues from the lack of imagick only the warning. Perhaps it will go away if ever I get NC21 installed successfully. :+1:

James

jkpirie commented 3 years ago

@j-be I just fixed the php-imagick issue using advice I found in this post: https://help.nextcloud.com/t/how-to-enable-svg-for-php-imagick/108646/32 Apparently all I had to do was remove and reinstall both imagemagick and php-imagick since I already had the files you mentioned in your reply (don't remember installing them though).
Anyway, all good, I do like to see the Green Tick ;)

James

Fuseteam commented 3 years ago

oof gmagick is still beta

gessel commented 3 years ago

graphicsmagick is currently v. 1.3.36, see http://www.graphicsmagick.org/ and pecl-imagick says "Provides a wrapper to the ImageMagick/GraphicsMagick library."

The latter would be more better, especially as 15 days ago the FreeBSD Makefile for /usr/ports/graphics/ImageMagick6 and therefore /usr/ports/graphics/ImageMagick6-nox11 added a dependency on libavutil.so:multimedia/ffmpeg, which means build sucks in ffmpeg and thus 21 additional libraries. This is a huge chain (and pain). It would be very nice to at least have an option for php74-pecl-imagick to depend on graphicsmagick instead to get the job done with a smaller flood of libraries.

gessel commented 3 years ago

The ffmpeg problem is, fortunately, patched now for FreeBSD. Hopefully will be committed soon, but for now: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256215

szaimen commented 3 years ago

Looks to me like graphicsmagick could be a good replacement to imagemagick in the meantime. Would also probably prevent many security issues but is probably a lot of work to be implemented.

szaimen commented 3 years ago

cc @nextcloud/server @nextcloud/security

J0WI commented 3 years ago

Anyone familiar with libvips? The benchmarks and reviews look great.