qemu smb feature blocked by apparmor

[Christian Ehrhardt ]

From Paul Donohue:
To use qemu's samba server without allowing access to all of /tmp, you can add the following lines to /etc/apparmor.d/abstractions/libvirt-qemu somewhere above the deny rule:

owner /tmp/qemu-smb.*/ rw,
owner /tmp/qemu-smb.*/* rw,

It would be nice if this could be included in the official version of that file...

[Christian Ehrhardt  (paelzer)]

We have had other cases that are breaking due to /tmp being Denied - so the denies to /tmp are taken out on a coming upload already.
The denies were added for bug 1365261 and a few comments in there mentioned that they inhibit other features like samba (but also more like some save/restore actions).

So with /tmp denies being gone, we still have no allows for the smb specific path in /tmp.

[Christian Ehrhardt  (paelzer)]

This is sort of safe because:
- while /tmp could contain anything it is not recommended to put critical data there anyway
- while it would be hard to predict the PID as part of the string (this is not exposed through https://libvirt.org/formatdomain.html) so that virt-aa-helper could generate it it is guarded by the "owner" statement

In fact there already is an abstraction meant for this apparmor.d/abstractions/user-tmp
  # per-user tmp directories
  owner @{HOME}/tmp/** rwkl,
  owner @{HOME}/tmp/ rw,

  # global tmp directories
  owner /var/tmp/** rwkl,
  /var/tmp/ rw,
  owner /tmp/** rwkl,
  /tmp/ rw,

This should be perfectly fine to be added I'd think.

[Christian Ehrhardt  (paelzer)]

This is part of the git for the next upload, but needs some tests to be good.
I intent to suggest the change upstream later on.

[Jamie Strandboge (jdstrand)]

I disagree that blanket access to /tmp should be allowed since it breaks application isolation (see https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1403648/comments/12). Perhaps a better compromise would be to change this:

  # allow access to charm-specific ceph config and silence spurious
  # denials (LP: #1403648).
  /var/lib/charm/*/ceph.conf r,
  deny /tmp/{,**} r,
  deny /var/tmp/{,**} r,

to:

  # allow access to charm-specific ceph config and allow reads
  # on the /tmp directories to silence spurious denials without
  # breaking additional rules (LP: #1403648, LP: #1786159).
  /var/lib/charm/*/ceph.conf r,
  /{,var/}tmp/ r,
  owner /{,var/}tmp/**/ r,

At this point, people are free to add:

  owner /tmp/qemu-smb.*/{,**} rw,

The question then becomes, is the smb functionality in the domain xml in a way that virt-aa-helper can query it at all? Apparently, it is not so I feel it is unreasonable to put it in the default libvirt policy. People can modify /etc/apparmor.d/abstractions/libvirt-qemu for this site-specific addition.

[Christian Ehrhardt  (paelzer)]

Thanks Jamie as always for your "security pair of eyes"!
The denies are already gone for multiple reasons - so yes.

So I read from you that the dir reads themselve as you outlined above, but not all of user-temp abstraction is a safe compromise - ok I'lll go for that.

Yeah qemu-smb functionality in particular is not readable to libvirt currently.
But wit hthe argument that users can add it later we could as well remove half of the rules in there.
It at least has a clear "qemu-smb" part in its name - wouldn't that be acceptable.
So it would overall look like:

I'm following your guidance and drop the user-temp abstraction, instead the suggested change would now look like:

+Description: allow expected /tmp access patterns
+Several cases were found needing this, for example ceph will try to list /tmp
+and the samba feature of qemu will place things in /tmp/qemu-smb.*.
+This is sort of safe because:
+ - While /tmp could contain anything it is not recommended to put critical
+ data there anyway
+ - We restrict general access to dir listing and reading of files owned
+ - While it would be hard to predict the PID as part of the string for the
+ qemu smb feature (this is not exposed through XML so virt-aa-helper
+ can't help) it is guarded by the "owner" statement and a pretty clear
+ qemu-smb infix in the path.
+
+Forwarded: no
+Forward-info: Needs to be proven to work reliably first
+Author: Christian Ehrhardt <email address hidden>
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1786159
+Last-Update: 2018-08-09
+--- a/examples/apparmor/libvirt-qemu
++++ b/examples/apparmor/libvirt-qemu
+@@ -203,6 +203,16 @@
+ /var/lib/charm/*/ceph.conf r,
+ /run/ceph/rbd-client-*.asok rw,
+
++ # various functions will need /tmp, allow the base dir and a few known functions
++ # we want to avoid to give blanket read or even write to everything in /tmp
++ # so users are expected to add site specific addons for more uncommon cases
++ # allow dir listing, but no general file read
++ /{,var/}tmp/ r,
++ # only owner file read
++ owner /{,var/}tmp/**/ r,
++ # qemu smb feature specific path with write access
++ owner /tmp/qemu-smb.*/{,**} rw,
++
+ # kvm.powerpc executes/accesses this
+ /{usr/,}bin/uname rmix,
+ /{usr/,}sbin/ppc64_cpu rmix,

It would make ceph and smb happy already.

Others like the snapshot issue we had at least would get a proper log error (since no more silently denied) and people - as you said can add site specific addons. That is an example with unpredictable paths that I won't add.

Is that an acceptable tradeoff for known features to work?

[Jamie Strandboge (jdstrand)]

It isn't my preference, but I guess it is a reasonable trade-off (indeed, I though you might suggest it :). Better would be if libvirt supported smb shares and we could add the rule (or a more specific one) conditionally, but that isn't the case right now.

[Christian Ehrhardt  (paelzer)]

FYI: Some of these rules are currently in discussion upstream as I summarized the proposed changes to be included there.

[Christian Ehrhardt  (paelzer)]

https://www.redhat.com/archives/libvir-list/2018-August/msg00783.html
https://www.redhat.com/archives/libvir-list/2018-August/msg00784.html

[Christian Ehrhardt  (paelzer)]

After having a good discussion upstream I have to finally nack that for now.
See: https://www.redhat.com/archives/libvir-list/2018-August/msg00957.html

TL;DR: This is too much of a security risk to generally be allowed, people are welcome to open these paths up as a local override on their system, but we won't add it in general.

Setting the bug to Won't Fix