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

Try poppler's pdftoppm before imagemagick's convert (pdf to png) #47

Open
anarcat opened this issue Feb 28, 2023 · 7 comments
Open

Try poppler's pdftoppm before imagemagick's convert (pdf to png) #47

anarcat opened this issue Feb 28, 2023 · 7 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@anarcat
Copy link
Contributor

anarcat commented Feb 28, 2023

In the FAQ you recommend disabling the security measures in place in Debian and Ubuntu that keep ImageMagick from generating PDFs.

As someone who has work with the Debian LTS security team, I can tell you those measures should not be removed. ImageMagick is an infested nest of security issues, and those measures are there because we could not find a reasonable way to fix all of those issues while keeping the software inside Debian.

I would recommend removing the convert dependency. I haven't looked in details, but it looks like it's only used on restore, to convert the PDF into a raster format zbar can parse. That can be done with something else! Alternatives include poppler (used by dangerzone) or GaphicsMagick, although the latter has similar problems than

poppler also has a pypi wrapper although that's not package in Debian...

i also noticed mchehab/zbar#227 which tries to improve zbar to be able to parse PDFs itself properly, but that also seems similarly error-prone... poppler could probably be used by zbar instead!

anyways, at least make that warning look a little less scary:

anarcat@angela:qr-backup$ ./qr-backup /etc/motd  -o motd.qr.pdf
CRITICAL: Skipping digital restore verification, because 'convert' is not available. Debian/Ubuntu forbid PDF conversion using imagemagick. More information at: https://github.com/za3k/qr-backup/tree/master/docs
anarcat@angela:qr-backup$ 

... at first glance I thought the thing didn't work at all!

thanks for this really interesting software!

@za3k
Copy link
Owner

za3k commented Mar 1, 2023

I don't recommend that, really. If you'd like to link me to a discussion of why you shouldn't remove it, I'd be happy to include it in the FAQ as well.

pdftoppm is a reasonable alternative to 'convert', probably. Great suggestion.

'convert' is indeed used only in restore. This would remove the dependency on 'convert' but not on 'imagemagick' so I'll change the title accordingly. (zbar and python-pillow both use imagemagick)

If you want to patch zbar, I'd encourage you. It's a bit past me, and the maintainer is really only keeping the video4linux drivers up to date as best I can tell.

@za3k za3k changed the title remove imagemagick dependency Try poppler's pdftoppm before imagemagick's convert (pdf to png) Mar 1, 2023
@anarcat
Copy link
Contributor Author

anarcat commented Mar 1, 2023

I don't recommend that, really.

Sorry, lacking context a bit here, recommend what?

If you'd like to link me to a discussion of why you shouldn't remove it, I'd be happy to include it in the FAQ as well.

I'm not sure I have that on hand exactly. But there are actual security advisories where the fix was to do exactly the thing you hint at removing. If you want I can find that, but it's pretty formal "there was a vuln and this is the patch" kind of thing...

pdftoppm is a reasonable alternative to 'convert', probably. Great suggestion.

Awesome, glad to hear that! :)

'convert' is indeed used only in restore. This would remove the dependency on 'convert' but not on 'imagemagick' so I'll change the title accordingly. (zbar and python-pillow both use imagemagick)

Okay, that's interesting, actually. I had noticed the "magick" references in the zbar source code, and this is a concern for me. But at least it's not a direct dependence of your project. In theory, zbar and pillow could both be fixed to reimplement things natively or with other libraries. And they have their own ways of dealing with security issues (or not)...

If you want to patch zbar, I'd encourage you. It's a bit past me, and the maintainer is really only keeping the video4linux drivers up to date as best I can tell.

yeah, I'm not sure I want to go there either, especially since upstream doesn't seem that responsive towards your relatively simple patch. :)

CRITICAL: Skipping digital restore verification, because 'convert' is not available. Debian/Ubuntu forbid PDF conversion using imagemagick. More information at: https://github.com/za3k/qr-backup/tree/master/docs

And how about toning down that message? Would you accept a PR for that? :)

@za3k
Copy link
Owner

za3k commented Mar 3, 2023

I don't recommend that, really.

Sorry, lacking context a bit here, recommend what?

I think you're confusing me with someone's opinion in the stackoverflow discussion I link to. I have my personal opinions, they're not informed enough for me to promote them as recommendations.

In the FAQ you recommend disabling the security measures in place in Debian and Ubuntu that keep ImageMagick from generating PDFs.

zbar has been responsive to my patches in the past, I'm not sure why that particular one got ignored.

I like the message how it is. I think failing to check restore actually is a serious failure. If you think I could make it clearer on the other hand, feel free to suggest something.

@anarcat
Copy link
Contributor Author

anarcat commented Mar 3, 2023

I think you're confusing me with someone's opinion in the stackoverflow discussion I link to. I have my personal opinions, they're not informed enough for me to promote them as recommendations.

i'm specifically refering to this wording in the FAQ:

qr-backup/docs/FAQ.md

Lines 243 to 247 in 5972066

## My self-test is failing on Ubuntu
The generated PDF is probably fine, but can't be read. Debian and Ubuntu have disabled ImageMagick working on PDFs for security reasons. This breaks qr-backup's self-test process. You have two options.
1. Disable or modify the security policy. Check out StackOverflow for information of [why](https://askubuntu.com/questions/1081895/trouble-with-batch-conversion-of-png-to-pdf-using-convert) and [how to disable it](https://askubuntu.com/questions/1127260/imagemagick-convert-not-allowed) if you want.
2. Test your restore by printing your backup.

maybe it's too much of a stretch to say you "recommend" it, let's just call it even and say you refer to this procedure as a workaround. :) at least the wording of the error message strongly indicates this problem should be fixed somewhat.

@za3k
Copy link
Owner

za3k commented Mar 3, 2023

OK, looks like the biggest problem with poppler would be platform-compatibility (largely linux-only atm). So we wouldn't remove imagemagick as an option, just try poppler first.

@anarcat
Copy link
Contributor Author

anarcat commented Mar 4, 2023

looks like the biggest problem with poppler would be platform-compatibility (largely linux-only atm).

I frankly don't care much about other platforms, personnally.. there's typically a way to do things there, either with Windows Linux Subsystem or MacOS homebrew...

That said, it looks like poppler does have "unofficial CI" for Mac and Windows here:

https://poppler.freedesktop.org/#ci

so i bet it's installable there...

So we wouldn't remove imagemagick as an option, just try poppler first.

but yeah that works too.

@anarcat
Copy link
Contributor Author

anarcat commented Mar 8, 2023

oh, just realized another thing... some apps have started migrating away from poppler towards mupdf, e.g.

https://blog.kowalczyk.info/article/2f72237a4230410a888acbfce3dc0864/lessons-learned-from-15-years-of-sumatrapdf-an-open-source-windows-app.html

i was about to suggest using mupdf instead, remembering dealing with poppler security issues in the past, but after browsing those two pages:

https://www.cvedetails.com/product/24992/Freedesktop-Poppler.html?vendor_id=7971

https://www.cvedetails.com/product/20840/Artifex-Mupdf.html?vendor_id=10846

.... I can't say I recommend either of those, security wise. but yeah, i just found out that mupdf also ships a mutool that can convert things back and forth with PDF, so that might be useful as a workaround here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants