Add Imagemagick support in PHP

[pachulo]

With the imagick PHP extension and it's dependencies.

When merged, this should fix #592

[kyrofa]

There was quite a security issue with Imagemagick there for a while, have you looked into that? Are there still concerns?

[pachulo]

Well, it has a pretty bad record regarding security vulns:

• https://www.cvedetails.com/vulnerability-list/vendor_id-1749/Imagemagick.html
• http://changelogs.ubuntu.com/changelogs/pool/main/i/imagemagick/imagemagick_6.8.9.9-7ubuntu5.11/changelog

And though I have not spent time trying to find out the real risk of having this activated I rely on the opinion of experts:

• https://lcamtuf.blogspot.com/2016/05/clearing-up-some-misconceptions-around.html

And so, the advice sorely missing from the "ImageTragick" webpage is this:

• If all you need to do is simple transcoding or thumbnailing of potentially untrusted images, don't use ImageMagick. Make a direct use of libpng, libjpeg-turbo, and giflib; for a robust way to use these libraries, have a look at the source code of Chromium or Firefox. The resulting implementation will be considerably faster, too.
• If you have to use ImageMagick on untrusted inputs, consider sandboxing the code with seccomp-bpf or an equivalent mechanism that robustly restricts access to all user space artifacts and to the kernel attack surface. Rudimentary sandboxing technologies, such as chroot() or UID separation, are probably not enough.
• If all other options fail, be zealous about limiting the set of image formats you actually pass down to IM. The bare minimum is to thoroughly examine the headers of the received files. It is also helpful to explicitly specify the input format when calling the utility, as to preempt auto-detection code. For command-line invocations, this can be done like so:
  • convert [...other params...] -- jpg:input-file.jpg jpg:output-file.jpg
  • The JPEG, PNG, and GIF handling code in ImageMagick is considerably more robust than the code that supports PCX, TGA, SVG, PSD, and the likes.

But I'm not really sure if this is a stopper for the integration.

[kyrofa]

Good news is, seccomp-bpf is already used by virtue of it being a snap. I'm not sure that will stop the denial of service issues, though. This makes me nervous, grr, I'm having second thoughts now. @pachulo how do you feel about this? @r4co0n how about you?

[ykgmfq]

Over in Docker camp: nextcloud/docker#263

[pachulo]

Well, I think that taking into account that:

• In the default snap install this should only be used for generating some stuff when using a custom theme.
• We will have the packets upgraded when new fixes are released, as they are in the main archive of Ubuntu.

I would say that we can take the risk of having these inside the snap.

[brainfrz82]

If imagemagick is included, how would this impact preview generation? On one hand, imagemagick should be a bit faster on preview generation, but on the other hand imagemagick supports a lot of new filetypes. I'm running on a pentium, but it would take weeks to generate previews for the 1TB+ RAW files i'm storing :) Couldn't even make an estimation on how a rPi would handle this.

[kyrofa]

Have you tried leaving these off and seeing if the snapcraft CLI pulls them over for you?

[pachulo]

No, I haven't, but I will now.

[kyrofa]

You can safely remove this comment, although I appreciate seeing it once to describe why the change was necessary, it doesn't need to stay in the YAML.

[kyrofa]

Were you able to verify that the snap can still use magick with these gone?

[pachulo]

No, but I'm compiling snaps with this code right now and let you know!

[steiny2k]

Hi what's the status with this change? I'm especially curious whether it would also support the newly added .heic files which we added to nextcloud/core with nextcloud/server#7406 I guess the snap would need libheif as well to make it work. Would you be interested if I dig into this?

[kyrofa]

Last I knew @pachulo was still playing with the stage-packages. @pachulo is this good to go? I still have mixed feelings about it, but I'm willing to give it a shot given that the snap is confined.

[pachulo]

Well, the last thing I've tried is what I wrote here #592 (comment) : everything seemed right, but I was not able to really see it working.

[JamesBream]

This change does make me uneasy particularly since this PR looks to be using imagemagick's default minimum security policies (correct me if I'm wrong @pachulo ) which Imagemagick themselves advise are not suitable for anything pubic facing.

Snap sandboxing has been brought up which I suppose does provide the system with some protection from being totally compromised, but wouldn't protect anything within the snap (e.g. sensitive user data) from being compromised (again, correct me if I'm wrong).

Others here might disagree with me but I question the benefit of such a change given Imagemagick's history, simply for autogenerating favicons and preview generation of non web-safe image formats? Also, if we do properly secure imagemagick, that typically means restricting image formats it will process which will likely mean that the only real benefit will be to auto-generate favicons?

I'm open to being persuaded on this one, but it seems like we're adding a pretty big attack surface here for all users of the snap so definitely worth the discussion.

[pachulo]

Hey @JamesBream ! First, thanks for the feedback!!

I don't know what those "imagemagick's default minimum security policies" are, so I cannot answer that.
Regarding the other questions:

