Refactoring PDF Converter (and other scripts)

[Jason Phan]

Hello!

I've recently taken an interest in Qubes and wanted to contribute. I figured a
good starting point would be the shell scripts peppered throughout the project
(e.g., qubes-app-linux-pdf-converter, qubes-app-linux-split-gpg).

Currently, a majority of them lack any real structure or organization. Also, I
believe some improvements can be made in the best practices department as well.
In any case, if there are no objections, I'd like to try to improve these as my
first contribution to Qubes (starting with the PDF converter).

Now, I did have some questions regarding the scripts:

1. Should I assume the scripts are POSIX shell scripts or Bash scripts? Some
have the /bin/sh shebang while others have /bin/bash.
2. Is there a minimum required shell version?

Jason Phan

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Sat, Mar 21, 2020 at 07:11:12PM -0500, Jason Phan wrote:
> Hello!

Hello!

> I've recently taken an interest in Qubes and wanted to contribute. I figured a
> good starting point would be the shell scripts peppered throughout the project
> (e.g., qubes-app-linux-pdf-converter, qubes-app-linux-split-gpg).
>
> Currently, a majority of them lack any real structure or organization. Also, I
> believe some improvements can be made in the best practices department as well.
> In any case, if there are no objections, I'd like to try to improve these as my
> first contribution to Qubes (starting with the PDF converter).

Yes, PDF converter seems like a good starting point.

As for the qubes-app-linux-split-gpg, don't bother, it's going to be
replaced by a rewritten version soon-ish:
https://github.com/QubesOS/qubes-issues/issues/474
https://github.com/HW42/qubes-app-linux-split-gpg2/pull/6

> Now, I did have some questions regarding the scripts:
>
> 1. Should I assume the scripts are POSIX shell scripts or Bash scripts? Some
> have the /bin/sh shebang while others have /bin/bash.

First of all, see https://www.qubes-os.org/doc/coding-style/, which
discourage shell scripts at all, in preference of Python. This isn't
always practical in case of very short scripts (where boilerplate
required in python would be longer than the script itself), but I think
PDF converter left that category long time ago already.
But if you're not into converting shell->python, the current scripts
improvements would be very much appreciated too!

As for the shell scripts, prefer POSIX shell and /bin/sh shebang. In
some cases, where shell is still acceptable, but bash features (like
arrays, or extended string compare) would greatly simplify the script,
it is allowed to use bash, and then obviously set /bin/bash shebang.
The reason for that is bash is quite powerful, but also not very fast
and some distributions (like Debian) use faster alternative for /bin/sh.

> 2. Is there a minimum required shell version?

Basically whatever is present in (oldest) supported distribution:
https://www.qubes-os.org/doc/supported-versions/

This means bash 4.3 (Debian jessie).

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAl52vvoACgkQ24/THMrX
1yzicggAmXw7dBM2NzNpr2uv6AQh58eSVxboPaQIrApfWY2J4lC2+slv6wvVoi7i
EpdbeXycZH5Re00v9CzKaLhJoAvY427hPebVWoqC0u+/W4RD54gtUjB34SEJ6d1g
2hiirNkSniTZgc0PvpQUYwDSt0pnYnbj1tDyPZNwLvJa3Vd4T6b2LG/d5GZxYT80
9YVDaPQRC3+1IJywwy6UBzAAx2qO30AdmJ/4YzsiRvF/LKobFAsdqFCCLO9Nj1op
WdL9u6A8cy0i8WtbdpuGJ0XVopJpT+bhMjO517XWZj07d21AfMFQObmzzYtALOTN
U3xAiAg2il432aeRIPnUZAZZA24j7g==
=lyJF
-----END PGP SIGNATURE-----

[Jason Phan]

On Mar 22, Marek Marczykowski-Górecki wrote:
>As for the qubes-app-linux-split-gpg, don't bother, it's going to be
>replaced by a rewritten version soon-ish:
>https://github.com/QubesOS/qubes-issues/issues/474
>https://github.com/HW42/qubes-app-linux-split-gpg2/pull/6

Gotcha.

>First of all, see https://www.qubes-os.org/doc/coding-style/, which
>discourage shell scripts at all, in preference of Python.

Ah, that's my bad; forgot to check there. But that's fine with me. I'll get
started with Python.

>As for the shell scripts [...]

Gotcha.

Jason Phan

[Jason Phan]

Woohoo! The initial PR has been submitted (though there are some GPG issues on my end).

Besides the port to Python, some other notable features in the PR include

support for multiple PDF files and proper error handling for the qrexec streams.

There's a bit more to do but before I get started on that I have some more questions (sorry):

1. Do I have to add a docstring for every function? I tried to name the functions and

     variables so that their type and usage would be self-explanatory, plus it'd be a bit

     redundant imo since quite a few of the functions take the same parameters.

2. How important is Windows compatibility here? I'm trying to replace as many shell

     commands as possible with pure Python but frankly some of them (e.g., the ImageMagick

     commands) don't have official alternatives and others for some reason don't work

     properly when implemented in Python (at least, with what I've tried so far).

3. The server-side rendering is just screaming to be async-ified. Any objections?

4. It'd be nice to make this program more generic for like .docx and stuff. Thoughts?

5. Right now I'm having all the files processed in the same DisposableVM, should they

     be in separate ones? I would imagine though that that would take quite a bit of resources

     if there were many files.

Thanks!

               Jason Phan

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Thu, Apr 02, 2020 at 01:57:39PM -0700, Jason Phan wrote:
> Woohoo! The initial PR has been submitted (though there are some GPG issues
> on my end).
>
> Besides the port to Python, some other notable features in the PR include
> support for multiple PDF files and proper error handling for the qrexec
> streams.
>
> There's a bit more to do but before I get started on that I have some more
> questions (sorry):
>
> 1. Do I have to add a docstring for every function? I tried to name the
> functions and
> variables so that their type and usage would be self-explanatory, plus
> it'd be a bit
> redundant imo since quite a few of the functions take the same
> parameters.

Yes, self-explanatory names should be enough for simple functions. But
still, more complex functions, or with non-trivial arguments should have
a docstring.

> 2. How important is Windows compatibility here? I'm trying to replace as
> many shell
> commands as possible with pure Python but frankly some of them (e.g.,
> the ImageMagick
> commands) don't have official alternatives and others for some reason
> don't work
> properly when implemented in Python (at least, with what I've tried so
> far).

Right now windows compatibility (especially on the server side) isn't
that important.

> 3. The server-side rendering is just screaming to be async-ified. Any
> objections?

Nothing specifically against. But also be careful about resources - for
example rendering all the pages in parallel is a bad idea, since there
may be many of them (and in fact we do have a test that tries to convert
500 pages PDF file).

> 4. It'd be nice to make this program more generic for like .docx and stuff.
> Thoughts?

On the server (rendering) side that should be fine, it is "just" using a
different tool to render the file. On the receiving side I'd save it to
pdf in any case. For example docx with just images is pretty much
useless.

> 5. Right now I'm having all the files processed in the same DisposableVM,
> should they
> be in separate ones? I would imagine though that that would take quite
> a bit of resources
> if there were many files.

On the other hand, using a single DisposableVM means one malicious PDF
could access/modify other files you're trying to convert.
IMO a better approach would be to use separate DisposableVMs, but
_independently_ optimize their resource usage (for example you don't
need the whole graphical stuff and most of other service just to convert
PDF -> RGB).

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAl6Gfy0ACgkQ24/THMrX
1ywFxAf/VayFSMEQt7sNUutSfWJlfGDMlPExI2Qwh9FRIMtVeyDyiMu4407UfcgG
7JYX7CIyVxbI0lx1emcxFor9WFLfNF07WdnU6bhNDMgU44F/2gdfIj3Ka2w6INBc
+Gxhh1Gw71nCuvrgiWDc+dhVxz0TNCTCyhTFGNYvtSBTFxDKszdsSUj03nW9gk/9
hdRnykWUhBUCiACtk2dfgr+EU6aWzI4TIFlIDA44m2Jg3eGhX3VWU05YNn4jW9Nr
Ble7k4XZ8C/MVmK3MWFCMOsrkceVk9P+SXV4Upo7gYoANZtEg43SCh3qKX+V7B8L
q5UUII+oRw6Bq6svi/OR6ivz+44rbA==
=zWkV
-----END PGP SIGNATURE-----

[Jason Phan]

On Apr 03, Marek Marczykowski-Górecki wrote:
>Yes, self-explanatory names should be enough for simple functions. But still,
>more complex functions, or with non-trivial arguments should have a docstring.

Okay. I'll go through again and make sure that's the case.

>Right now windows compatibility (especially on the server side) isn't that
>important.

W00t!

>Nothing specifically against. But also be careful about resources - for
>example rendering all the pages in parallel is a bad idea, since there
>may be many of them (and in fact we do have a test that tries to convert
>500 pages PDF file).

I was trying to come up with a limit on the number of simultaneous renderings.
Maybe something like half the total # of pages and if that's over some number
like 10 or something, then the limit is 10.

>On the other hand, using a single DisposableVM means one malicious PDF could
>access/modify other files you're trying to convert. IMO a better approach would
>be to use separate DisposableVMs, but _independently_ optimize their resource
>usage (for example you don't need the whole graphical stuff and most of other
>service just to convert PDF -> RGB).

Good point. I'll read some more on Qubes to see how to go about doing that.


Also, I knew I would forget a question... While we can "sanitize" data sent by
the server, we can never really "verify" them since that would require parsing
the PDF client-side. So should variables received from the server always have
the "untrusted_" prefix? That's sort of the logic I'm going with right now.

Jason Phan

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Thu, Apr 02, 2020 at 07:45:00PM -0500, Jason Phan wrote:
> On Apr 03, Marek Marczykowski-Górecki wrote:
> > Yes, self-explanatory names should be enough for simple functions. But still,
> > more complex functions, or with non-trivial arguments should have a docstring.
>
> Okay. I'll go through again and make sure that's the case.
>
> > Right now windows compatibility (especially on the server side) isn't that
> > important.
>
> W00t!
>
> > Nothing specifically against. But also be careful about resources - for
> > example rendering all the pages in parallel is a bad idea, since there
> > may be many of them (and in fact we do have a test that tries to convert
> > 500 pages PDF file).
>
> I was trying to come up with a limit on the number of simultaneous
> renderings. Maybe something like half the total # of pages and if that's
> over some number like 10 or something, then the limit is 10.

Sounds reasonable.

> > On the other hand, using a single DisposableVM means one malicious PDF
> > could access/modify other files you're trying to convert. IMO a better
> > approach would be to use separate DisposableVMs, but _independently_
> > optimize their resource usage (for example you don't need the whole
> > graphical stuff and most of other service just to convert PDF -> RGB).
>
> Good point. I'll read some more on Qubes to see how to go about doing that.
>
>
> Also, I knew I would forget a question... While we can "sanitize" data sent
> by the server, we can never really "verify" them since that would require
> parsing the PDF client-side. So should variables received from the server
> always have the "untrusted_" prefix? That's sort of the logic I'm going with
> right now.

The idea is to verify if they are non-malicious, not necessary
"correct". So, as soon you verify they conform to the expected format,
you can drop "untrusted_" prefix.
Also, be careful about things like server sending too much or too little
data.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAl6GiGwACgkQ24/THMrX
1yzj7Af9GPQVlXdDi4yZYEfAj0Vm1g3DstYn0LjFVgvdIkif2eXohNUhP4UXfIUW
wRSnWs70/9kEyRQH/msLF0I1rEGMQG2O0aj3xiDLG3SWeTgtcH8BQ6aIRekqHlUb
7+z0JhB0x1irurvRgxuLcpBWXz5c9Re6ZwiY3tw+PrtN0tAjUOqC4iKGKsOfJI1C
NdnQA4L8BUlYCxgpQnaC9Js9SW6RJ8AFNK+Y61NgCwQNlZqBI9hQHq4kjft5zLhm
Z3mIpiGRJH/cwbfx2tqo3+3DBH4ib66n31qoNv92LWQrRlx4A0FOpKF1mbUyFHze
1tccnamPZq5UHWV/KIcrj8PUa1TV8w==
=JDZF
-----END PGP SIGNATURE-----

[Jason Phan]

On Apr 03, Marek Marczykowski-Górecki wrote:
>The idea is to verify if they are non-malicious, not necessary "correct". So,
>as soon you verify they conform to the expected format, you can drop
>"untrusted_" prefix.

Ah, gotcha.

>Also, be careful about things like server sending too much or too little data.

Oof, my brain is blanking. Could you give some examples of what could happen if
a server sent too much/little?

Thanks btw for all the help!

Jason Phan

[Demi M. Obenour]

Also, don’t forget about other use-cases, like printing or OCR.
A converted PDF should print just as well as the original.

Marek: is OCR on a converted PDF safe? Being able to reconstruct the
text is very much useful. Also, could this be integrated into CUPS?

Sincerely,

Demi

[Jason Phan]

On Apr 02, Demi M. Obenour wrote:
>A converted PDF should print just as well as the original.

Overall, the resulting PDF is just a tad bit lower quality than the original,
but still perfectly readable on screen/paper imo. I've tried messing around with
ImageMagick's parameters but someone with more knowledge about images will have
to step in if a significant improvement in quality is expected.

>OCR

Ooh! I wanted to do this too.

Jason Phan

[Demi M. Obenour]

On 2020-04-02 21:34, Jason Phan wrote:
> On Apr 02, Demi M. Obenour wrote:
>> A converted PDF should print just as well as the original.
>
> Overall, the resulting PDF is just a tad bit lower quality than the original, but still perfectly readable on screen/paper imo. I've tried messing around with ImageMagick's parameters but someone with more knowledge about images will have to step in if a significant improvement in quality is expected.

I had a problem where a converted PDF didn’t print properly,
probably due to missing DPI information.

>> OCR
>
> Ooh! I wanted to do this too.
>
>            Jason Phan

That sounds awesome!

Another suggestion would be to jail the server in a sandbox using
seccomp and namespaces, to make it harder for the server to attack
Linux (and then Xen).

Sincerely,

Demi

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Thu, Apr 02, 2020 at 09:20:40PM -0400, Demi M. Obenour wrote:
> On 2020-04-02 21:12, Jason Phan wrote:
> > On Apr 03, Marek Marczykowski-Górecki wrote:
> >> The idea is to verify if they are non-malicious, not necessary "correct". So, as soon you verify they conform to the expected format, you can drop "untrusted_" prefix.
> >
> > Ah, gotcha.
> >> Also, be careful about things like server sending too much or too little data.
> >
> > Oof, my brain is blanking. Could you give some examples of what could happen if a server sent too much/little?

For too much - at least running out of memory on the client side. For
too little, depending on a place, most likely you'll get an exception,
but I wouldn't be surprised to see also some corner cases like silently
dropping half of the page.

> > Thanks btw for all the help!
> >
> >            Jason Phan
> >
>
> Also, don’t forget about other use-cases, like printing or OCR.
> A converted PDF should print just as well as the original.
>
> Marek: is OCR on a converted PDF safe? Being able to reconstruct the
> text is very much useful.

That's a tricky question. qpdf-convert-server have significant control
over input for such OCR (within realm of valid image data). So, given
complexity of OCR software, I think nothing can be completely ruled out.
But also, I think (because of guaranteed proper input format) some
catastrophic failure is unlikely.

In fact, I consider another method for preserving text data. Enhanced
"simpler representation", which besides pure image, contains also text
annotations. Thing like series of (coordinates, text) pairs. This needs
careful design, to be reasonably safe (for example defining what "text"
could contain, to not risk re-interpreting it as something else in the
PDF, or some intermediate tool).

> Also, could this be integrated into CUPS?

I don't see why not.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAl6GlX4ACgkQ24/THMrX
1yzTRAf/ZpR89/rXY60DlaDlkKFfpS0/id4InapJC4XMveVxwVcQF8dtyyprzPJ0
0i+jn8NLntBnYVYwYb9K4zyfbEEo36uuDN0emE6/XcdkhjbQ6zldPMTwVIWxphEH
e1VwA0+OvoFEqT1xAQoR+KXbbuQ2F/U1uy9IUaI7TjNxxdhR91WG7r/mA183ZR/P
xkGgXo+oTFDTEeSvMVVDqR3u0fGUbr17cY3ltypcBDrTP9XGvfLKmQT4wEzPKtWg
E93YrsSmSH8OvvYRHAo/wEgZoBlaoA6T+kwMw8FN5CYR9zEfwI3wi8jP04KBGP5U
JKFlhRg9w5t/B9Md0Vl6VUjf77/E7w==
=0qdK
-----END PGP SIGNATURE-----

[Jason Phan]

On Apr 02, Demi M. Obenour wrote:
>I had a problem where a converted PDF didn’t print properly,
>probably due to missing DPI information.

:O Is this with the Python port or the original converter?

>Another suggestion would be to jail the server in a sandbox using seccomp and
>namespaces, to make it harder for the server to attack Linux (and then Xen).

I thought the server being in a DispVM was our sandboxing. Or are you saying in
addition to that we should use seccomp/namespaces.

Jason Phan

[Demi M. Obenour]

On 2020-04-02 21:46, Marek Marczykowski-Górecki wrote:>> Marek: is OCR on a converted PDF safe? Being able to reconstruct the

>> text is very much useful.
>
> That's a tricky question. qpdf-convert-server have significant control
> over input for such OCR (within realm of valid image data). So, given
> complexity of OCR software, I think nothing can be completely ruled out.
> But also, I think (because of guaranteed proper input format) some
> catastrophic failure is unlikely.
>
> In fact, I consider another method for preserving text data. Enhanced
> "simpler representation", which besides pure image, contains also text
> annotations. Thing like series of (coordinates, text) pairs. This needs
> careful design, to be reasonably safe (for example defining what "text"
> could contain, to not risk re-interpreting it as something else in the
> PDF, or some intermediate tool).

That would be absolutely awesome. The biggest problem with
qvm-convert-pdf is the loss of text, and keeping the text would make
it far more usable.

>> Also, could this be integrated into CUPS?
>
> I don't see why not.

Given how insecure printers are, this would be a very good idea.
Perhaps similar technology (possibly based on something like seL4
instead of Xen) could be incorporated into printers themselves.

Sincerely,

Demi

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Thu, Apr 02, 2020 at 09:59:32PM -0400, Demi M. Obenour wrote:
> On 2020-04-02 21:46, Marek Marczykowski-Górecki wrote:>> Marek: is OCR on a converted PDF safe? Being able to reconstruct the

> >> Also, could this be integrated into CUPS?
> >
> > I don't see why not.
>
> Given how insecure printers are, this would be a very good idea.
> Perhaps similar technology (possibly based on something like seL4
> instead of Xen) could be incorporated into printers themselves.

Taking care of the printer itself would make much more difference.
Otherwise, to keep the printer "safe", you would need to ensure that
_everything_, _anyone_ print on that printer gets sanitized first. This
includes all the files you print (which could be solved at CUPS level
indeed), but also other users (if applicable) and (assuming network
printer) anything that could connect to the printer, possibly without
your consent (any compromised device within the local network, possibly
even including your sys-net).

But also redesigning printer firmware is on a totally different level
than writing a python script for sanitizing pdf files...

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAl6GnLwACgkQ24/THMrX
1yw5/Af8Di8VU7u3D4Nj6d0yf7FWXrJxr8usc6/4iabRaTgtBqO6wvivCC0BeUMf
vqkAUxd3CoZ2mGR3LNXfXtobBCr32d54ckERr8LLRcfoaDoXPZ25wFBOAXgwN25G
pupLOldjHMeKqxMjofUiQRBNTnxDWX3i1ICiCJ0le5woC0L7yk7jmDnKRoq2uU9+
uFNjq+zstcw5KxO/IxB25+dcY7ETeyhCMUrfv+plHB4Sck7mwOvDePvzcmdUP3sw
kRqkMxfTDyLshP4zJd8aKC7S7PXAtJx8H7nOdtxBrD61wXJbuUyVONMpgyMRl49+
1kz4roWPC19Tm3UUFdSj5oYQFaSQoA==
=NoKI
-----END PGP SIGNATURE-----

[Demi M. Obenour]

On 2020-04-02 21:48, Jason Phan wrote:
> On Apr 02, Demi M. Obenour wrote:
>> I had a problem where a converted PDF didn’t print properly,
>> probably due to missing DPI information.
>
> :O Is this with the Python port or the original converter?

Original converter

>> Another suggestion would be to jail the server in a sandbox using seccomp and namespaces, to make it harder for the server to attack Linux (and then Xen).
>
> I thought the server being in a DispVM was our sandboxing. Or are you saying in addition to that we should use seccomp/namespaces.
>
>            Jason Phan

Xen is the main sandboxing, but additional sandboxing is definitely
worthwhile.

Sincerely,

Demi

[Jason Phan]

On Apr 02, Demi M. Obenour wrote:
>>> I had a problem where a converted PDF didn’t print properly,
>>> probably due to missing DPI information.

>Original converter

I'll print some stuff tomorrow and see what's what.

>Xen is the main sandboxing, but additional sandboxing is definitely worthwhile.

I think general security mechanisms like that should probably be implemented
outside of qvm-convert-pdf though. Can't say whether we should though, hardening
a DispVM seems unecessary to me given how they're typically used.

Jason Phan

[Demi M. Obenour]

On 2020-04-02 22:39, Jason Phan wrote:
> On Apr 02, Demi M. Obenour wrote:
>>>> I had a problem where a converted PDF didn’t print properly,
>>>> probably due to missing DPI information.
>> Original converter
>
> I'll print some stuff tomorrow and see what's what.

Thank you.

>> Xen is the main sandboxing, but additional sandboxing is definitely worthwhile.
>
> I think general security mechanisms like that should probably be implemented outside of qvm-convert-pdf though. Can't say whether we should though, hardening a DispVM seems unecessary to me given how they're typically used.
>
>            Jason Phan

More than anything else, it is a matter of making an attacker’s
life as hard as possible. Adding a seccomp filter and namespacing
(via bubblewrap) is cheap.

Sincerely,

Demi