Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test

[Christian Schoenebeck]

On Donnerstag, 22. Oktober 2020 11:07:49 CEST Greg Kurz wrote:
> > Ok, I found the problem on the mentioned box that failed to create hard
> > links with 9p: it is libvirt auto generating AppArmor policy rules for 9p
> > export pathes, which libvirt generates with "rw" AA (AppArmor)
> > permissions. Once I manually adjusted the AA rule to "rwl", creating hard
> > links worked again.
> > 
> > I guess I'll send a patch for libvirt to change that. At least IMO
> > creating
> > hard links with 9pfs is a very basic operation and should be enabled for
> > the respective 9p export path by default.
> > 
> > Actually I think it should also include "k" which means "file locking", as
> > file locking is also a fundamental operation with 9pfs, right?
> 
> Well, I don't know exactly why libvirt is generating a strict AppArmor
> policy but I'm not that surprised. If the user can _easily_ change the
> policy to fit its needs, it's fine to have a "better safe than sorry"
> default.

Alone identifying the root cause of this is not that easy I would say. And if
it takes me quite some time to find it, then I imagine that other people would
struggle with this even more.

A large portion of software, even scripts, rely on being able to create hard
links and locking files. Right now they typically error out on guest with no
helpful error message.

So you start looking into the logs, don't find something obvious, then strace
on guest side to find the most relevant failing syscall on guest side and see
it was probably link(). Then you have to check several security layers: file
permissions on guest, file permissions on host, effective UID of qemu process.
You try creating hard links directly on host with that UID, works. Next you
check is it qemu's seccomp? Is it host's SELinux? Is it AppArmor?

Even for an experienced sysadmin I doubt it'll be a matter of minutes to
resolve this issue. Now imagine a regular user who just wants to sandbox
something on a workstation.

Looking at libvirt git log, it seems this security policy exists more or less
since day one (SHA-1 29ea8a9b64aac60251d283f74d57690e4edb5a6b, Mar 9 2014).
And I don't see an explanation that would suggest a specific reason for
exactly "rw".

I think something has to be improved here, so I'll challenge by sending a
simple libvirt patch, CCing involved authors, and seeing the feedback:

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 12429278fb..ce243e304b 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1142,7 +1142,7 @@ get_files(vahControl * ctl)
             /* We don't need to add deny rw rules for readonly mounts,
              * this can only lead to troubles when mounting / readonly.
              */
-            if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", 
true) != 0)
+            if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwlk", 
true) != 0)
                 goto cleanup;
         }
     }

Even after this change, this is not a global policy. Allowing hard links and
file locks would only be lifted for the 9p export path.

There would be other options as well of course: e.g. detecting on 9pfs side
whether AppArmor and co are enabled, and log a warning to the user a syscall
failed for that reason. But that would be much more complicated and I wonder
whether it would be worth it.

Best regards,
Christian Schoenebeck