On 1/30/20 7:12 AM, Michal Privoznik wrote:

The configure script allows users to specify different paths for /etc/, /usr/sbin/, /var/run/ and /usr/libexec/. Instead of assuming user will pass expected value, generate the apparmor profiles using the actual values.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/Makefile.inc.am | 29 +++++++++++++++---- ...lper => usr.lib.libvirt.virt-aa-helper.in} | 10 +++---- ...usr.sbin.libvirtd => usr.sbin.libvirtd.in} | 12 ++++---- 3 files changed, 35 insertions(+), 16 deletions(-) rename src/security/apparmor/{usr.lib.libvirt.virt-aa-helper => usr.lib.libvirt.virt-aa-helper.in} (85%) rename src/security/apparmor/{usr.sbin.libvirtd => usr.sbin.libvirtd.in} (94%)

diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am index 6fe9d50f29..3d669275d4 100644 --- a/src/security/Makefile.inc.am +++ b/src/security/Makefile.inc.am @@ -30,16 +30,36 @@ SECURITY_DRIVER_APPARMOR_SOURCES = \ security/security_apparmor.c \ $(NULL)

+SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES_IN = \ + security/apparmor/usr.lib.libvirt.virt-aa-helper.in \ + security/apparmor/usr.sbin.libvirtd.in \ + $(NULL) + +SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES = \ + $(SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES_IN:%.in=%) + $(NULL) + +security/apparmor/%: $(srcdir)/security/apparmor/%.in + $(AM_V_GEN)$(MKDIR_P) `dirname $@` && \ + $(SED) \ + -e 's|[@]sysconfdir[@]|@sysconfdir@|' \ + -e 's|[@]sbindir[@]|@sbindir@|' \ + -e 's|[@]runstatedir[@]|@runstatedir@|' \ + -e 's|[@]libexecdir[@]|@libexecdir@|' \ + $< > $@ + +BUILT_SOURCES += $(SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES) +CLEANFILES += $(SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES) + EXTRA_DIST += \ $(SECURITY_DRIVER_SELINUX_SOURCES) \ $(SECURITY_DRIVER_APPARMOR_SOURCES) \ $(SECURITY_DRIVER_APPARMOR_HELPER_SOURCES) \ + $(SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES_IN) \ security/apparmor/TEMPLATE.qemu \ security/apparmor/TEMPLATE.lxc \ security/apparmor/libvirt-qemu \ security/apparmor/libvirt-lxc \ - security/apparmor/usr.lib.libvirt.virt-aa-helper \ - security/apparmor/usr.sbin.libvirtd \ $(NULL)

libvirt_security_manager_la_SOURCES = $(SECURITY_DRIVER_SOURCES) @@ -91,8 +111,7 @@ endif WITH_SECDRIVER_APPARMOR if WITH_APPARMOR_PROFILES apparmordir = $(sysconfdir)/apparmor.d/ apparmor_DATA = \ - security/apparmor/usr.lib.libvirt.virt-aa-helper \ - security/apparmor/usr.sbin.libvirtd \ + $(SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES) $(NULL)

abstractionsdir = $(apparmordir)/abstractions @@ -108,7 +127,7 @@ templates_DATA = \ $(NULL)

APPARMOR_LOCAL_DIR = "$(DESTDIR)$(apparmordir)/local" -install-apparmor-local: +install-apparmor-local: $(SECURITY_DRIVER_APPARMOR_GENERATED_PROFILES) $(MKDIR_P) "$(APPARMOR_LOCAL_DIR)" echo "# Site-specific additions and overrides for \ 'usr.lib.libvirt.virt-aa-helper'" \ diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in similarity index 85% rename from src/security/apparmor/usr.lib.libvirt.virt-aa-helper rename to src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index af434ab539..dd18c8ab89 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -1,6 +1,6 @@ #include <tunables/global>

-profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { +profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base>

# needed for searching directories @@ -19,7 +19,7 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { # Used when internally running another command (namely apparmor_parser) @{PROC}/@{pid}/fd/ r,

- /etc/libnl-3/classid r, + @sysconfdir@/libnl-3/classid r,

# for gl enabled graphics /dev/dri/{,*} r, @@ -38,11 +38,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { deny /dev/mapper/ r, deny /dev/mapper/* r,

- /usr/{lib,lib64}/libvirt/virt-aa-helper mr, + @libexecdir@/virt-aa-helper mr, /{usr/,}sbin/apparmor_parser Ux,

- /etc/apparmor.d/libvirt/* r, - /etc/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw, + @sysconfdir@/apparmor.d/libvirt/* r, + @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw,

# for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools diff --git a/src/security/apparmor/usr.sbin.libvirtd b/src/security/apparmor/usr.sbin.libvirtd.in similarity index 94% rename from src/security/apparmor/usr.sbin.libvirtd rename to src/security/apparmor/usr.sbin.libvirtd.in index b21f31b2e1..f4fc51d705 100644 --- a/src/security/apparmor/usr.sbin.libvirtd +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -1,7 +1,7 @@ #include <tunables/global> @{LIBVIRT}="libvirt"

-profile libvirtd /usr/sbin/libvirtd flags=(attach_disconnected) { +profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { #include <abstractions/base> #include <abstractions/dbus>

@@ -80,8 +80,8 @@ profile libvirtd /usr/sbin/libvirtd flags=(attach_disconnected) { /bin/* PUx, /sbin/* PUx, /usr/bin/* PUx, - /usr/sbin/virtlogd pix, - /usr/sbin/* PUx, + @sbindir@/virtlogd pix, + @sbindir@/* PUx, /{usr/,}lib/udev/scsi_id PUx, /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx, /usr/{lib,lib64}/xen/bin/* Ux, @@ -98,9 +98,9 @@ profile libvirtd /usr/sbin/libvirtd flags=(attach_disconnected) { audit deny /sys/kernel/security/apparmor/matching rwxl, audit deny /sys/kernel/security/apparmor/.* rwxl, /sys/kernel/security/apparmor/profiles r, - /usr/{lib,lib64}/libvirt/* PUxr, - /usr/{lib,lib64}/libvirt/libvirt_parthelper ix, - /usr/{lib,lib64}/libvirt/libvirt_iohelper ix, + @libexecdir@/* puxr,

s/puxr/PUxr/ to match the existing access modes. Regards, Jim

+ @libexecdir@/libvirt_parthelper ix, + @libexecdir@/libvirt_iohelper ix, /etc/libvirt/hooks/** rmix, /etc/xen/scripts/** rmix,

On Thu, Jan 30, 2020 at 03:12:31PM +0100, Michal Privoznik wrote:

Both of these binaries are spawn by libvirt. Add a rule to the default profile to allow that.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index f4fc51d705..c950a83db8 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -99,6 +99,8 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { audit deny /sys/kernel/security/apparmor/.* rwxl, /sys/kernel/security/apparmor/profiles r, @libexecdir@/* puxr, + @libexecdir@/virt-aa-helper PUxr,

I'm really puzzelled about this one. If this was required, then surely apparmor has been broken since day 1 this was introduced to libvirt ? Can anyone explain why we've been able todo with this rule forever ?

+ @libexecdir@/libvirt_lxc PUxr,

I can understand a little more why this might be missing, as it is not so common as QEMU usage.

@libexecdir@/libvirt_parthelper ix, @libexecdir@/libvirt_iohelper ix, /etc/libvirt/hooks/** rmix, -- 2.24.1

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 2/3/20 5:50 PM, Daniel P. Berrangé wrote:

On Thu, Jan 30, 2020 at 03:12:31PM +0100, Michal Privoznik wrote:

...

Both of these binaries are spawn by libvirt. Add a rule to the default profile to allow that.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 2 ++ 1 file changed, 2 insertions(+)

...

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index f4fc51d705..c950a83db8 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -99,6 +99,8 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { audit deny /sys/kernel/security/apparmor/.* rwxl, /sys/kernel/security/apparmor/profiles r, @libexecdir@/* puxr, + @libexecdir@/virt-aa-helper PUxr,

I'm really puzzelled about this one. If this was required, then surely apparmor has been broken since day 1 this was introduced to libvirt ?

Can anyone explain why we've been able todo with this rule forever ?

...

+ @libexecdir@/libvirt_lxc PUxr,

I can understand a little more why this might be missing, as it is not so common as QEMU usage.

Frankly, I don't understand that too. I just copied what was in the gentoo patch. But I can drop this one. The @libexecdir@/* rule should allow what is needed anyway. Michal