za3k / qr-backup

Paper backup of files using QR codes
Other
127 stars 12 forks source link

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

Open anarcat opened 1 year ago

anarcat commented 1 year ago

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 https://github.com/mchehab/zbar/pull/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 commented 1 year ago

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.

anarcat commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

https://github.com/za3k/qr-backup/blob/597206617cd69f7463410646089ecba637574ebc/docs/FAQ.md?plain=1#L243-L247

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.