[libvirt] PATCH 3/4: AppArmor updates
Attached is 0003-apparmor-examples.patch -- Jamie Strandboge | http://www.canonical.com
On Fri, Aug 13, 2010 at 05:00:06PM -0500, Jamie Strandboge wrote:
Attached is 0003-apparmor-examples.patch
Can you include full commit messages with each patch, since it makes it easier to review & understand, and will be needed when the patches are applied to GIT.
diff -Naurp libvirt.orig/examples/apparmor/libvirt-qemu libvirt/examples/apparmor/libvirt-qemu --- libvirt.orig/examples/apparmor/libvirt-qemu 2010-04-06 16:14:52.000000000 -0500 +++ libvirt/examples/apparmor/libvirt-qemu 2010-08-13 16:46:34.000000000 -0500 @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Aug 13 16:38:32 2010
#include <abstractions/base> #include <abstractions/consoles> @@ -9,6 +9,10 @@ capability dac_read_search, capability chown,
+ # needed to drop privileges + capability setgid, + capability setuid, + network inet stream, network inet6 stream,
diff -Naurp libvirt.orig/examples/apparmor/usr.lib.libvirt.virt-aa-helper libvirt/examples/apparmor/usr.lib.libvirt.virt-aa-helper --- libvirt.orig/examples/apparmor/usr.lib.libvirt.virt-aa-helper 2010-04-06 16:14:52.000000000 -0500 +++ libvirt/examples/apparmor/usr.lib.libvirt.virt-aa-helper 2010-08-13 16:44:01.000000000 -0500 @@ -1,8 +1,9 @@ -# Last Modified: Mon Apr 5 15:10:27 2010 +# Last Modified: Fri Aug 13 16:38:32 2010 #include <tunables/global>
/usr/lib/libvirt/virt-aa-helper { #include <abstractions/base> + #include <abstractions/user-tmp>
# needed for searching directories capability dac_override, @@ -12,11 +13,16 @@ network inet,
deny @{PROC}/[0-9]*/mounts r, + @{PROC}/[0-9]*/net/psched r, @{PROC}/filesystems r,
# for hostdev /sys/devices/ r, /sys/devices/** r, + /sys/bus/usb/devices/ r, + deny /dev/sd* r, + deny /dev/mapper/ r, + deny /dev/mapper/* r,
/usr/lib/libvirt/virt-aa-helper mr, /sbin/apparmor_parser Ux, @@ -24,8 +30,11 @@ /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,
- # for backingstore -- allow access to non-hidden files in @{HOME} as well - # as storage pools + # For backingstore, virt-aa-helper may need to peek inside the disk image, so + # allow access to non-hidden files in @{HOME} as well as storage pools, and + # removable media and filesystems, and certain file extentions. A + # virt-aa-helper failure when checking a disk for backinsgstore is non-fatal + # (but obviously the backingstore won't be added). audit deny @{HOME}/.* mrwkl, audit deny @{HOME}/.*/ rw, audit deny @{HOME}/.*/** mrwkl,
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, 2010-08-16 at 17:15 +0100, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 05:00:06PM -0500, Jamie Strandboge wrote:
Attached is 0003-apparmor-examples.patch
Can you include full commit messages with each patch, since it makes it easier to review & understand, and will be needed when the patches are applied to GIT.
Certainly, and I apologize. Attached is an updated patch with messages. -- Jamie Strandboge | http://www.canonical.com
On Mon, Aug 16, 2010 at 02:45:02PM -0500, Jamie Strandboge wrote:
On Mon, 2010-08-16 at 17:15 +0100, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 05:00:06PM -0500, Jamie Strandboge wrote:
Attached is 0003-apparmor-examples.patch
Can you include full commit messages with each patch, since it makes it easier to review & understand, and will be needed when the patches are applied to GIT.
Certainly, and I apologize. Attached is an updated patch with messages.
-- Jamie Strandboge | http://www.canonical.com
Author: Jamie Strandboge <jamie@canonical.com> Description: AppArmor example profile adjustments: - libvirt-qemu: allow guests setgid and setuid so qemu can drop privileges - virt-aa-helper: + allow access to @{PROC}/[0-9]*/net/psched + allow searching /sys/bus/usb/devices/ + deny access to /dev to suppress confusing, non-fatal profile denials + allow access to user-tmp abstraction Bug-Ubuntu: LP: #579584, LP: #565691
diff -Naurp libvirt.orig/examples/apparmor/libvirt-qemu libvirt/examples/apparmor/libvirt-qemu --- libvirt.orig/examples/apparmor/libvirt-qemu 2010-04-06 16:14:52.000000000 -0500 +++ libvirt/examples/apparmor/libvirt-qemu 2010-08-13 16:46:34.000000000 -0500 @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Aug 13 16:38:32 2010
#include <abstractions/base> #include <abstractions/consoles> @@ -9,6 +9,10 @@ capability dac_read_search, capability chown,
+ # needed to drop privileges + capability setgid, + capability setuid, + network inet stream, network inet6 stream,
Does QEMU really need this ? The libvirt QEMU driver will drop privileges from root:root to qemu:qemu after forking, but before the /usr/bin/qemu binary is actually exec'd. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 2010-09-23 at 16:10 +0100, Daniel P. Berrange wrote:
On Mon, Aug 16, 2010 at 02:45:02PM -0500, Jamie Strandboge wrote:
Author: Jamie Strandboge <jamie@canonical.com> Description: AppArmor example profile adjustments: - libvirt-qemu: allow guests setgid and setuid so qemu can drop privileges - virt-aa-helper: + allow access to @{PROC}/[0-9]*/net/psched + allow searching /sys/bus/usb/devices/ + deny access to /dev to suppress confusing, non-fatal profile denials + allow access to user-tmp abstraction Bug-Ubuntu: LP: #579584, LP: #565691
diff -Naurp libvirt.orig/examples/apparmor/libvirt-qemu libvirt/examples/apparmor/libvirt-qemu --- libvirt.orig/examples/apparmor/libvirt-qemu 2010-04-06 16:14:52.000000000 -0500 +++ libvirt/examples/apparmor/libvirt-qemu 2010-08-13 16:46:34.000000000 -0500 @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Aug 13 16:38:32 2010
#include <abstractions/base> #include <abstractions/consoles> @@ -9,6 +9,10 @@ capability dac_read_search, capability chown,
+ # needed to drop privileges + capability setgid, + capability setuid, + network inet stream, network inet6 stream,
Does QEMU really need this ? The libvirt QEMU driver will drop privileges from root:root to qemu:qemu after forking, but before the /usr/bin/qemu binary is actually exec'd.
Yes. Users were seeing errors like: libvir: QEMU error : cannot change to '109' group: Operation not permitted libvir: QEMU error : cannot change to '104' user: Operation not permitted For details, see: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/579584 -- Jamie Strandboge | http://www.canonical.com
On Thu, Sep 23, 2010 at 11:49:21AM -0500, Jamie Strandboge wrote:
On Thu, 2010-09-23 at 16:10 +0100, Daniel P. Berrange wrote:
On Mon, Aug 16, 2010 at 02:45:02PM -0500, Jamie Strandboge wrote:
Author: Jamie Strandboge <jamie@canonical.com> Description: AppArmor example profile adjustments: - libvirt-qemu: allow guests setgid and setuid so qemu can drop privileges - virt-aa-helper: + allow access to @{PROC}/[0-9]*/net/psched + allow searching /sys/bus/usb/devices/ + deny access to /dev to suppress confusing, non-fatal profile denials + allow access to user-tmp abstraction Bug-Ubuntu: LP: #579584, LP: #565691
diff -Naurp libvirt.orig/examples/apparmor/libvirt-qemu libvirt/examples/apparmor/libvirt-qemu --- libvirt.orig/examples/apparmor/libvirt-qemu 2010-04-06 16:14:52.000000000 -0500 +++ libvirt/examples/apparmor/libvirt-qemu 2010-08-13 16:46:34.000000000 -0500 @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Aug 13 16:38:32 2010
#include <abstractions/base> #include <abstractions/consoles> @@ -9,6 +9,10 @@ capability dac_read_search, capability chown,
+ # needed to drop privileges + capability setgid, + capability setuid, + network inet stream, network inet6 stream,
Does QEMU really need this ? The libvirt QEMU driver will drop privileges from root:root to qemu:qemu after forking, but before the /usr/bin/qemu binary is actually exec'd.
Yes. Users were seeing errors like: libvir: QEMU error : cannot change to '109' group: Operation not permitted libvir: QEMU error : cannot change to '104' user: Operation not permitted
Hmm, that's a libvirt error rather than a QEMU error. Is the restricted AppArmour policy taking effect *before* the actual QEMU binary is exec()d ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 2010-09-23 at 18:03 +0100, Daniel P. Berrange wrote:
On Thu, Sep 23, 2010 at 11:49:21AM -0500, Jamie Strandboge wrote:
On Thu, 2010-09-23 at 16:10 +0100, Daniel P. Berrange wrote:
On Mon, Aug 16, 2010 at 02:45:02PM -0500, Jamie Strandboge wrote:
Author: Jamie Strandboge <jamie@canonical.com> Description: AppArmor example profile adjustments: - libvirt-qemu: allow guests setgid and setuid so qemu can drop privileges - virt-aa-helper: + allow access to @{PROC}/[0-9]*/net/psched + allow searching /sys/bus/usb/devices/ + deny access to /dev to suppress confusing, non-fatal profile denials + allow access to user-tmp abstraction Bug-Ubuntu: LP: #579584, LP: #565691
diff -Naurp libvirt.orig/examples/apparmor/libvirt-qemu libvirt/examples/apparmor/libvirt-qemu --- libvirt.orig/examples/apparmor/libvirt-qemu 2010-04-06 16:14:52.000000000 -0500 +++ libvirt/examples/apparmor/libvirt-qemu 2010-08-13 16:46:34.000000000 -0500 @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Aug 13 16:38:32 2010
#include <abstractions/base> #include <abstractions/consoles> @@ -9,6 +9,10 @@ capability dac_read_search, capability chown,
+ # needed to drop privileges + capability setgid, + capability setuid, + network inet stream, network inet6 stream,
Does QEMU really need this ? The libvirt QEMU driver will drop privileges from root:root to qemu:qemu after forking, but before the /usr/bin/qemu binary is actually exec'd.
Yes. Users were seeing errors like: libvir: QEMU error : cannot change to '109' group: Operation not permitted libvir: QEMU error : cannot change to '104' user: Operation not permitted
Hmm, that's a libvirt error rather than a QEMU error. Is the restricted AppArmour policy taking effect *before* the actual QEMU binary is exec()d ?
This is related to the stacked security driver implementation. Specifically, if I strace libvirtd, I see in one of its threads: gettid() = 20306 open("/proc/20306/attr/current", O_WRONLY) = 3 write(3, "changeprofile libvirt-7d781722-6"..., 58) = 58 close(3) = 0 chown("/tmp/qrt-test-libvirt/libvirt/qatest/qatest.img", 116, 123) = 0 setregid(123, 123) = -1 EPERM (Operation not permitted) This chown appears to come from qemuSecurityDACSetProcessLabel(). What seems to be happening is that in __virExec() we call the security hook and the apparmor hook is being called before the DAC one, so we aa_change_profile() to the more restricted libvirt-<uuid> profile. It seems that it would be preferable to reverse the calling order of the hooks, but I am not sure how to do that. -- Jamie Strandboge | http://www.canonical.com
On Thu, 2010-09-23 at 15:02 -0500, Jamie Strandboge wrote:
Hmm, that's a libvirt error rather than a QEMU error. Is the restricted AppArmour policy taking effect *before* the actual QEMU binary is exec()d ?
This is related to the stacked security driver implementation. Specifically, if I strace libvirtd, I see in one of its threads: gettid() = 20306 open("/proc/20306/attr/current", O_WRONLY) = 3 write(3, "changeprofile libvirt-7d781722-6"..., 58) = 58 close(3) = 0 chown("/tmp/qrt-test-libvirt/libvirt/qatest/qatest.img", 116, 123) = 0 setregid(123, 123) = -1 EPERM (Operation not permitted)
This chown appears to come from qemuSecurityDACSetProcessLabel(). What seems to be happening is that in __virExec() we call the security hook and the apparmor hook is being called before the DAC one, so we aa_change_profile() to the more restricted libvirt-<uuid> profile. It seems that it would be preferable to reverse the calling order of the hooks, but I am not sure how to do that.
Err.. of course I meant both the 'chown' and the 'setregid' appear to come from qemuSecurityDACSetProcessLabel() and that it is the setregid that is causing the error. -- Jamie Strandboge | http://www.canonical.com
participants (2)
-
Daniel P. Berrange -
Jamie Strandboge