1. Yes, an exploitation of imagemagick would allow an attacker to access sensitive data stored in nextcloud.
2. Yes, maybe the functional gain is too low compared to the risks.

So @kyrofa do we ditch this then?

[JamesBream]

@pachulo Oops, I meant to include a link to https://www.imagemagick.org/script/security-policy.php

[pachulo]

OK, now I get it. I only have to say that all your points are quite fair, so I have no arguments to try to convince you for adding this to the snap.

[steiny2k]

Hi Okay, I understand the concerns raised. However, for a usual home-user environment I'm not so sure whether they are applicable. I'm running my nextcloud for family and friends which I trust. So I value the feature above the possible risk. However, if you're a service provider you might come to a different conclusion.

Overall, architecturally it would be best, to sandbox imagemagick in its own snap I suppose, and call it as a microservice to provide preview images. However, this wouldn't rely on the PHP PECL interface anymore I'd guess... Or to run ImageMagick together with its own PHP. Ideas have been drafted here for example: owncloud/core#24424

There are even other approaches outside of Next-/ownCloud continuum addressing this. E.g:

• Flyimg PHP based
• thumbor Python based
• imgproxy Go based

Though I don't know whether they would work with the heif image file format I'd be especially interested in. However for a service provider this would be something I'd be looking into.

What's your opinion on this, shall we provide ImageMagick in a add-on flavour to some home-use channel of the snap? Should we ditch the option all-together and wait for a microservice-approach centrally implemented?

For my home-use setup I'll be sticking with a freebsd jail running all the necessary services together (MariaDB, PHP, nginx, ImageMagick, etc.). As this snap also includes the MySQL database, it's not really a pure micro-service approach already in the first place. Guess there's no right or wrong here...

[brainfrz82]

I'm with steiny2k on this one. I also run my nextcloud for my family and would benefit from RAW file thumbnails. I don't think enterprise users would/should generally run the snap version. Maybe the imagemagick module could be implemented as an opt-in setting via OCC?

[r4co0n]

The concerns raised here seem to apply upstream as well. I can't see netcloud/vm mitigating any of this, though they are installing php-imagick.

Digging through the Debian packages, I couldn't find anything about policymap, so there are apparently no restrictions applied as outlined on imagick.org . But maybe I didn't look hard enough.

[kyrofa]

I'm sorry for the delay everyone, I've been thinking a lot about this. @brainfrz82 my wife is a professional photographer, so believe me when I say that I want RAW previews as well. However, I just don't feel right pushing a piece of software that we know has a shoddy security record to all 17k snap users. I know of businesses running this as well, but even if all snap users were home users, I don't want to open them up to that kind of risk either.

With that said, if someone can think of a way to make this risk opt-in, let me know (I can think of none, I'm afraid, since such a decision would seem to be needed at build-time). It would be nice to see Nextcloud move to using something with a better record, instead.

@pachulo I truly appreciate the effort you've put into this. I hope you understand.

[brainfrz82]

@kyrofa In hindsight, with nextcloud's subtitle being "Protecting your data", I do believe you made the right call here. Practicality should not trump security if security is the main thing you advertise with :)

Let's hope they'll think of something more secure upstream :)

[kyrofa]

Thanks for your understanding, @brainfrz82. I do apologize for the inconvenience!

[pachulo]

No problem @kyrofa !

[skewty]

As a possible solution, why not add another snap channel; named stable-risky perhaps. In this channel, more risky modules can be included.

The diff between stable and stable-risky hopefully will be small enough that maintenance shouldn't be too bad.

Perhaps out of scope, but some snap packages have extra permissions you can turn on. Can snap packages add their own special permissions?

[r4co0n]

I had the pleasure to talk about this with Nextcloud's groundskeeper (he said 'Hausmeister', groundskeeper Willy from the Simpsons is 'Hausmeister Willy' in German) @MorrisJobke about this, as he happened to talk at our company stuff about handling git properly.

I was told that the security-critical features of ImageMagick are mostly about complex image formats, think SVG and its embedded JavaScript. Rendering of those formats is disabled in Nextcloud by default, which should make these concerns a non-issue. Someone should go have a look at the upstream code to find the portion where they handle those file formats and make sure everything indeed is as I claim it to be, I might do this myself, but wanted to report my finding anyway.

This also concerns nextcloud/vm#743

[MorrisJobke]

There is also an alternative approach: nextcloud/server#13551

[MorrisJobke]

See also nextcloud/server#12821 (comment) and the comment from @kyrofa - maybe we should re-evaluate this a bit more.

[pachulo]

Hey, it seems like upstream is solving this, at least for the image/* preview generation, in Nextcloud 24: nextcloud/server#24166

🥳

[kyrofa]

Good find, @pachulo. And given that it's an external service, it looks like nothing extra will be needed in the snap at all for it to work, you just need the service.