[GSoC] Qubes-MIME-Handlers Weekly Progress Report #6

[Andrew Morgan]

Hello everyone, it's that time of the week again.

On this episode: tests, file managing, something else?!?!

Grab it with format here:
https://blog.amorgan.xyz/gsoc-weekly-progress-report-6.html

Otherwise text-only is below:

---

Hello again! Few different things going on this week so let's just jump
right in.

## Tests

First off a basic set of tests have been written for the
`qvm-file-trust` tool. So far these are all unit tests, and mainly test
sending input to each method in `qvm-file-trust`, ensuring that certain
methods are called with the correct parameters, and anything that is
returned contains the correct data.

In the process of writing these tests I uncovered a few bugs and quirks
here and there in the code, which is awesome because that's exactly what
tests are designed to do. Currently I only test each method directly,
however in the long run there will be tests that send varied input
externally to the program. These will check the output as well as the
trust status of any files that may be involved.

As was evident from my last post, I had a bit of trouble getting used to
some 'pythonic' quirks, as well as the whole concept of mocking.
Nevertheless, after getting a few fundamentals down on how to feed in
fake inputs and data, writing the majority of the tests was mostly just
time-consuming.

I also ran a few passes of [pylint](https://pypi.python.org/pypi/pylint)
over `qvm-file-trust` to really make sure it was clean and following
python's syntactical standards.

I still plan to add a few more tests for the cli tool to the suite, as
well as those for the upcoming untrusted folder watching daemon.

Speaking of which...

## Look out! It's a daemon!

I took a little time this week to start work on the C daemon that will
watch all untrusted folders and mark any files created inside them as
untrusted as well. For starters, I've switched to making it a C++ deamon
instead. As far as I can tell there isn't any noticable performance
difference between the two languages, and C++ comes with a few
abstractions for various low-level things like file handling, meaning
less room for errors and bugs. I'm also much more experienced in C++, so
it seemed to the correct choice.

While following the [Qubes Code Style
Guidelines](https://www.qubes-os.org/doc/coding-style/), I was able to
implement parsing of the global and local rule lists (including
functionality such as comment ignoring and override rules) as well as
being able to call on `qvm-file-trust` and set any file as untrusted
through it. I'm currently working on iterating through the rule list and
setting up an `inotify` watch on each folder (and its subfolders) for
any new files placed within them.

In a very scientific test of setting up a basic C++ program to watch
`/tmp` and holding down tab after `ls /tmp/`, generating many
simulataneous inotify events, the CPU usage of the program was 0%.
Whether it will spike up when creating thousands of files within
untrusted folders remains to be seen, but I'm trying to keep the code as
efficient as possible.

One thing I wasn't entirely sure about was where to send logs. I'm not
sure if I should just log to syslog (and thus journald), or
/var/log/qubes/... (or both). It's simple enough to do either one.

## Water-based File Managers

I've started with modifying some of Dolphin's source code. Unfortunately
I've been having a little bit of trouble with their build tool,
[kdesrc-build](https://kdesrc-build.kde.org/). The amount of source code
necessary to build Dolphin is apparently quite large. Every time I try
to download the full dependencies of Dolphin it seems to stall halfway
through.

This may not be the only way to work on Dolphin, but I haven't seen an
alternative so far...

The other matter is that Whonix's current Dolphin version is 4.14.2,
while an updated Arch system's Dolphin version is 17.04.2 (and source is
17.04.70).

According to [this bug report](https://phabricator.whonix.org/T633),
Whonix should be using KDE Frameworks 5 in v14 (next release), which
should be included in Qubes soon following release. Due to this, I'm
wondering whether I should base my patches on top of Dolphin's latest
code or the legacy v4.14. If the latest, then it may be best to develop
on Arch instead of Qubes, or maybe I could find a pre-release build of
Whonix 14...

For Nautilus I've just been using the latest code and it's going well.
Update on that in a bit!

In any case, I've outlined the required changes that need to be made to
each file manager below:

Nautilus:

1. Expose file open events to extensions

2. Respect not opening file if extension requests not to (upstream may
not like this part :)

Dolphin:

1. Work around the complete prevention of attempting to open files for
which we have no read permissions for (instead check with our cli tool
and decide what to do based on its output)

2. If the file is deemed an untrusted file, attempt to open it with our
dvm-desktop handler

3. Failing that, display an error

That should about cover the functionality. Hopefully it'll be at least
partially upstream-able. I'm open to suggestions if anyone has any :)

That about wraps it up for this week! As always the code is available
[here](https://github.com/anoadragon453/qubes-mime-types).

See you next time!

[Marek Marczykowski-Górecki]

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

Speaking of "pythonic" way, you should really consider using
console_scripts entry point and proper python package, instead of using
hacks for importing file directly. See here:
https://setuptools.readthedocs.io/en/latest/setuptools.html#basic-use
https://setuptools.readthedocs.io/en/latest/setuptools.html#automatic-script-creation
https://github.com/QubesOS/qubes-core-agent-linux/blob/master/setup.py

At this stage the change is still simple:
1. Rename the actual script to qvm_file_trust.py and put in to some
subdirectory (python package name) - like qubesfiletrust, add empty
__init__.py file there too.
2. Add setup.py with appropriate console_scripts entry point (pointing
at `qubesfiletrust.qvm_file_trust:main`).
3. Move tests to that package too
4. Change hacky import into simply `import .qvm_file_trust` or `import
qubesfiletrust.qvm_file_trust`

In Makefile you can install it with:

python setup.py install --root $(DESTDIR)

And to run tests directly from git without installation, add "." (or
full path to the source dir) to PYTHONPATH.

> I also ran a few passes of [pylint](https://pypi.python.org/pypi/pylint)
> over `qvm-file-trust` to really make sure it was clean and following
> python's syntactical standards.

A hint: take a look at pylint + unit tests integration with (Travis) CI
we have here:
https://github.com/qubesos/qubes-core-admin/

I'm not saying to do it now, but you may be interested in some examples.

> I still plan to add a few more tests for the cli tool to the suite, as
> well as those for the upcoming untrusted folder watching daemon.
>
> Speaking of which...
>
> ## Look out! It's a daemon!
>
> I took a little time this week to start work on the C daemon that will
> watch all untrusted folders and mark any files created inside them as
> untrusted as well. For starters, I've switched to making it a C++ deamon
> instead. As far as I can tell there isn't any noticable performance
> difference between the two languages, and C++ comes with a few
> abstractions for various low-level things like file handling, meaning
> less room for errors and bugs. I'm also much more experienced in C++, so
> it seemed to the correct choice.

+1

> While following the [Qubes Code Style
> Guidelines](https://www.qubes-os.org/doc/coding-style/), I was able to
> implement parsing of the global and local rule lists (including
> functionality such as comment ignoring and override rules) as well as
> being able to call on `qvm-file-trust` and set any file as untrusted
> through it. I'm currently working on iterating through the rule list and
> setting up an `inotify` watch on each folder (and its subfolders) for
> any new files placed within them.
>
> In a very scientific test of setting up a basic C++ program to watch
> `/tmp` and holding down tab after `ls /tmp/`, generating many
> simulataneous inotify events, the CPU usage of the program was 0%.
> Whether it will spike up when creating thousands of files within
> untrusted folders remains to be seen, but I'm trying to keep the code as
> efficient as possible.

Slightly more scientific way would be something like this with output
redirected to /dev/null, otherwise most of the time you're waiting for
terminal output... For example:

for x in `seq 100000`; do ls /tmp >/dev/null; done

> One thing I wasn't entirely sure about was where to send logs. I'm not
> sure if I should just log to syslog (and thus journald), or
> /var/log/qubes/... (or both). It's simple enough to do either one.

Either stderr (being redirected to syslog or file by the caller), or
syslog. Leave log file management (including rotation, adding timestamps
etc) to appropriate tools. Those places we still use /var/log/qubes
directly are artifacts of the past mistakes...

> ## Water-based File Managers
>
> I've started with modifying some of Dolphin's source code. Unfortunately
> I've been having a little bit of trouble with their build tool,
> [kdesrc-build](https://kdesrc-build.kde.org/). The amount of source code
> necessary to build Dolphin is apparently quite large. Every time I try
> to download the full dependencies of Dolphin it seems to stall halfway
> through.
>
> This may not be the only way to work on Dolphin, but I haven't seen an
> alternative so far...
>
> The other matter is that Whonix's current Dolphin version is 4.14.2,
> while an updated Arch system's Dolphin version is 17.04.2 (and source is
> 17.04.70).
>
> According to [this bug report](https://phabricator.whonix.org/T633),
> Whonix should be using KDE Frameworks 5 in v14 (next release), which
> should be included in Qubes soon following release. Due to this, I'm
> wondering whether I should base my patches on top of Dolphin's latest
> code or the legacy v4.14. If the latest, then it may be best to develop
> on Arch instead of Qubes, or maybe I could find a pre-release build of
> Whonix 14...

I'd focus on the new version. Patrick, can you confirm the above? What
is the base Debian version for Whonix 14?

> For Nautilus I've just been using the latest code and it's going well.
> Update on that in a bit!
>
> In any case, I've outlined the required changes that need to be made to
> each file manager below:
>
> Nautilus:
>
> 1. Expose file open events to extensions
>
> 2. Respect not opening file if extension requests not to (upstream may
> not like this part :)

I'd not assume this - instead ask on appropriate mailing list. IMO this
should also be acceptable if using clean mechanism for this.

> Dolphin:
>
> 1. Work around the complete prevention of attempting to open files for
> which we have no read permissions for (instead check with our cli tool
> and decide what to do based on its output)
>
> 2. If the file is deemed an untrusted file, attempt to open it with our
> dvm-desktop handler
>
> 3. Failing that, display an error
>
> That should about cover the functionality. Hopefully it'll be at least
> partially upstream-able. I'm open to suggestions if anyone has any :)
>
> That about wraps it up for this week! As always the code is available
> [here](https://github.com/anoadragon453/qubes-mime-types).

And as usual, see comments there.

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZZKCFAAoJENuP0xzK19csSJYIAJQZ4DLTrtDdYLLaQOyqBV2e
9uTr0RMTYJjsMdbt/fC20xhvgdsJlKU8/8MHPOa8dnrt/nSyQLxyxdnJFtkHxgTh
mEoLcb9vEzInaPIBUU5rMnMu0S5vDk0vI0r5pKFLKYMUgh0pC9GuFC2Z3q7esanI
aNT0omcdzzgntszqGYSkjoOCjzo/hk4CST12EV/cTA/NajkPkulbuFoAkmslWo3Y
KkED5kBnzxJXYuQdTfiz8fLT+7wqdQxxSEPRHXRR0Tz1F/qAySuRkT5high5rpb8
wL7t9p+TlmmiM4yFt9by9+HMb2BThj3l8VTQaFS9yiQlRec9Eod6YYOjAQ/QJiE=
=DXuy
-----END PGP SIGNATURE-----

[Patrick Schleizer]

Marek Marczykowski-Górecki:

> Patrick, can you confirm the above? What is the base Debian version
> for Whonix 14?

Debian stretch.

Cheers,
Patrick

[Andrew Morgan]

Very cool, I'll set this up ASAP. Not running the tests manually before
every commit is very welcome :)

>
>> I also ran a few passes of [pylint](https://pypi.python.org/pypi/pylint)
>> over `qvm-file-trust` to really make sure it was clean and following
>> python's syntactical standards.
>
> A hint: take a look at pylint + unit tests integration with (Travis) CI
> we have here:
> https://github.com/qubesos/qubes-core-admin/
>
> I'm not saying to do it now, but you may be interested in some examples

Yeah I wasn't sure if it would be necessary to set up travis on my repo
since the end goal is to integrate it into the main repos at some point.

I'm pretty sure unittest has a pylint plugin I could utilize locally
(and thus before every commit).

Unfortunately only scanning the directory with `ls /tmp<tab>` actually
produces any events, just ls'ing it doesn't. So instead I ran the
following, which should more accurately cover our use case (notified of
files being created in a watched directory):

for x in `seq 100000`; do touch /tmp/asd; rm /tmp/asd; done

Our C++ program seemed to max out at 0.7% CPU, which I think is pretty
good :)

One thing that may take more CPU is when we call our python program to
mark each of these new files as untrusted though. It might not end well
if we have to start up and shut down the python interpreter for every
single new file...

Possible solutions might be:

1. Mark the file as untrusted manually (chmod 0, xattr) through the C++
program. (Con: if we change the definition of an untrusted file down the
line then we have to edit the C++ program as well)

2. Every second or so, grab all newly created files and just do a batch
run of `qvm-file-trust` with all of them. (Con: still need to start the
python interpreter but at least we're not hammering it now)

>
>> One thing I wasn't entirely sure about was where to send logs. I'm not
>> sure if I should just log to syslog (and thus journald), or
>> /var/log/qubes/... (or both). It's simple enough to do either one.
>
> Either stderr (being redirected to syslog or file by the caller), or
> syslog. Leave log file management (including rotation, adding timestamps
> etc) to appropriate tools. Those places we still use /var/log/qubes
> directly are artifacts of the past mistakes...

Thanks, I'll just log to stderr for flexibility then.

[Marek Marczykowski-Górecki]

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

On Tue, Jul 11, 2017 at 03:39:57PM -0700, Andrew Morgan wrote:
> On 07/11/2017 02:55 AM, Marek Marczykowski-Górecki wrote:
> > On Mon, Jul 10, 2017 at 09:38:22PM -0700, Andrew Morgan wrote:
> >> I also ran a few passes of [pylint](https://pypi.python.org/pypi/pylint)
> >> over `qvm-file-trust` to really make sure it was clean and following
> >> python's syntactical standards.
> >
> > A hint: take a look at pylint + unit tests integration with (Travis) CI
> > we have here:
> > https://github.com/qubesos/qubes-core-admin/
> >
> > I'm not saying to do it now, but you may be interested in some examples
>
> Yeah I wasn't sure if it would be necessary to set up travis on my repo
> since the end goal is to integrate it into the main repos at some point.

Well, "set up travis" is quite simple - add .travis.yml (look at
existing repositories), then login at travis-ci.org and enable
repository. Anyway, I don't think it's necessary right now.

> I'm pretty sure unittest has a pylint plugin I could utilize locally
> (and thus before every commit).
>

> > Slightly more scientific way would be something like this with output
> > redirected to /dev/null, otherwise most of the time you're waiting for
> > terminal output... For example:
> >
> > for x in `seq 100000`; do ls /tmp >/dev/null; done
>
> Unfortunately only scanning the directory with `ls /tmp<tab>` actually
> produces any events, just ls'ing it doesn't. So instead I ran the
> following, which should more accurately cover our use case (notified of
> files being created in a watched directory):
>
> for x in `seq 100000`; do touch /tmp/asd; rm /tmp/asd; done
>
> Our C++ program seemed to max out at 0.7% CPU, which I think is pretty
> good :)
>
> One thing that may take more CPU is when we call our python program to
> mark each of these new files as untrusted though. It might not end well
> if we have to start up and shut down the python interpreter for every
> single new file...
>
> Possible solutions might be:
>
> 1. Mark the file as untrusted manually (chmod 0, xattr) through the C++
> program. (Con: if we change the definition of an untrusted file down the
> line then we have to edit the C++ program as well)
>
> 2. Every second or so, grab all newly created files and just do a batch
> run of `qvm-file-trust` with all of them. (Con: still need to start the
> python interpreter but at least we're not hammering it now)

For now I'd ignore the problem. This happens when new untrusted file is
created, which should be rare case assuming proper compartmentalization.
But if it will be an issue, I'd go with 2, to not duplicate definition
of untrusted file (bugs like this could be fatal).

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZZVlRAAoJENuP0xzK19cs5JsH/1SeQzeAOwAwhaYrEXrQu1ef
ek4UgT+KeZakmPPI3ppaUy7Jo71uDHrhc2bEvdgKs5fDHU/WirytAXENhYtuZbEQ
A5x+gPY6VyMCl8/7xXXuUbMUDUOrQ1zzGsxRhH4AClGqUREBU2nIXBaYzqP7v9jf
TMoXsWX7yf7n8eCppHVD9+UJJZ88XlVOFM5wK++Rga0Dei8I5Ew+oWjLPXVcMxf+
RBPm6Wfiq3U8dPH31iKBPf/oAnCoGaDQRVXAc26pz7txFWIGysZxCsvlMwIxBOPy
wtIVFUHT/26v7j/Cx7kKYqHoNkDTPIJYWHBDgfHpHfDKBWSxV0Ua7Ugr0HO0k2I=
=fSih
-----END PGP SIGNATURE-----

[Andrew Morgan]

A good example someone brought up a while ago was starting a build
inside of an untrusted folder. Many, many binary files will then be
created all at once.

I'll do some benchmarks once everything is hooked up to decide what the
best course of action will be.

[Marek Marczykowski-Górecki]

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

That would be very bad idea:
1. In essence it means executing those untrusted files locally (think:
Makefile)
2. We chmod 0 untrusted files, so it wouldn't work

Instead, DispVM should be used for this.

> I'll do some benchmarks once everything is hooked up to decide what the
> best course of action will be.

+1

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZZVvaAAoJENuP0xzK19cs+usH/jDvE1AHlAsfIZBkQat9zVaR
9eK/KMbmR9zOcIooTYHgvvvqiiVqr4Fkc6Xme2OQAJ0EImr0+hTyS6Jt6rRWmzXK
gTmKtAkF74uebqL+oG7epSuAL2DN7/3sf0DYULluOp9h/sEWDPJ+Qoe3nFsO1B8Q
2qJJkGcaFKJD/y610ACa38ZqaymbcJpWcsDooI4kPjGNbI/P+zEw+xte/fLxfn5q
ax4VaT3LLTY5Ed/f0deqIfBbM/ry9kZNwQWWrJnVYjTc7leVQj+Z/GMvVdwlR/WR
l94WtKrSmSeEnavzd3tPs0AsaPjuNNuR3DLUVq4ztowLG0VFASaJHd4a+wTs8xg=
=t3nR
-----END PGP SIGNATURE-----

[Andrew Morgan]

Hey Marek,

Just wanted to quickly point out that the next report is coming out
soon. Just wanted to finish up the C++ daemon beforehand. It can already
track untrusted folders and their subfolders, as well as mark any new
folders or files that are created or moved inside of them.

I'm currently trying to work out a bug where inotify_watch calls will
fail around the 8000th folder that's created or moved in. I'm assuming
this probably has to do with a limit coded somewhere so I'm looking out
for that.

I also packaged qvm-file-trust as a python module. Installation goes
great, but I get some errors after trying to run the installed script:

https://gist.github.com/anonymous/c51749ce9f951e4b53fd4da9ed5a925b

qvm_file_trust.py is placed in /rw/usrlocal/bin/qvm_file_trust.py
qvm-file-trust is placed in /usr/local/qvm-file-trust but is unable to
find the correct module
Just moving qvm-file-trust to /rw/usrlocal/bin/ makes it work, so seems
like it's looking relatively for files...

Also did a test with moving in an enormous folder, daemon took up 16%
CPU for a second in htop then right back to 0%, so seems pretty well
optimized for now. inotify finds all the files and folders in way until
a few hundred milli-seconds, so we may need to scale our period for
calling qvm-file-trust with a list of files down a bit (unless python
can take in 10K+ full filepaths as arguments).

Thanks
Andrew Morgan

[Patrik Hagara]

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

On 07/20/2017 07:42 AM, Andrew Morgan wrote:
> I'm currently trying to work out a bug where inotify_watch calls
> will fail around the 8000th folder that's created or moved in. I'm
> assuming this probably has to do with a limit coded somewhere so
> I'm looking out for that.

Sounds like fs.inotify.max_user_watches sysctl [1] is the culprit (set
to 8192 by default).


Cheers,
Patrik


[1]
https://unix.stackexchange.com/questions/13751/kernel-inotify-watch-limi
t-reached

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJZcFQ1AAoJEFwecd8DH5rlNmEQAIbO/LfURLkToAf64mnAULbZ
IN+sKg+1H/w6Jx7cQvA/GmNk+2av1lxOG9R9mmQR+JbH/DEtBCaY3rjWVEDvmeVf
uMTE9qo8SYA9b74xN1H/HaNG1q9+0ChxNmyi055yyVqElz5q2TaQ0TrpuK2jdvvx
4Cdo8voRvpI7iUG+TNDAajX7OJie5DreKjSNBAmSF4hGM2ppgGCtcLke1to6buAi
R8Nj7iCLqAvbM79tdw2rOpnUn5ZVKDaa4r/tpnM8MM755zZlmTUAP17uFm+iSZU4
WZZQ9sQy13IxpPmiBDt8QAvgqlzwVWcN1CTFBCM9PO5PFJxAktWhU/8azuAVNHdF
+tGDNktj9RWsjtdd9OdnLHlnlAulYymoKMu+V6ZSFgSqYxcYPF8KD2UPRjUbxMzh
wjOpaW9Xu8C1jFB5nVL/ls6Argh89VyU8FbahC3w9wJc6vnd8Sd7BJp/qpFrnmE+
utV86OECDYMCMYq8L9CVIOIzO5/WTVW773H8ZxmGTGIGXYTY5WHllnQq4w8IB38E
2xeFG2PrhYppR+xZ60yoENEkQAJEFeunzAh2y54KVNo9d0YMA/KWqWxaZbAv8d3Z
hKxM5qMzbt/sBkyR/iYe2For1Qc7NYPQ8ODPQ+RlgnXyHwLWpXjKrDAOhsIg+puX
TqDnEhYVVRPjiBJGgQ7f
=/f9Z
-----END PGP SIGNATURE-----

[Andrew Morgan]

On 07/19/2017 11:56 PM, Patrik Hagara wrote:
> On 07/20/2017 07:42 AM, Andrew Morgan wrote:
>> I'm currently trying to work out a bug where inotify_watch calls
>> will fail around the 8000th folder that's created or moved in. I'm
>> assuming this probably has to do with a limit coded somewhere so
>> I'm looking out for that.
>
> Sounds like fs.inotify.max_user_watches sysctl [1] is the culprit (set
> to 8192 by default).
>
>
> Cheers,
> Patrik
>
>
> [1]
> https://unix.stackexchange.com/questions/13751/kernel-inotify-watch-limi
> t-reached
>

That was it, thanks Patrik!

So should we just up this from the daemon or from within the template?
Is there any reason not to I wonder?

Andrew Morgan

[Andrew Morgan]

Ah just checked the stackoverflow source, seems as if memory is the only
concern. We definitely don't want the daemon to be unable to correctly
mark files, so either we set it appropriately high and/or warn the user
when we've reached the limit.

>
> Andrew Morgan
>

Andrew Morgan

[Patrik Hagara]

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

You're welcome. :)

Yes, simply printing a warning is what I've seen programs like Dropbox
and minidlna do.

If your daemon is running with root privileges, it could modify the
sysctl itself (but it's an ugly hack). If not, adding the sysctl call
to /rw/config/rc.local would work on a per-VM basis (either manually
by user or automatically by daemon). Setting the limit globally in a
template would not waste any extra kernel memory in AppVMs that don't
make use of inotify watches (ie. don't run the daemon) and would be
the cleanest option IMO. And it could even be done via a post-install
scriptlet of your new package.


Cheers,
Patrik

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJZcFpXAAoJEFwecd8DH5rlmM4QAI6PGHnpaG/MR6UkEd02152N
WXCeODX6JjxfxTzAf64ll3eitdATJT+g3yU0E4XEj+qa8DzfPgMwDFrnOPXMQuK6
NIEqBib4yoy6OgQ7e9i2ns8k4qy5dVM8547hnAVVQsD3p4ruJi/AOyKeDFZJlreq
n2nubxm15LviMNw6mIq5ktYUY1+Xr0Zi4hziwcrezgwmyfuYs9D2OSCyQWLjUQFz
WdByy1vYJYEU0y9BdgoRGo4l+At4DuBNClamgvTOFtuD4U7psdu1SQcpMR3wGHdm
Tm23fvMEfsvsQwjckJS3dq9QWJY6pKcHaOCfnNYzM0dD7bHhKfAvL8xUapgSFc+3
cWBiwyv1Z0OxZV9Obw/wBeSGzIgrboZH0nMNZecoeYc79+V39NV4zZ2iGJNECr1/
DDUExU7jPwfmexce2rGwUCm02CCl6+EvScpaG2kyU6YgjkpiA5AZ7F4uxm5yRw3N
gDZ1cVeOvv/pyUAm0igDVQyGNFBLbQWOf0hyLz7soP1SNOA+LUKK2IGcc1osONQ0
QgwhJHgYxkBPPzVEzcEIKWOEAgQkfNUi7SI/Uqstq3gUApw+Yz9PDHXUWyOJnqsj
PpVVSp4GCLSxMQRvcFZZJmY/FuWXND0fXjMU2eVSS37Lw5qopd79/K9lAp/1ZKWl
VzLx5sgHu2myl4K+KwOJ
=dZQs
-----END PGP SIGNATURE-----

[Andrew Morgan]

Fair point! I'll add it to the Makefile for now.

Andrew Morgan

[Jean-Philippe Ouellet]

On Thu, Jul 20, 2017 at 1:42 AM, Andrew Morgan <an...@openmailbox.org> wrote:
> Also did a test with moving in an enormous folder, daemon took up 16%
> CPU for a second in htop then right back to 0%, so seems pretty well
> optimized for now. inotify finds all the files and folders in way until
> a few hundred milli-seconds, so we may need to scale our period for
> calling qvm-file-trust with a list of files down a bit (unless python
> can take in 10K+ full filepaths as arguments).

During exec(2), the kernel places arguments somewhere at the top of
the stack, along with your environment variables and some other stuff.
Thus, the limit is likely actually some number of total bytes (also
dependent on other things like the total size of your current
environment), rather than the limit being only a fixed number of
arguments. This means you would have to check not just the number of
arguments, but the sum of the lengths of each.

If you find yourself running into problems with to much data in argv
for a single exec, you may wish to consider letting xargs handle
splitting the paths into an appropriate number of separate execs of
your python script. This is one of the reasons it exists. If you do
this, be sure to split the paths with '\0' and use xargs -0.

Consider this example:
$ cat argc.c
#include <stdio.h>
int main(int argc) { printf("%d\n", argc); }

$ make argc
cc argc.c -o argc

$ yes AAAA | head -$((1024*100)) | xargs ./argc
26214
26214
26214
23762

$ yes AAAAAAAAAAAA | head -$((1024*100)) | xargs ./argc
10082
10082
10082
10082
10082
10082
10082
10082
10082
10082
1591

You may also wish to set an artificially small max length to guard
against any potential edge cases which xargs itself may have or may
develop in the future which may cause final arguments to get dropped
or truncated, as such bugs may be unlikely to be found and may have
very bad consequences (files not being marked as untrusted).

Cheers,
Jean-Philippe

[Jean-Philippe Ouellet]

Either with xargs -s, or in your own script if you don't use xargs.
The same concern exists either way.

ISTM that being extra cautious at the expense of a few extra execs is
a good trade-off. If performance really mattered you wouldn't be
execing in the first place.

[Marek Marczykowski-Górecki]

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

On Wed, Jul 19, 2017 at 10:42:36PM -0700, Andrew Morgan wrote:
> Hey Marek,
>
> Just wanted to quickly point out that the next report is coming out
> soon. Just wanted to finish up the C++ daemon beforehand. It can already
> track untrusted folders and their subfolders, as well as mark any new
> folders or files that are created or moved inside of them.
>
> I'm currently trying to work out a bug where inotify_watch calls will
> fail around the 8000th folder that's created or moved in. I'm assuming
> this probably has to do with a limit coded somewhere so I'm looking out
> for that.
>
> I also packaged qvm-file-trust as a python module. Installation goes
> great, but I get some errors after trying to run the installed script:
>
> https://gist.github.com/anonymous/c51749ce9f951e4b53fd4da9ed5a925b
>
> qvm_file_trust.py is placed in /rw/usrlocal/bin/qvm_file_trust.py
> qvm-file-trust is placed in /usr/local/qvm-file-trust but is unable to
> find the correct module
> Just moving qvm-file-trust to /rw/usrlocal/bin/ makes it work, so seems
> like it's looking relatively for files...

1. You should place setup.py one level up - outside of "qubesfiletrust"
directory - so it could find such a package relative to setup.py
location
2. You don't need qvm_file_trust.py in "scripts" - I guess this
is a workaround for point 1.
3. (unrelated) You've committed to git qubesfiletrust.egg-info, which is
generated by setup.py.

> Also did a test with moving in an enormous folder, daemon took up 16%
> CPU for a second in htop then right back to 0%, so seems pretty well
> optimized for now. inotify finds all the files and folders in way until
> a few hundred milli-seconds, so we may need to scale our period for
> calling qvm-file-trust with a list of files down a bit (unless python
> can take in 10K+ full filepaths as arguments).

As Jean-Philippe already said, better split to multiple calls.

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZcQYmAAoJENuP0xzK19cs7qcH/iptsMhTd/H0U3ODaDjsRlRK
ZwC5EvF83TRZC1SuOPC2fMCpqU0lw5nSqFl40h+p1ETx0ipqghwLqHh/qHU0ytQ8
SeCqSb4JXrRFAGpnAInxuv4y+ul2e6irjHfjOq9SI1nB9YFH4itupe/FbVTjWeGp
6dCkNvwvUcaaCZrA6R64L63d3HRihomDnl5uyDXIcLFfgKqLK6NkPBJfbBsV63CE
mohDmh2sO/SpVpq7UdKJk4tpmwZ9WC18v5uKtLkNcNOvveX+ZJyDx6Xu6l/6hFg8
M1/xDsLf0AdNhnBTG6F8968LYpzgwl43orgkD91ZVvc8eNK3gz8lQWnFJnuJ9J0=
=vNYV
-----END PGP SIGNATURE-----

[Andrew Morgan]

On 07/20/2017 08:27 AM, Jean-Philippe Ouellet wrote:
> On Thu, Jul 20, 2017 at 11:22 AM, Jean-Philippe Ouellet <jpo-PjA...@public.gmane.org> wrote:

That seems like a pretty nice solution, thanks Jean-Philippe! With this
we should be able to set the periodic runtime of the script to any
reasonable value without worry that the hardware will outpace it.

I'll give it a shot and let you know how it goes.

Thanks
Andrew Morgan

[Andrew Morgan]

Thanks Marek, I've made those changes and it now installs in
/usr/local/bin instead of /usr/bin. I don't see any other qubes tools in
there so I assume we should either install in /usr/bin or at least
symlink to it. Do you know how we could achieve that?

Andrew Morgan

[Marek Marczykowski-Górecki]

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

I guess you're doing that on Debian, right? Debian have a specific patch
on python setuptools to install everything into /usr/local, unless you
explicitly request some other place. One way to request /usr is to add
- --install-layout=deb option.

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZcUAcAAoJENuP0xzK19csaKAH/1s693Gh2xLIobo270QNkpTR
sro8fjd2ImTtBGg4svzT4N8Wn8ihj06TvXLoXkQ1cAx1RIYoiLPOFavk0W2cIoWE
P2vCk44es/7nyob56zM5q+eEHF3dPVKZc5A9R+sfyQ5o2JVPOdluQ5WVK5aoMOXt
KSj35nCX1axl4gx5CcvYvtyoghzZp49ziOnEjgm2rxziQrzWvgft/HnC3z+ICcQU
dQfYWY9LO7XqPxhbEx0WAeDhxaafggKsYjf2MQqE8BXPfULHORU13mg2xc+1YFwB
8idXtlITq2/FmEBISHq27Lpt3gGU7376cKNtO2XVFqQy/LDTP4ZYz9jGXhZVno8=
=0q90
-----END PGP SIGNATURE-----

[Andrew Morgan]

> --install-layout=deb option.
>
>

Yep that worked wonders, thanks!

Andrew Morgan

[Andrew Morgan]

On 07/20/2017 08:27 AM, Jean-Philippe Ouellet wrote:
> On Thu, Jul 20, 2017 at 11:22 AM, Jean-Philippe Ouellet <jpo-PjA...@public.gmane.org> wrote:

So the exec* family of C functions separates char pointers by spaces,
and it doesn't seem to be configurable, thus I may have to keep the
space separation but escape spaces in the argument list.

user@dev$ echo "hello there" this is a test for many words and xargs in
one go | xargs -s 24 ./argc
5
5
4
3
2
user@dev$ echo "hello\ there" this is a test for many words and xargs in
one go | xargs -s 24 ./argc
4
5
4
3
2

I'll note it _only_ works if there is a preceding backslash and the
words are surrounded by double-quotes.

Again I'm not entirely sure if a workaround for the large amount of
arguments I'm handing python is needed, but one strong benefit of using
xargs (or a similar method) is the ability to split up the list and
parallel calls. Since the script simply marks each file as untrusted
when called from the daemon, it should be fine to parallelize.

Another blocker is that the script current sys.exit's on an error. This
behavior is undesirable as depending on which file errors out, all
subsequent files will not attempt to be marked. In this case, is it best
to thus catch the call, store the error number, then return the error at
the end of processing all files regardless of what it may be?

As long as that number is only overridden by erratic behavior, then any
script calling it should detect the non-zero return code and act
accordingly. The only issue would be if the calling script attempted to
act based on the specific error number which may be overridden during
execution with a later error.

Probably not something we need to worry about at the moment, continuing
execution is more important, just wanted to get people's thoughts on it.

Andrew Morgan

[Jean-Philippe Ouellet]

Err... not sure what you mean by that. Perhaps you are confusing
exec's behavior with echo's?

> and it doesn't seem to be configurable, thus I may have to keep the
> space separation but escape spaces in the argument list.
>
> user@dev$ echo "hello there" this is a test for many words and xargs in
> one go | xargs -s 24 ./argc
> 5
> 5
> 4
> 3
> 2
> user@dev$ echo "hello\ there" this is a test for many words and xargs in
> one go | xargs -s 24 ./argc
> 4
> 5
> 4
> 3
> 2
>
> I'll note it _only_ works if there is a preceding backslash and the
> words are surrounded by double-quotes.

That's why I suggested using xargs -0 and splitting filenames with '\0'.

Consider the following:

$ cat dumpargs.c
#include <stdio.h>

int
main(int argc, char *argv[])
{
int i;
printf("%d args:\n", argc);
for (i = 0; i < argc; i++)
printf("\targv[%d]: %s\n", i, argv[i]);
return 0;
}

$ make dumpargs
cc dumpargs.c -o dumpargs

$ split0() { for x in "$@"; do printf "%s\0" "$x"; done }

$ split0 "hello there" this is a test for many words in xargs in one
go | hexdump -C
00000000 68 65 6c 6c 6f 20 74 68 65 72 65 00 74 68 69 73 |hello there.this|
00000010 00 69 73 00 61 00 74 65 73 74 00 66 6f 72 00 6d |.is.a.test.for.m|
00000020 61 6e 79 00 77 6f 72 64 73 00 69 6e 00 78 61 72 |any.words.in.xar|
00000030 67 73 00 69 6e 00 6f 6e 65 00 67 6f 00 |gs.in.one.go.|
0000003d

$ split0 "hello there" this is a test for many words in xargs in one
go | xargs -0 -s24 ./dumpargs
2 args:
argv[0]: ./dumpargs
argv[1]: hello there
4 args:
argv[0]: ./dumpargs
argv[1]: this
argv[2]: is
argv[3]: a
3 args:
argv[0]: ./dumpargs
argv[1]: test
argv[2]: for
3 args:
argv[0]: ./dumpargs
argv[1]: many
argv[2]: words
4 args:
argv[0]: ./dumpargs
argv[1]: in
argv[2]: xargs
argv[3]: in
3 args:
argv[0]: ./dumpargs
argv[1]: one
argv[2]: go

Trying to guarantee different parsers escape things & interpret
escapes the same way is a battle best avoided entirely.

[Marek Marczykowski-Górecki]

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

No, you're observing "xargs" behaviour.

> and it doesn't seem to be configurable, thus I may have to keep the
> space separation but escape spaces in the argument list.



> user@dev$ echo "hello there" this is a test for many words and xargs in
> one go | xargs -s 24 ./argc
> 5
> 5
> 4
> 3
> 2
> user@dev$ echo "hello\ there" this is a test for many words and xargs in
> one go | xargs -s 24 ./argc
> 4
> 5
> 4
> 3
> 2
>
> I'll note it _only_ works if there is a preceding backslash and the
> words are surrounded by double-quotes.

As Jean-Philippe pointed before, use \0 for separating arguments (-0,
- --null), _instead_ of spaces:

$ echo -e '"hello there"\0this\0is\0a\0test\0for\0many\0words\0and\0xargs\0in\0one\0go' | xargs -0s 26 ./argc
3
6
3
5

$ echo -e 'hello there\0this\0is\0a\0test\0for\0many\0words\0and\0xargs\0in\0one\0go' | xargs -0s 26 ./argc
3
6
3
5

> Again I'm not entirely sure if a workaround for the large amount of
> arguments I'm handing python is needed, but one strong benefit of using
> xargs (or a similar method) is the ability to split up the list and
> parallel calls. Since the script simply marks each file as untrusted
> when called from the daemon, it should be fine to parallelize.

I wouldn't worry about parallel execution now. Lets have it working
first. But yes, using xargs makes it easy too.

> Another blocker is that the script current sys.exit's on an error. This
> behavior is undesirable as depending on which file errors out, all
> subsequent files will not attempt to be marked. In this case, is it best
> to thus catch the call, store the error number, then return the error at
> the end of processing all files regardless of what it may be?

Probably, something like:

retcode = 0
for filename in ...:
try:
do something
except ...:
... log exception
retcode = 1

sys.exit(retcode)

> As long as that number is only overridden by erratic behavior, then any
> script calling it should detect the non-zero return code and act
> accordingly. The only issue would be if the calling script attempted to
> act based on the specific error number which may be overridden during
> execution with a later error.

It should be easy to define exit codes, like "exit code X means _any_ of
files is Y". Anyway IMO it's fine to allow multiple filenames only while
changing attributes.

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZdQNlAAoJENuP0xzK19cssDYH/2Zt/ib1hN7koqbHn63SNJmL
RWXD+Ozmv7gX1IIB89ENuBz43fy2QiyHxZCHBehhsLmN8hZEshZL1LQ2+5/MCo4F
/I9e3nfekz+q3aH+Fz88TBwWuXuXP9VNNr8LYOkXOhLRdrgvpsr1oUReeIdvNtid
5aCnCS8rE40dAhvG/zjUdhV4k8csP7TevxquuFCLbHsM4znj1cGovDP/CcEPaB47
Jt4SnhsOa0m58huqsNGyLFQXSRsD5NkICUKKpzy5RPB8cZgdG47CtozXzgenbSlM
N4NOiPuq9BXUlyl7chPdHw/Ve84K5dPpSMboYsPdLbnlUyrzQDDwe0nBXsk02uk=
=ie4u
-----END PGP SIGNATURE-----

[Andrew Morgan]

On 07/23/2017 12:58 PM, Jean-Philippe Ouellet wrote:

Er, sorry. I'm referring to the following lines of C/C++ code I have in
qubes-trust-daemon:

// Fork and attempt to call qvm-file-trust
switch (child_pid=fork()) {
case 0:
// We're the child, call qvm-file-trust
execv("/usr/bin/qvm-file-trust", (char **) qvm_argv);

// Unreachable if no error
perror("execl qvm-file-trust");
exit(1);
...

Filenames are contained within qvm_argv and are passed to qvm-file-trust
as arguments separated by space. As far as I can tell this isn't
configurable.

[Marek Marczykowski-Górecki]

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

And here you are wrong. Arguments are passed exactly as you given them
in qvm_argv, regardless of spaces inside.
Also, remember that argv[0] should be a program name...

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZdZrXAAoJENuP0xzK19cs/QwH/joTKNdA/QH5MhXViuIccTTO
8eTDWsjz0EAa0MHHk37VrQJwTQKD5uYHBWGMKmhuKQkqVQ/WWP9xwR/SDH2SXqLQ
3ZuVFx75HEjUUB339AvcC6yHHYRs+6V8/N9YkJavm5rwv+Bo0fSrxiCRcVIf/rYN
jhnv0ZjInFFIbKVmE1Xnr0AASP4gjOmTQ04//CJuDeQ2paFiq6nlcNEnbXyeSoV5
jEWXIq+Uq29t3oM4GiCjhqHPTH9ksmlEns57Cy1j0ya/rAavJZXGjyxOp7PzWj/x
pMeKUkWsG/2WjYHjrtaC/D0MGyHXna+TDLi9IztYqnXt8/IA1AIVfSYhWgy4I1I=
=ZRoU
-----END PGP SIGNATURE-----

[Andrew Morgan]

Yes, a few lines before argv[0] is set to the program name and any
arguments are included before file paths are pumped into the array :)

As for the arguments situation, if I understand correctly execv launches
a program and hands it its *argv array as if taken from stdin. If that
is so, would it then be correct to simply instead use execl in the
following manner?

execl("/usr/bin/xargs", "-0", "file name 1\0filename 2\0filename3\0")

Essentially just one long string of all arguments, the final string
being built through a loop? That should work if I'm understanding it
correctly (correct me if I'm not), though not sure if an enormous string
of many, many filenames is desirable.

Alternatively you could simply use execv and insert a null character at
the end of each string (presumably there's one already there though).

While toying around with above solutions, I in the mean time built a
simple splitter in C++ that would handle running a separate instance of
qvm-file-trust after a configurable amount of arguments had been passed.
You can find that implementation here:

https://github.com/anoadragon453/qubes-mime-types/blob/master/qubes-trust-daemon.cpp#L115

I'm not entirely sure if that is enough or if a solution such as xargs
that adjusts to system conditions is necessary, but for now I can say
that the splitter is enough to actually now have qubes-trust-daemon
function properly. It now watches folders and subfolders moved into
watched folders, as well as marking any file it comes across as
untrusted along the way.

I also changed the daemon to mark all files in untrusted directories as
untrusted upon first run, to ensure that if the user added a new
untrusted directory while the daemon was offline, then the files within
would still be marked appropriately.

Andrew Morgan

[Marek Marczykowski-Górecki]

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

The above will pass it as an argument, instead of stdin. xargs is for
transforming data from stdin into argv, it would require setting pipe
for stdin, then writing to it.

But ...

> That should work if I'm understanding it
> correctly (correct me if I'm not), though not sure if an enormous string
> of many, many filenames is desirable.
>
> Alternatively you could simply use execv and insert a null character at
> the end of each string (presumably there's one already there though).
>
> While toying around with above solutions, I in the mean time built a
> simple splitter in C++ that would handle running a separate instance of
> qvm-file-trust after a configurable amount of arguments had been passed.
> You can find that implementation here:
>
> https://github.com/anoadragon453/qubes-mime-types/blob/master/qubes-trust-daemon.cpp#L115

... this looks simpler.
Two additional things there:

1. Check total length of arguments (sum of strlen(file_path). AFAIR
sensible limit should be about 32k. You can simply break from the inner
loop when the limit is reached. Also the outer loop condition could "it
!= file_paths.end()", instead of iteration counter.

2. You should check (and at least log non-zero) exit code of
qvm-file-trust. Failing to mark untrusted file as untrusted may result
in opening it locally and exploiting some bug in local editor, so this
case should have some serious error handling (maybe even removing such
file?).

> I'm not entirely sure if that is enough or if a solution such as xargs
> that adjusts to system conditions is necessary, but for now I can say
> that the splitter is enough to actually now have qubes-trust-daemon
> function properly. It now watches folders and subfolders moved into
> watched folders, as well as marking any file it comes across as
> untrusted along the way.
>
> I also changed the daemon to mark all files in untrusted directories as
> untrusted upon first run, to ensure that if the user added a new
> untrusted directory while the daemon was offline, then the files within
> would still be marked appropriately.

+1

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZdcR4AAoJENuP0xzK19cs1mMH+QGLPrnXzalARNJ8iZFwwtWt
ArsyZGnnymjQ7jpj3TLfFt9RmZ1V0ZD5tFNDYeumWni59R5+rZgsIf0sYNjSvqsF
mNe3LFJBxzLr3vMXliKwTQqY/V3ObEpBDmFoRK1c7bkNz+kybjaXFzUKxlv30NNN
UU781COymYNQosGFJCvw7onYFt6IHW3rbD8Uc70sujIR2UMuvTI6b7qWGjB2vKsq
pgM49CXfJBG4s7sNmdyM+hN4x+J6y1NiEtItmPerO5FZrZhooSYGHy1y5c0aBSVE
LdjI1ZIq6XhwAM+MdkYhissDZ+nUAQmz9JpHMHJOL85fHCOZnu3yO79Fpoycoos=
=dXbV
-----END PGP SIGNATURE-----

[Andrew Morgan]

Yes, that makes sense.

>
> 2. You should check (and at least log non-zero) exit code of
> qvm-file-trust. Failing to mark untrusted file as untrusted may result
> in opening it locally and exploiting some bug in local editor, so this
> case should have some serious error handling (maybe even removing such
> file?).

Hm, we could quarantine it perhaps? Maybe in some folder TrustQuarantine
a file ~/Downloads/folder/file could be moved to
TrustQuarantine/home/user/Downloads/folder/file. This way the user knows
where the file was originally, without us having to touch the file in
any way.

[Marek Marczykowski-Górecki]

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

I'd be careful about rebuilding directory structure, because it could be
quite complex (some VM sending very deep directory like
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/.../some-file).
But maybe I'm too paranoid here? ;)

- --
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-----
Version: GnuPG v2

iQEcBAEBCAAGBQJZdcfMAAoJENuP0xzK19cs73sH/RI3wU+YLpNpddzpIoO6/rQu
ZrUeIxv+0g3xoX63QnszBkYap84UbKXq1Q6ArJFrQAwPeDxh+Mo0UpWsP7aFCagi
UD4RMJFFGH3b20AVgvBv6dK0QXw4BlSbXDkRl81sHZzg2Nu7wC86q/692e1Tc/4s
DqZtzdS10WOYWmbOn+coLJ013VuS0zOdSrAx96pffLFRkNne19EEaI/bh/sElt1X
uKcsvFSQn5FSNtRRW7It9/vonHZVeQ2WcO8FfkSSS8qUItavYV8UHHpeDigITfMi
1qM2wmhQuUc8tSa0LFHU7pTtb9HEbt+84Ffd0JwZhIhrlUSghRVULFtZabzpFXM=
=DhSr
-----END PGP SIGNATURE-----

[Andrew Morgan]

On 07/24/2017 03:11 AM, Marek Marczykowski-Górecki wrote:
> On Mon, Jul 24, 2017 at 03:01:38AM -0700, Andrew Morgan wrote:
>> On 07/24/2017 02:57 AM, Marek Marczykowski-Górecki wrote:
>>> 2. You should check (and at least log non-zero) exit code of
>>> qvm-file-trust. Failing to mark untrusted file as untrusted may result
>>> in opening it locally and exploiting some bug in local editor, so this
>>> case should have some serious error handling (maybe even removing such
>>> file?).
>
>> Hm, we could quarantine it perhaps? Maybe in some folder TrustQuarantine
>> a file ~/Downloads/folder/file could be moved to
>> TrustQuarantine/home/user/Downloads/folder/file. This way the user knows
>> where the file was originally, without us having to touch the file in
>> any way.
>
> I'd be careful about rebuilding directory structure, because it could be
> quite complex (some VM sending very deep directory like
> a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/.../some-file).
> But maybe I'm too paranoid here? ;)
>
>

Yeah not sure what other potential repercussions there might be, but if
there already exists a very deep directory structure inside of an
untrusted folder, I'd assume that moving that structure to a different
folder would not be a big issue, unless the path of the untrusted file
was already right up to the max filepath limit already?

Sounds like an edge case, but overall still seems like the
easiest/simplest solution, if we do decide to implement a quarantine
feature :)

Andrew Morgan

[Jean-Philippe Ouellet]

If quarantining is something of interest, one possibility may be to
take inspiration from the xdg Trash specification [1].

They purposefully do not keep the original directory structures, but
rather just the base filename, and keep a separate index of where the
files came from.

[1]: https://standards.freedesktop.org/trash-spec/trashspec-latest.html

[Jean-Philippe Ouellet]

Oops, I am mistaken. They do indeed keep directory structures if a
whole directory is deleted, but not if files are deleted one at a
time. Similar to placement within ~/QubesIncoming by qvm-copy-to-vm.

Ignore me :)