Re: [PATCH V2 1/4] Apparmor: Add profile for virtqemud

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 6/23/21 11:43 PM, Christian Ehrhardt wrote:
> On Wed, Jun 23, 2021 at 1:27 AM Jim Fehlig <jfehlig@xxxxxxxx> wrote:
> > A new apparmor profile derived from the libvirtd profile, with non-QEMU
> > related rules removed. Adopt the libvirt-qemu abstraction to work with
> > the new profile.
> >
> > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
>
> Thanks for your work on this, but since in the split daemon mode
> virtqemud will do the
> majority of the tasks I wonder if along this change (or later) we
> should consider
> removing rules from the libvirtd profile.

AFAIK (at least in theory), the modular and monolithic daemons are mutually 
exclusive. Either you run the desired modular daemon(s) or the monolithic 
libvirtd. So the libvirtd rules need to stay IMO.

And IIRC, Daniel has long-term plans to remove the monolithic daemon, at which 
point the libvirtd profile can be dropped too.

> It should now have less tasks and therefore need less permissions.
> Have you had the chance to take a look into that already?
>
> There is a bonus-problem though, as long as we provide the option to build
> non-split daemons we would effectively need two profiles.
> One for the monolithic libvirtd and a reduced one for the split kind.

Agreed. We'll need both as long as we have the modular and monolithic daemons.

> This shall not stall this patch series, it is just a related thought
> that I wanted
> to bring to your attention.
>
> > ---
> >   src/security/apparmor/libvirt-qemu          |   3 +
> >   src/security/apparmor/meson.build           |   1 +
> >   src/security/apparmor/usr.sbin.virtqemud.in | 135 ++++++++++++++++++++
> >   3 files changed, 139 insertions(+)
>
>
> On the patch itself while I had no chance to test it myself it looks
> exactly as I'd have expected a virtqemud profile.
> Reviewed-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>

Thanks! I did notice another problem while doing more testing yesterday. See below.

> > diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
> > index 85c9e61d6c..3e31ed4981 100644
> > --- a/src/security/apparmor/libvirt-qemu
> > +++ b/src/security/apparmor/libvirt-qemu
> > @@ -16,9 +16,11 @@
> >
> >     ptrace (readby, tracedby) peer=libvirtd,
> >     ptrace (readby, tracedby) peer=/usr/sbin/libvirtd,
> > +  ptrace (readby, tracedby) peer=virtqemud,
> >
> >     signal (receive) peer=libvirtd,
> >     signal (receive) peer=/usr/sbin/libvirtd,
> > +  signal (receive) peer=virtqemud,
> >
> >     /dev/kvm rw,
> >     /dev/net/tun rw,
> > @@ -221,6 +223,7 @@
> >     # allow connect with openGraphicsFD to work
> >     unix (send, receive) type=stream addr=none peer=(label=libvirtd),
> >     unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd),
> > +  unix (send, receive) type=stream addr=none peer=(label=virtqemud),
> >
> >     # for gathering information about available host resources
> >     /sys/devices/system/cpu/ r,
> > diff --git a/src/security/apparmor/meson.build b/src/security/apparmor/meson.build
> > index af43780211..56f308bf3a 100644
> > --- a/src/security/apparmor/meson.build
> > +++ b/src/security/apparmor/meson.build
> > @@ -1,6 +1,7 @@
> >   apparmor_gen_profiles = [
> >     'usr.lib.libvirt.virt-aa-helper',
> >     'usr.sbin.libvirtd',
> > +  'usr.sbin.virtqemud',
> >   ]
> >
> >   apparmor_gen_profiles_conf = configuration_data()
> > diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in
> > new file mode 100644
> > index 0000000000..b986241c74
> > --- /dev/null
> > +++ b/src/security/apparmor/usr.sbin.virtqemud.in
> > @@ -0,0 +1,135 @@
> > +#include <tunables/global>
> > +@{LIBVIRT}="libvirt"
> > +
> > +profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
> > +  #include <abstractions/base>
> > +  #include <abstractions/dbus>
> > +
> > +  capability kill,
> > +  capability net_admin,
> > +  capability net_raw,
> > +  capability setgid,
> > +  capability sys_admin,
> > +  capability sys_module,
> > +  capability sys_ptrace,
> > +  capability sys_pacct,
> > +  capability sys_nice,
> > +  capability sys_chroot,
> > +  capability setuid,
> > +  capability dac_override,
> > +  capability dac_read_search,
> > +  capability fowner,
> > +  capability chown,
> > +  capability setpcap,
> > +  capability mknod,
> > +  capability fsetid,
> > +  capability audit_write,
> > +  capability ipc_lock,
> > +  capability sys_rawio,
> > +  capability bpf,
> > +  capability perfmon,
> > +
> > +  # Needed for vfio
> > +  capability sys_resource,
> > +
> > +  mount options=(rw,rslave)  -> /,
> > +  mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,
> > +  umount /{var/,}run/libvirt/qemu/*.dev/,
> > +
> > +  # 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/*{,/},
> > +  mount options=(rw, move) /{,var/}run/libvirt/qemu/*.dev/ -> /dev/,
> > +  mount options=(rw, move) /{,var/}run/libvirt/qemu/*{,/} -> /dev/**,
> > +
> > +  network inet stream,
> > +  network inet dgram,
> > +  network inet6 stream,
> > +  network inet6 dgram,
> > +  network netlink raw,
> > +  network packet dgram,
> > +  network packet raw,
> > +
> > +  # for --p2p migrations
> > +  unix (send, receive) type=stream addr=none peer=(label=unconfined addr=none),
> > +
> > +  ptrace (read,trace) peer=unconfined,
> > +  ptrace (read,trace) peer=@{profile_name},
> > +  ptrace (read,trace) peer=dnsmasq,
> > +  ptrace (read,trace) peer=/usr/sbin/dnsmasq,
> > +  ptrace (read,trace) peer=libvirt-*,
> > +
> > +  signal (send) peer=dnsmasq,
> > +  signal (send) peer=/usr/sbin/dnsmasq,
> > +  signal (read, send) peer=libvirt-*,
> > +  signal (send) set=("kill", "term") peer=unconfined,
> > +
> > +  # For communication/control to qemu-bridge-helper
> > +  unix (send, receive) type=stream addr=none peer=(label=libvirtd//qemu_bridge_helper),
> > +  signal (send) set=("term") peer=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),
> > +
> > +  # required if guests run unconfined seclabel type='none' but libvirtd is confined
> > +  signal (read, send) peer=unconfined,
> > +
> > +  # Very lenient profile for libvirtd since we want to first focus on confining
> > +  # the guests. Guests will have a very restricted profile.
> > +  / r,
> > +  /** rwmkl,
> > +
> > +  /bin/* PUx,
> > +  /sbin/* PUx,
> > +  /usr/bin/* PUx,
> > +  @sbindir@/virtlogd pix,
> > +  @sbindir@/* PUx,
> > +  /{usr/,}lib/udev/scsi_id PUx,
> > +  /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> > +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
> > +
> > +  # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> > +  # read and run an ebtables script.
> > +  /var/lib/libvirt/virtd* ixr,
> > +
> > +  # force the use of virt-aa-helper
> > +  audit deny /{usr/,}sbin/apparmor_parser rwxl,
> > +  audit deny /etc/apparmor.d/libvirt/** wxl,
> > +  audit deny /sys/kernel/security/apparmor/features rwxl,
> > +  audit deny /sys/kernel/security/apparmor/matching rwxl,
> > +  audit deny /sys/kernel/security/apparmor/.* rwxl,
> > +  /sys/kernel/security/apparmor/profiles r,
> > +  @libexecdir@/* PUxr,
> > +  @libexecdir@/libvirt_parthelper ix,
> > +  @libexecdir@/libvirt_iohelper ix,
> > +  /etc/libvirt/hooks/** rmix,
> > +
> > +  # allow changing to our UUID-based named profiles
> > +  change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
> > +
> > +  /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper Cx -> qemu_bridge_helper,
> > +  # child profile for bridge helper process
> > +  profile qemu_bridge_helper {
> > +   #include <abstractions/base>
> > +
> > +   capability setuid,
> > +   capability setgid,
> > +   capability setpcap,
> > +   capability net_admin,
> > +
> > +   network inet stream,
> > +
> > +   # For communication/control from libvirtd
> > +   unix (send, receive) type=stream addr=none peer=(label=libvirtd),

s/libvirtd/virtqemud/

> > +   signal (receive) set=("term") peer=/usr/sbin/libvirtd,

Not needed.

> > +   signal (receive) set=("term") peer=libvirtd,

s/libvirtd/virtqemud/

I'll include these changes in V3.

Regards,
Jim

> > +
> > +   /dev/net/tun rw,
> > +   /etc/qemu/** r,
> > +   owner @{PROC}/*/status r,
> > +
> > +   /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
> > +  }
> > +}
> > --
> > 2.31.1
>
>
> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd