search

Islandora-Labs / islandora_vagrant

Islandora testing and development environment
GNU General Public License v3.0
34 stars 38 forks source link

Bug: Recent ImageMagick policy file doesn't permit creation of image derivatives from PDFs #161

Closed grf closed 5 years ago

grf commented 5 years ago

Apparently this is a common issue with recent ImageMagick distributions. I am using the OVA distribution of islandora 7.x-1.12 from https://wiki.duraspace.org/display/ISLANDORA/Release+Notes+and+Downloads

The /etc/ImageMagick/policy.xml file needs to be changed along the lines of:

<policy domain="coder" rights="read|write" pattern="PS"/> <policy domain="coder" rights="read|write" pattern="EPS"/> <policy domain="coder" rights="read|write" pattern="PDF"/> <policy domain="coder" rights="read|write" pattern="LABEL"/> <policy domain="coder" rights="read|write" pattern="XPS"/>

As distributed in the 7.x-1.12 OVA image, the rights are set to "none" which causes thumbnail and medium image derivations to fail.

DonRichards commented 5 years ago

I can verify a PDF ingest causes errors and this needs to be corrected and the suggested fix does work. I'll look into correcting this Ingest Error

Terminal

bryjbrown commented 5 years ago

I created a JIRA ticket for this so we have a number to reference in PRs: https://jira.duraspace.org/browse/ISLANDORA-2372

I will also try to get a fix in for this.

DiegoPino commented 5 years ago

@bryjbrown it could be cool (good, nice, or any other positive adjective could work too) if we add this as part of the documentation of any islandora module that makes use of imageMagick too (large image, simple image, etc)

br2490 commented 5 years ago

Sorry I can't catch this myself right now. I encountered this and IIRC it was the policy. If this can wait about a week I'd be glad to add it to my todo list

@bryjbrown: the files here may help as a starting point https://github.com/Islandora-Collaboration-Group/isle-apache/tree/master/rootfs/usr/local/etc/ImageMagick-7

bryjbrown commented 5 years ago

@grf @DonRichards @DiegoPino @br2490

Tested this today on the current build, and confirmed (third confirmation here) that PDFs cmodels and any PDF-centric cmodels (citations and theses) fail to create derivatives and get the same errors @DonRichards screenshotted.

I also confirmed that changing /etc/ImageMagick/policy.xml to what @grf suggested in the first post:

<policy domain="coder" rights="read|write" pattern="PS"/>
<policy domain="coder" rights="read|write" pattern="EPS"/>
<policy domain="coder" rights="read|write" pattern="PDF"/>
<policy domain="coder" rights="read|write" pattern="LABEL"/>
<policy domain="coder" rights="read|write" pattern="XPS"/>

fixes the problem and PDF-centric cmodels build derivatives fine.

The problem is that I can't find any place where this policy.xml file is getting written. I'm assuming that it gets created automagically when ImageMagick is installed, but I can't find any scripts that are doing that, either. Perhaps ImageMagick is preinstalled as part of whatever base box islandora_vagrant_base_box is using?

Either way, I think that the right place to fix this would be in islandora_vagrant_base_box, but that won't build due to Pear being out of commission, so I guess this will have to be put on hold until its up again.

DonRichards commented 5 years ago

Sorry, I meant to link to a PR. I created a branch but forgot to submit the PR. Here it is. https://github.com/Islandora-Labs/islandora_vagrant_base_box/pull/40

DonRichards commented 5 years ago

Sorry @bryjbrown I wrote this before I saw your response.

bryjbrown commented 5 years ago

@DonRichards Amazing work! This passes the ol' eyeball test as @bondjimbond would say. The only thing I would ask is that @grf's original posted solution involves the line <policy domain="coder" rights="read|write" pattern="LABEL"/> in it, but your PR does not. I'm not sure what this line means or what it does, as I'm not an ImageMagick expert.

@DonRichards or @grf can either of you confirm what this does? I don't want to leave it out if its important, since my testing this morning included the line and I haven't tested without it.

br2490 commented 5 years ago

@bryjbrown: IM's policy.xml can set a variety of run time parameters. The coder policies are security-type features that determine which encoders can be utilized. The default action is to permit unless specified in the policy, and the shipped policy disabled encoders Islandora relies on. @DonRichards' PR resolves this by explicitly permitting access to those encoders.

DonRichards commented 5 years ago

@bryjbrown the "LABEL" coder is blacklisted as part of the CVE-2016-3714 remote code execution exploit. Here's an easy to read breakdown.

DonRichards commented 5 years ago

TL;DR "It is possible to get content of the files from the server by using ImageMagick's 'label' pseudo protocol"

bryjbrown commented 5 years ago

@DonRichards So https://github.com/Islandora-Labs/islandora_vagrant_base_box/pull/40 was merged, but we still need for a fresh-built version of that base box with your new changes to be uploaded to the Vagrant cloud so that it can be pulled down and used by islandora_vagrant. https://jira.duraspace.org/browse/ISLANDORA-2372 can't officially be closed until the issue is gone from a fresh VM.

DonRichards commented 5 years ago

@dannylamb When you get a chance could you please cut a new vm version?

bryjbrown commented 5 years ago

@dannylamb ping?

dannylamb commented 5 years ago

@bryjbrown @dannylamb Just got back from an extended spring break. Thanks for being patient.

I think I can do this without timing out over and over again. Time to verify that my magic method for country internets works.

dannylamb commented 5 years ago

vagrant up :wave: :game_die:

dannylamb commented 5 years ago

Whelp, that took forever. The base box always takes a long time to build for me, but it was excessive today. Ubuntu's package servers were having serious issues today, apparantly. I've exported the box and am uploading now.

dannylamb commented 5 years ago

v1.0.9 is up at https://app.vagrantup.com/islandora/boxes/islandora-base

DonRichards commented 5 years ago

Fix merged and version updated

bryjbrown commented 5 years ago

@DonRichards What do you mean by "Fix merged and version updated"?

bondjimbond commented 5 years ago

@DonRichards Last commit was 14 days ago...?

bryjbrown commented 5 years ago

@DonRichards @bondjimbond yeah, I'm not sure what, other than the new vagrant box @dannylamb pushed, has changed. Either way, we can't close this ticket yet because the issue isn't resolved. The Vagrantfile still specifies to use box 1.0.7, so until we change that the VM still doesn't make PDF derivatives. Working on a PR now.

bryjbrown commented 5 years ago

@DonRichards @bondjimbond https://github.com/Islandora-Labs/islandora_vagrant/pull/163

bryjbrown commented 5 years ago

@DonRichards @bondjimbond @dannylamb @grf @br2490 With https://github.com/Islandora-Labs/islandora_vagrant/pull/163 being merged, I have tested this on a fresh VM and can verify that new objects of PDF-based cmodels are indeed getting derivatives again.