On Mon, Aug 13, 2018 at 6:53 PM Jamie Strandboge <jamie@canonical.com> wrote:

On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:

...

virt-manager's UI connection will need socket access for openGraphicsFD to work - otherwise users will face a failed connection error when opening the UI view.

Depending on the exact versions of libvirt and qemu involved this needs either a rule from qemu to libvirt or vice versa.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- examples/apparmor/libvirt-qemu | 3 +++ examples/apparmor/usr.sbin.libvirtd | 5 +++++ 2 files changed, 8 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index df5f512487..5caf14e418 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -188,6 +188,9 @@ @{PROC}/device-tree/** r, /sys/firmware/devicetree/** r,

+ # allow connect with openGraphicsFD to work + unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd),

+1 to apply

...

diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 3102cab382..dd37866c2a 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -69,6 +69,11 @@ unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd//qemu_bridge_helper), signal (send) set=("term") peer=/usr/sbin/libvirtd//qemu_bridge_helper,

+ # allow connect with openGraphicsFD, direction reversed in newer versions + unix (send, receive) type=stream addr=none peer=(label=libvirt-[0- 9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*), + # unconfined also required if guests run without security module + unix (send, receive) type=stream addr=none peer=(label=unconfined),

Makes sense. This libvirtd policy is meant to be super restrictive, so +1 to apply.

Thanks, added your Ack in the v2 submission due to rewriting the latter patches of this series. --

Jamie Strandboge | http://www.canonical.com

-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd

On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:

If a guest runs unconfined <seclabel type='none'>, but libvirtd is confined then the peer for signal can only be detected as 'unconfined'. That triggers issues like: apparmor="DENIED" operation="signal" profile="/usr/sbin/libvirtd" pid=22395 comm="libvirtd" requested_mask="send" denied_mask="send" signal=term peer="unconfined"

To fix this add unconfined as an allowed peer for those operations.

I discussed with the apparmor folks, right now there is no better separation to be made in this case. But there might be further down the road with "policy namespaces with scope and view control + stacking"

This is more a use-case addition than a fix to the following two changes: - 3b1d19e6 AppArmor: add rules needed with additional mediation features - b482925c apparmor: support ptrace checks

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Acked-by: Jamie Strandboge <jamie@canonical.com> Acked-by: intrigeri <intrigeri+libvirt@boum.org> --- examples/apparmor/usr.sbin.libvirtd | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index dd37866c2a..3ff43c32a2 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -74,6 +74,9 @@ # unconfined also required if guests run without security module unix (send, receive) type=stream addr=none peer=(label=unconfined),

+ # required if guests run unconfined seclabel type='none' but libvirtd is confined + signal (read, send) peer=unconfined,

A tad unfortunate, but again, the libvirtd profile is meant to be super strict. +1 to apply -- Jamie Strandboge | http://www.canonical.com

On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:

Several cases were found needing /tmp, 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 only dir listing and reading of files owned (intentionally not the full power of user-tmp abstraction) - 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.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- examples/apparmor/libvirt-qemu | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 5caf14e418..c4f231b328 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -180,6 +180,16 @@ # for rbd /etc/ceph/ceph.conf r,

+ # various functions will need /tmp (e.g. ceph), allow the base dir and a + # few known functions. + # we want to avoid to give blanket read or even write to everything under /tmp + # so users are expected to add site specific addons for more uncommon cases. + # allow only dir listing and owner based file read + /{,var/}tmp/ r, + owner /{,var/}tmp/**/ r, + # allow qemu smb feature specific path with write access + owner /tmp/qemu-smb.*/{,**} rw,

The actual rule additions are a compromise between security and usability. Personally I'd prefer they not be there and let admins add them or allow distros to patch them. That said, the comments and commit message don't seem quite right for what you are adding. Eg, you mention ceph, but there is no ceph rule and the profile comment doesn't mention ceph only needs to list dirs; the profile comments are formatted a bit weird too. You mention 'sort of safe', but because of the '/{,var/}tmp/ r,' rule, you are allowing guests to enumerate all the /tmp/qemu-smb.* directories and then there is a 'rw' rule for that path. While this is an 'owner' match, in practice, VMs all run under the same uid so this means a rogue vm would be allowed to access files in these directories. That said, I don't know what qemu is setting up in there-- so maybe it is designed in such a way that this doesn't matter. I'd much rather not call this 'sort of safe' but instead call out the problem, justify why the rule should be there and perhaps add a TODO that once smb is supported in domain xml that this rule will be added conditionally. +0 for rule inclusion provided the comments and commit message are addressed. +1 if it can be demonstrated that qemu is handling those dirs safely wrt vm isolation. -- Jamie Strandboge | http://www.canonical.com

On Mon, Aug 13, 2018 at 7:11 PM Jamie Strandboge <jamie@canonical.com> wrote:

On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:

...

Libvirt now tries to preserve all mounts under /dev in qemu namespaces. The old rules only listed a set of known paths but those are no more enough.

I found some due to containers like /dev/.lxc/* and such but also /dev/console and /dev/net/tun.

Libvirt is correct to do so, but we can no more predict the names properly, so we modify the rule to allow a wildcard based pattern matching what libvirt does.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- examples/apparmor/usr.sbin.libvirtd | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 3ff43c32a2..b2e38fe0ad 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -33,17 +33,11 @@ mount options=(rw,rslave) -> /, mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,

- mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/, - mount options=(rw, move) /dev/hugepages/ -> /{var/,}run/libvirt/qemu/*.hugepages/, - mount options=(rw, move) /dev/mqueue/ -> /{var/,}run/libvirt/qemu/*.mqueue/, - mount options=(rw, move) /dev/pts/ -> /{var/,}run/libvirt/qemu/*.pts/, - mount options=(rw, move) /dev/shm/ -> /{var/,}run/libvirt/qemu/*.shm/, - - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ -> /dev/hugepages/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/ -> /dev/mqueue/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ -> /dev/pts/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ -> /dev/shm/, + # libvirt provides any mounts under /dev to qemu namespaces + mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/, + mount options=(rw, move) /dev/**{/,} -> /{var/,}run/libvirt/qemu/*{/,},

What are you trying to convey with this rule? As written, the '{/,}' is redundant since '**' will match that.

I had issues on the other end, with different paths being accessed with/without trailing slash /{var/,}run/libvirt/qemu/*{/,}, So I added the trailing "with or without slash" part. You are right, on the other end due to the unpredictable path I already had ** which will cover the trailing slash. I can do a V2 without the trailing part on the side that has the "**" already

...

+ mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/, + mount options=(rw, move) /{var/,}run/libvirt/qemu/*{/,} -> /dev/**{/,},

ditto

-- Jamie Strandboge | http://www.canonical.com

-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd