[PATCH 0/8] Further Debian/Ubuntu Apparmor Delta
Hi, I don't even remember which number of submissions that is #5 maybe? Anyway - I'm hereby continuing to bring Debian and Ubuntu apparmor Delta into upstream libvirt. I have kept out all patches that are either Distro-specific or we ran into trouble/discussions in the past. But there are enough left for a new submission. I have kept the most-original (read the earliest - as some patches appeared in Ubuntu and later with a different Author in Debian) patch author that I could find intact and git-send-email should auto-cc them. I added some more bug links and descriptions so one can understand the case a commit tries to fix without knowing too much context. Christian Ehrhardt (2): apparmor: allow virt-aa-helper nameservices apparmor: let qemu load old shared objects after upgrades Jamie Strandboge (1): apparmor: read only access to overcommit_memory Sam Hartman (1): apparmor: allow default pki path Serge Hallyn (1): apparmor: allow virt-aa-helper to read openvswitch sockets Stefan Bader (3): apparmor: allow libvirtd to call pygrub apparmor: qemu access to @{PROC}/*/auxv for hw_cap apparmor: allow virt-aa-helper to read from tmp src/security/apparmor/libvirt-qemu | 9 +++++++++ src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 +++++ src/security/apparmor/usr.sbin.libvirtd.in | 1 + 3 files changed, 15 insertions(+) -- 2.27.0
From: Sam Hartman <hartmans@debian.org> /etc/pki/qemu is a pki path recommended by qemu tls docs [1] and one that can cause issues with spice connections when missing. Add the path to the allowed list of pki paths to fix the issue. Note: this is active in Debian/Ubuntu [1] for quite a while already. [1]: https://www.qemu.org/docs/master/system/tls.html [2]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=930100 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/libvirt-qemu | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 1a4b226612..2d08d6f7ad 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -94,6 +94,8 @@ /etc/pki/CA/* r, /etc/pki/libvirt{,-spice,-vnc}/ r, /etc/pki/libvirt{,-spice,-vnc}/** r, + /etc/pki/qemu/ r, + /etc/pki/qemu/** r, # the various binaries /usr/bin/kvm rmix, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Sam Hartman <hartmans@debian.org>
/etc/pki/qemu is a pki path recommended by qemu tls docs [1] and one that can cause issues with spice connections when missing.
Add the path to the allowed list of pki paths to fix the issue.
Note: this is active in Debian/Ubuntu [1] for quite a while already.
[1]: https://www.qemu.org/docs/master/system/tls.html [2]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=930100
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/libvirt-qemu | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 1a4b226612..2d08d6f7ad 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -94,6 +94,8 @@ /etc/pki/CA/* r, /etc/pki/libvirt{,-spice,-vnc}/ r, /etc/pki/libvirt{,-spice,-vnc}/** r, + /etc/pki/qemu/ r, + /etc/pki/qemu/** r,
+1 to apply -- Jamie Strandboge | http://www.canonical.com
From: Stefan Bader <stefan.bader@canonical.com> When using xen through libxl in Debian/Ubuntu it needs to be able to call pygrub. This is placed in a versioned path like /usr/lib/xen-4.11/bin. In theory the rule could be more strict by rendering the libexec_dir setting pkg-config can derive from libbxen-dev. But that would make particular libvirt/xen packages version-depend on each other. It seems more reasonable to avoid these versioned dependencies and use a wildcard rule instead as it is already in place for libxl-save-helper. Note: This change was in Debian [1] and Ubuntu [2] for quite some time already. [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931768 [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1326003 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index 1e137039e9..312fa4b6d1 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx, /usr/{lib,lib64}/xen/bin/* Ux, /usr/lib/xen-*/bin/libxl-save-helper PUx, + /usr/lib/xen-*/bin/pygrub PUx, /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx, # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Stefan Bader <stefan.bader@canonical.com>
When using xen through libxl in Debian/Ubuntu it needs to be able to call pygrub.
This is placed in a versioned path like /usr/lib/xen-4.11/bin. In theory the rule could be more strict by rendering the libexec_dir setting pkg-config can derive from libbxen-dev. But that would make particular libvirt/xen packages version-depend on each other. It seems more reasonable to avoid these versioned dependencies and use a wildcard rule instead as it is already in place for libxl-save-helper.
Note: This change was in Debian [1] and Ubuntu [2] for quite some time already.
[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931768 [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1326003
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index 1e137039e9..312fa4b6d1 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx, /usr/{lib,lib64}/xen/bin/* Ux, /usr/lib/xen-*/bin/libxl-save-helper PUx, + /usr/lib/xen-*/bin/pygrub PUx,
LGTM. +1 to apply -- Jamie Strandboge | http://www.canonical.com
Since quite a while libvirt-aa-helper triggers nss related apparmor denials like: operation="open" profile="virt-aa-helper" name="/etc/nsswitch.conf" operation="open" profile="virt-aa-helper" name="/etc/host.conf" operation="open" profile="virt-aa-helper" name="/etc/resolv.conf" operation="open" profile="virt-aa-helper" name="/etc/hosts" Rules to allow these are in Debian [1] / Ubuntu [2] since quite a while but do not seem to be specific to those distributions. There can be much more reasons than one would think to inadvertently use/trigger nameservices as can be seen in the comments in profiles/apparmor.d/abstractions/nameservice at [3]. The nameservices abstraction provides a nice and upgrade safe way to cover all of them. [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979 [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1546674 [3]: https://gitlab.com/apparmor/apparmor Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index dd18c8ab89..dfc61e8de4 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -2,6 +2,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> + #include <abstractions/nameservice> # needed for searching directories capability dac_override, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
Since quite a while libvirt-aa-helper triggers nss related apparmor denials like: operation="open" profile="virt-aa-helper" name="/etc/nsswitch.conf" operation="open" profile="virt-aa-helper" name="/etc/host.conf" operation="open" profile="virt-aa-helper" name="/etc/resolv.conf" operation="open" profile="virt-aa-helper" name="/etc/hosts"
Rules to allow these are in Debian [1] / Ubuntu [2] since quite a while but do not seem to be specific to those distributions.
There can be much more reasons than one would think to inadvertently use/trigger nameservices as can be seen in the comments in profiles/apparmor.d/abstractions/nameservice at [3]. The nameservices abstraction provides a nice and upgrade safe way to cover all of them.
[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979 [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1546674 [3]: https://gitlab.com/apparmor/apparmor
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index dd18c8ab89..dfc61e8de4 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -2,6 +2,7 @@
profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> + #include <abstractions/nameservice>
nameservice brings in network rules so this is actually a lot of access. Why is it reaching out to nss? Is it just cause some library happens to look at /etc/nsswitch.conf and pull in other things or does it actually need networking? I suspect the former. If my suspicion is true, perhaps instead: # virt-aa-helper dependent libraries read (and if successful, other # files) but virt-aa-helper itself doesn't require the access, so # silence the denial. deny /etc/nsswitch.conf r, -- Jamie Strandboge | http://www.canonical.com
On Mon, Aug 3, 2020 at 5:05 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
Since quite a while libvirt-aa-helper triggers nss related apparmor denials like: operation="open" profile="virt-aa-helper" name="/etc/nsswitch.conf" operation="open" profile="virt-aa-helper" name="/etc/host.conf" operation="open" profile="virt-aa-helper" name="/etc/resolv.conf" operation="open" profile="virt-aa-helper" name="/etc/hosts"
Rules to allow these are in Debian [1] / Ubuntu [2] since quite a while but do not seem to be specific to those distributions.
There can be much more reasons than one would think to inadvertently use/trigger nameservices as can be seen in the comments in profiles/apparmor.d/abstractions/nameservice at [3]. The nameservices abstraction provides a nice and upgrade safe way to cover all of them.
That is possible, initially we only had rules that looked like the head of /etc/apparmor.d/abstractions/nameservice which is like /etc/{group,gai.conf,nssswitch.conf,hosts} But you are right, chances are that is reads that by accident. Back then in bug 1513367 the discussion was a bit complex as it was about three different apparmor denials at once. And in the Debian counterpart of https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979 they headed to "nss should be save" right away. All of the old reports called the denials either noise or a slowdown - so looking back there didn't seem to be a functional impact. Which would be a +1 to your theory. I was removing the rule, but just as when this bug came up at first it isn't triggering for various test environments and cases that I tried.
[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979
[2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1546674 [3]: https://gitlab.com/apparmor/apparmor
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index dd18c8ab89..dfc61e8de4 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -2,6 +2,7 @@
profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> + #include <abstractions/nameservice>
nameservice brings in network rules so this is actually a lot of access. Why is it reaching out to nss? Is it just cause some library happens to look at /etc/nsswitch.conf and pull in other things or does it actually need networking? I suspect the former. If my suspicion is true, perhaps instead:
# virt-aa-helper dependent libraries read (and if successful, other # files) but virt-aa-helper itself doesn't require the access, so # silence the denial. deny /etc/nsswitch.conf r,
I like your suggestion, especially as this isn't the profile for libvirtd but just virt-aa-helper. That denial seems non critical while most likely prevents the follow up access. I was looking for nss libs in virt-aa-helper context that "might be required" but the only path to it seems to come up in regard to rbd support which isn't needed to render apparmor rules in virt-aa-helper. I'd really want to make it part of a v2 submission, but feel that it is wrong to do so without being able to re-create it to know for sure where the access was from exactly. I was trying back to 16.04 (being the time this was reported) with local disks (that is what triggered it back then) or ceph (as rbd is the only path to libnss3) :-/ For now I'm just removing this commit from the list of proposed changes. --
Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
From: Jamie Strandboge <jamie@ubuntu.com> Allow qemu to read @{PROC}/sys/vm/overcommit_memory. This is read on guest start-up and (as read-only) not a critical secret that has to stay hidden. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Jamie Strandboge <jamie@ubuntu.com> --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 2d08d6f7ad..b132cf0226 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -32,6 +32,7 @@ # only modify its comm value or those in its thread group. owner @{PROC}/@{pid}/task/@{tid}/comm rw, @{PROC}/sys/kernel/cap_last_cap r, + @{PROC}/sys/vm/overcommit_memory r, # For hostdev access. The actual devices will be added dynamically /sys/bus/usb/devices/ r, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Jamie Strandboge <jamie@ubuntu.com>
Allow qemu to read @{PROC}/sys/vm/overcommit_memory. This is read on guest start-up and (as read-only) not a critical secret that has to stay hidden.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Jamie Strandboge <jamie@ubuntu.com> --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 2d08d6f7ad..b132cf0226 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -32,6 +32,7 @@ # only modify its comm value or those in its thread group. owner @{PROC}/@{pid}/task/@{tid}/comm rw, @{PROC}/sys/kernel/cap_last_cap r, + @{PROC}/sys/vm/overcommit_memory r,
+1 to apply -- Jamie Strandboge | http://www.canonical.com
From: Stefan Bader <stefan.bader@canonical.com> On some architectures (ppc, s390x, sparc, arm) qemu will read auxv to detect hardware capabilities via qemu_getauxval. Allow that access read-only for the entry owned by the current qemu process. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index b132cf0226..25eff20b82 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -33,6 +33,7 @@ owner @{PROC}/@{pid}/task/@{tid}/comm rw, @{PROC}/sys/kernel/cap_last_cap r, @{PROC}/sys/vm/overcommit_memory r, + owner @{PROC}/*/auxv r, # For hostdev access. The actual devices will be added dynamically /sys/bus/usb/devices/ r, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Stefan Bader <stefan.bader@canonical.com>
On some architectures (ppc, s390x, sparc, arm) qemu will read auxv to detect hardware capabilities via qemu_getauxval.
Allow that access read-only for the entry owned by the current qemu process.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index b132cf0226..25eff20b82 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -33,6 +33,7 @@ owner @{PROC}/@{pid}/task/@{tid}/comm rw, @{PROC}/sys/kernel/cap_last_cap r, @{PROC}/sys/vm/overcommit_memory r, + owner @{PROC}/*/auxv r,
+1 to apply. A code comment that is simply the first sentence of Stefan's commit message might be a nice touch, but that is not a blocker. -- Jamie Strandboge | http://www.canonical.com
On Mon, Aug 3, 2020 at 5:07 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Stefan Bader <stefan.bader@canonical.com>
On some architectures (ppc, s390x, sparc, arm) qemu will read auxv to detect hardware capabilities via qemu_getauxval.
Allow that access read-only for the entry owned by the current qemu process.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index b132cf0226..25eff20b82 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -33,6 +33,7 @@ owner @{PROC}/@{pid}/task/@{tid}/comm rw, @{PROC}/sys/kernel/cap_last_cap r, @{PROC}/sys/vm/overcommit_memory r, + owner @{PROC}/*/auxv r,
+1 to apply. A code comment that is simply the first sentence of Stefan's commit message might be a nice touch, but that is not a blocker.
Yeah I added that comment when researching the reason - added a comment to the commit, but not worth a v2 submission. Tanks for reading through all of these changes!
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
From: Stefan Bader <stefan.bader@canonical.com> temporary directories are a common place images are placed by users for any sort of quick evaluation. Allow virt-aa-helper access to tmp via the existing user-tmp apparmor abstraction. That way if a guest definition has paths in temporary directories virt-aa-helper can properly probe them e.g. for further backing files in the case of qcow2. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index dfc61e8de4..3f204799a6 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -3,6 +3,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> #include <abstractions/nameservice> + #include <abstractions/user-tmp> # needed for searching directories capability dac_override, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Stefan Bader <stefan.bader@canonical.com>
temporary directories are a common place images are placed by users for any sort of quick evaluation. Allow virt-aa-helper access to tmp via the existing user-tmp apparmor abstraction.
That way if a guest definition has paths in temporary directories virt-aa-helper can properly probe them e.g. for further backing files in the case of qcow2.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index dfc61e8de4..3f204799a6 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -3,6 +3,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> #include <abstractions/nameservice> + #include <abstractions/user-tmp>
user-tmp allows write and all other accesses for disks are read. We have these rules: /**.img r, /**.raw r, /**.qcow{,2} r, /**.qed r, /**.vmdk r, /**.vhd r, /**.[iI][sS][oO] r, /**/disk{,.*} r, Why are these not sufficient? What was the denial that triggered the issue? -- Jamie Strandboge | http://www.canonical.com
On Mon, Aug 3, 2020 at 5:11 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Stefan Bader <stefan.bader@canonical.com>
temporary directories are a common place images are placed by users for any sort of quick evaluation. Allow virt-aa-helper access to tmp via the existing user-tmp apparmor abstraction.
That way if a guest definition has paths in temporary directories virt-aa-helper can properly probe them e.g. for further backing files in the case of qcow2.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index dfc61e8de4..3f204799a6 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -3,6 +3,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> #include <abstractions/nameservice> + #include <abstractions/user-tmp>
user-tmp allows write and all other accesses for disks are read. We have these rules:
/**.img r, /**.raw r, /**.qcow{,2} r, /**.qed r, /**.vmdk r, /**.vhd r, /**.[iI][sS][oO] r, /**/disk{,.*} r,
Why are these not sufficient? What was the denial that triggered the issue?
Great question to ask - this is one of the Deltas that was "just carried" for quite some time. But you made me analyze the background and it isn't reasonable IMHO. It was added quite some time ago without outlining a particular reason. The list you refer to above wasn't as long back then (~5 years ago), maybe extending the list would have been all that was needed and instead the user-tmp abstraction was added. I'll drop the commit from this series as well as from Ubuntu on next merge and check for any fallout - it is easy to be added back and most likely not needed. --
Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
From: Serge Hallyn <serge.hallyn@ubuntu.com> Chardevs/sockets configured for openvswitch-dpdk use cases might be probed by virt-aa-helper. Allow that access to enable virt-aa-helper rendering per-guest rules for the actual qemu guest accessing these sockets eventually. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index 3f204799a6..877cb04b1e 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -46,6 +46,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @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 openvswitch sockets + /{,var/}run/openvswitch/** rw, + # for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools audit deny @{HOME}/.* mrwkl, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Chardevs/sockets configured for openvswitch-dpdk use cases might be probed by virt-aa-helper. Allow that access to enable virt-aa-helper rendering per-guest rules for the actual qemu guest accessing these sockets eventually.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index 3f204799a6..877cb04b1e 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -46,6 +46,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @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 openvswitch sockets + /{,var/}run/openvswitch/** rw,
A bit unfortunate and unexpected. What kind of probing does virt-aa-helper do on these? -- Jamie Strandboge | http://www.canonical.com
On Mon, Aug 3, 2020 at 5:13 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Chardevs/sockets configured for openvswitch-dpdk use cases might be probed by virt-aa-helper. Allow that access to enable virt-aa-helper rendering per-guest rules for the actual qemu guest accessing these sockets eventually.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index 3f204799a6..877cb04b1e 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -46,6 +46,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @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 openvswitch sockets + /{,var/}run/openvswitch/** rw,
A bit unfortunate and unexpected. What kind of probing does virt-aa-helper do on these?
I'm so glad we do this exercise and you have the "investigative hat on" to challenge the few bits of the series that seem odd. I have read through virt-aa-helper again with a focus on this and at least today's openvswitch-dpdk+libvirt should not need this anymore. It seems this was a wild guess many years ago and added for bug 1513367 but eventually (or just noadays) is no longer needed. I have set up a 20.04 based openvswitch-dpdk system and dropped the rule. Once with vhostuserclient and once on an older system with the older vhostuser type connection. Things are still working, so I'm removing this rule from this series as well as from the Ubuntu builds.
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
Since [1] qemu can after upgrade fall back to pre-upgrade modules to still be able to dynamically load qmeu-module based features. The paths for these modules are pre-defined by the code and should be allowed to be mapped and loaded from which will allow packagers avoiding the inability of late feature load [2] after package upgrades. [1]: https://github.com/qemu/qemu/commit/bd83c861 [2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/libvirt-qemu | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 25eff20b82..c6f7149799 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -168,6 +168,11 @@ /usr/{lib,lib64}/qemu/*.so mr, /usr/lib/@{multiarch}/qemu/*.so mr, + # let qemu load old shared objects after upgrades (LP: #1847361) + /{var/,}run/qemu/*/*.so mr, + # but explicitly deny writing to these files + audit deny /{var/,}run/qemu/*/*.so w, + # swtpm /{usr/,}bin/swtpm rmix, /usr/{lib,lib64}/libswtpm_libtpms.so mr, -- 2.27.0
On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
Since [1] qemu can after upgrade fall back to pre-upgrade modules to still be able to dynamically load qmeu-module based features.
The paths for these modules are pre-defined by the code and should be allowed to be mapped and loaded from which will allow packagers avoiding the inability of late feature load [2] after package upgrades.
[1]: https://github.com/qemu/qemu/commit/bd83c861 [2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/libvirt-qemu | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu index 25eff20b82..c6f7149799 100644 --- a/src/security/apparmor/libvirt-qemu +++ b/src/security/apparmor/libvirt-qemu @@ -168,6 +168,11 @@ /usr/{lib,lib64}/qemu/*.so mr, /usr/lib/@{multiarch}/qemu/*.so mr,
+ # let qemu load old shared objects after upgrades (LP: #1847361) + /{var/,}run/qemu/*/*.so mr, + # but explicitly deny writing to these files + audit deny /{var/,}run/qemu/*/*.so w, + +1 to apply
-- Jamie Strandboge | http://www.canonical.com
participants (2)
-
Christian Ehrhardt -
Jamie Strandboge