[libvirt] Resubmission: [PATCH 0/6] sVirt AppArmor security driver
Resubmitting based on feedback from this list. Notably, *alloc calls have been removed and syntax-check completes without error for all files. Jamie On Fri, 04 Sep 2009, Jamie Strandboge wrote:
This patch series implements the AppArmor security driver for sVirt. This implementation was developed for the Ubuntu AppArmorLibvirtProfile specification[1], but is general enough for any AppArmor deployment (such as Ubuntu, *SUSE and Mandriva).
This patch has seen quite a bit of real world testing in Ubuntu 9.10 (our development release) in our 0.7.0-1ubuntu3 package. I did make a few small changes after going through HACKING, but mostly I got the tests going and added documentation.
DESIGN ------ When a virtual machine is started, determine if a profile is currently defined for the machine, and use it if available. If not, generate a new profile for the machine based on a template, which is by default a very restrictive profile allowing access to disk files, and anything else needed to run, such as the pid, monitor and log files.
Virtual machines should have a unique profile specific to that machine. To ensure uniqueness, the profile name will be derived from the UUID of the virtual machine. These profiles should be configurable, either by adjusting the profile template for new machines, creating/modifying the VM profile directly or through the use of AppArmor abstractions. This will allow for administrators to fine-tune confinement for individual machines if desired.
If enabled at compile time, the sVirt security model will be activated if AppArmor is available on the host OS and a profile for the libvirtd daemon is loaded when libvirtd is started.
libvirtd should not be allowed to create arbitrary profiles or modify profiles directly, so as to not allow libvirtd to potentially (ie via a security bug in libvirtd itself) bootstrap out of AppArmor confinement.
Because root privileges are needed to manipulate AppArmor profiles, qemu:///session will not be supported at this time, but the implementation must allow for a confined libvirtd with qemu:///session guests running unconfined. This can be revisited when AppArmor supports per-user profiles.
Please see the specification[1] for more details.
PATCHES ------- The patches are all against trunk as of yesterday. Testing was done on trunk and there seem to be no regressions over the the 0.7.0-1ubuntu3 package in Ubuntu.
[PATCH 1*] patch_1a_reenable-nonfile-labels.patch: When James Morris originally submitted his sVirt patches (as seen in libvirt 0.6.1), he did not require on disk labelling for virSecurityDomainRestoreImageLabel. A later commit[2] changed this behavior to assume on disk labelling, which halts implementations for path-based MAC systems such as AppArmor and TOMOYO where vm->def->seclabel is required to obtain the label. This patch simply adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel.
patch_1b_optional.patch: Due to the above change, 'make syntax-check' fails because SELinuxRestoreSecurityImageLabel() does not use the 'virDomainObjPtr vm'. patch_1b_optional.patch is a simple patch to fix this by checking if vm->def->seclabel == NULL and returns with error if it does. I realize this may not be desired in the long term, but it should be harmless enough to include.
[PATCH 2] patch_2_security_c.patch: Updates src/security.c for AppArmor
[PATCH 3] patch_3_security_apparmor.patch: Adds security_apparmor.c, security_apparmor.h, virt-aa-helper.c and updates po/POTFILES.in. virt-aa-helper.c is a new binary which is used exclusively by the AppArmor security driver to manipulate AppArmor. These files compile without warning and pass syntax-check.
[PATCH 4] patch_4_tests.patch: Adds tests for virt-aa-helper and the security driver. secaatest.c is identical to seclabeltest.c except it initializes the 'apparmor' driver instead of 'selinux'. These tests are integrated into 'make check' and pass.
[PATCH 5] patch_5_docs.patch: Updates docs/drvqemu.html.in for AppArmor and adds profile examples to examples/apparmor.
[PATCH 6] patch_6_autoconf.patch: Updates Makefile.am and configure.in for AppArmor. It is based on and should operate the same as the SELinux configuration.
Caveats and known issues: 1. it does not take advantage of the recent host device labelling functionality yet 2. it does not properly handle hot-plugging of devices yet 3. qemu:///session runs unconfined (see above)
Thanks!
Jamie (jdstrand on Freenode and OFTC)
[1] https://wiki.ubuntu.com/SecurityTeam/Specifications/AppArmorLibvirtProfile [2] http://libvirt.org/git/?p=libvirt.git;a=commit;h=c86afc85ee0d1ec6d76c2d254ba...
-- Jamie Strandboge | http://www.canonical.com
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 1*] patch_1a_reenable-nonfile-labels.patch: When James Morris originally submitted his sVirt patches (as seen in libvirt 0.6.1), he did not require on disk labelling for virSecurityDomainRestoreImageLabel. A later commit[2] changed this behavior to assume on disk labelling, which halts implementations for path-based MAC systems such as AppArmor and TOMOYO where vm->def->seclabel is required to obtain the label. This patch simply adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel.
patch_1b_optional.patch: Due to the above change, 'make syntax-check' fails because SELinuxRestoreSecurityImageLabel() does not use the 'virDomainObjPtr vm'. patch_1b_optional.patch is a simple patch to fix this by checking if vm->def->seclabel == NULL and returns with error if it does. I realize this may not be desired in the long term, but it should be harmless enough to include.
-- Jamie Strandboge | http://www.canonical.com
On Tue, Sep 08, 2009 at 04:19:59PM -0500, Jamie Strandboge wrote:
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 1*] patch_1a_reenable-nonfile-labels.patch: When James Morris originally submitted his sVirt patches (as seen in libvirt 0.6.1), he did not require on disk labelling for virSecurityDomainRestoreImageLabel. A later commit[2] changed this behavior to assume on disk labelling, which halts implementations for path-based MAC systems such as AppArmor and TOMOYO where vm->def->seclabel is required to obtain the label. This patch simply adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel.
patch_1b_optional.patch: Due to the above change, 'make syntax-check' fails because SELinuxRestoreSecurityImageLabel() does not use the 'virDomainObjPtr vm'. patch_1b_optional.patch is a simple patch to fix this by checking if vm->def->seclabel == NULL and returns with error if it does. I realize this may not be desired in the long term, but it should be harmless enough to include.
The 'ATTRIBUTE_UNUSED' anotation takes care of this much better.
diff -Nurp ./libvirt.orig/src/qemu_driver.c ./libvirt/src/qemu_driver.c --- ./libvirt.orig/src/qemu_driver.c 2009-09-08 13:00:00.000000000 -0500 +++ ./libvirt/src/qemu_driver.c 2009-09-08 15:32:22.000000000 -0500 @@ -6144,7 +6144,7 @@ static int qemudDomainDetachDevice(virDo dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) { ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); if (driver->securityDriver) - driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); + driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, vm, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) VIR_WARN0("Fail to restore disk device ownership"); } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { diff -Nurp ./libvirt.orig/src/security.h ./libvirt/src/security.h --- ./libvirt.orig/src/security.h 2009-08-17 11:00:40.000000000 -0500 +++ ./libvirt/src/security.h 2009-09-08 15:32:22.000000000 -0500 @@ -32,6 +32,7 @@ typedef virSecurityDriverStatus (*virSec typedef int (*virSecurityDriverOpen) (virConnectPtr conn, virSecurityDriverPtr drv); typedef int (*virSecurityDomainRestoreImageLabel) (virConnectPtr conn, + virDomainObjPtr vm, virDomainDiskDefPtr disk); typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn, virDomainObjPtr vm, diff -Nurp ./libvirt.orig/src/security_selinux.c ./libvirt/src/security_selinux.c --- ./libvirt.orig/src/security_selinux.c 2009-09-02 14:34:08.000000000 -0500 +++ ./libvirt/src/security_selinux.c 2009-09-08 15:32:22.000000000 -0500 @@ -354,6 +354,7 @@ SELinuxSetFilecon(virConnectPtr conn, co
static int SELinuxRestoreSecurityImageLabel(virConnectPtr conn, + virDomainObjPtr vm, virDomainDiskDefPtr disk) { struct stat buf; @@ -423,7 +429,8 @@ SELinuxRestoreSecurityLabel(virConnectPt int rc = 0; if (secdef->imagelabel) { for (i = 0 ; i < vm->def->ndisks ; i++) { - if (SELinuxRestoreSecurityImageLabel(conn, vm->def->disks[i]) < 0) + if (SELinuxRestoreSecurityImageLabel(conn, vm, + vm->def->disks[i]) < 0) rc = -1; } VIR_FREE(secdef->model);
ACK to this part
diff -Nurp ./libvirt.orig/src/security_selinux.c ./libvirt/src/security_selinux.c --- ./libvirt.orig/src/security_selinux.c 2009-09-02 14:34:08.000000000 -0500 +++ ./libvirt/src/security_selinux.c 2009-09-08 15:32:22.000000000 -0500 @@ -364,6 +365,11 @@ SELinuxRestoreSecurityImageLabel(virConn char *newpath = NULL; const char *path = disk->src;
+ if (&vm->def->seclabel == NULL) { + virSecurityReportError(conn, VIR_ERR_ERROR, _("seclabel is NULL")); + return rc; + } + /* Don't restore labels on readoly/shared disks, because * other VMs may still be accessing these * Alternatively we could iterate over all running
This should have ATTRIBUTE_USED instead Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Jamie Strandboge wrote:
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 1*] patch_1a_reenable-nonfile-labels.patch: When James Morris originally submitted his sVirt patches (as seen in libvirt 0.6.1), he did not require on disk labelling for virSecurityDomainRestoreImageLabel. A later commit[2] changed this behavior to assume on disk labelling, which halts implementations for path-based MAC systems such as AppArmor and TOMOYO where vm->def->seclabel is required to obtain the label. This patch simply adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel.
patch_1b_optional.patch: Due to the above change, 'make syntax-check' fails because SELinuxRestoreSecurityImageLabel() does not use the 'virDomainObjPtr vm'. patch_1b_optional.patch is a simple patch to fix this by checking if vm->def->seclabel == NULL and returns with error if it does. I realize this may not be desired in the long term, but it should be harmless enough to include.
It looks fine, except instead of using the optional patch, just use "ATTRIBUTE_UNUSED" instead. (Oh, I see danpb gave you the same feedback :). -- Chris Lalancette
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 2] patch_2_security_c.patch: Updates src/security.c for AppArmor
-- Jamie Strandboge | http://www.canonical.com
On Tue, Sep 08, 2009 at 04:20:43PM -0500, Jamie Strandboge wrote:
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 2] patch_2_security_c.patch: Updates src/security.c for AppArmor
-- Jamie Strandboge | http://www.canonical.com
diff -Nurp ./libvirt.orig/src/security.c ./libvirt/src/security.c --- ./libvirt.orig/src/security.c 2009-08-17 11:00:40.000000000 -0500 +++ ./libvirt/src/security.c 2009-09-08 15:32:22.000000000 -0500 @@ -20,10 +20,17 @@ #include "security_selinux.h" #endif
+#ifdef WITH_SECDRIVER_APPARMOR +#include "security_apparmor.h" +#endif + static virSecurityDriverPtr security_drivers[] = { #ifdef WITH_SECDRIVER_SELINUX &virSELinuxSecurityDriver, #endif +#ifdef WITH_SECDRIVER_APPARMOR + &virAppArmorSecurityDriver, +#endif NULL };
ACK, but this part should really be part of the next patch, since you can't apply this & still compile without also applying the next patch Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 3] patch_3_security_apparmor.patch: Adds security_apparmor.c, security_apparmor.h, virt-aa-helper.c and updates po/POTFILES.in. virt-aa-helper.c is a new binary which is used exclusively by the AppArmor security driver to manipulate AppArmor. These files compile without warning and pass syntax-check.
-- Jamie Strandboge | http://www.canonical.com
On Tue, Sep 08, 2009 at 04:21:23PM -0500, Jamie Strandboge wrote:
diff -Nurp ./libvirt.orig/src/security_apparmor.c ./libvirt/src/security_apparmor.c --- ./libvirt.orig/src/security_apparmor.c 1969-12-31 18:00:00.000000000 -0600 +++ ./libvirt/src/security_apparmor.c 2009-09-08 15:32:42.000000000 -0500
+/* + * profile_status returns '-1' on error, '0' if loaded + * + * If check_enforcing is set to '1', then returns '-1' on error, '0' if + * loaded in complain mode, and '1' if loaded in enforcing mode. + */ +static int +profile_status(const char *str, const int check_enforcing) +{ + char ebuf[1024]; + char *content = NULL; + char *tmp = NULL; + char *etmp = NULL; + int rc = -1; + + /* create string that is '<str> \0' for accurate matching */ + if (VIR_ALLOC_N(tmp, strlen(str) + 2) < 0) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + _ + ("%s: could not allocate memory for string"), + __func__); + return rc; + } + sprintf(tmp, "%s ", str); + + if (check_enforcing != 0) { + /* create string that is '<str> (enforce)\0' for accurate matching */ + if (VIR_ALLOC_N(etmp, strlen(str) + 11) < 0) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + _("%s: could not allocate memory for " + "string"), __func__); + VIR_FREE(tmp); + return rc; + } + sprintf(etmp, "%s (enforce)", str); + }
Can you replace the VIR_ALLOC_N + sprintf calls with just virAsprintf() which is safer wrt future changes because it is guarenteed to automatically allocate the correct amount of memory without us needing to hardcode it. Likewise for future instances of ALLOC+printf in this patch.
+ + if (virFileReadAll(APPARMOR_PROFILES_PATH, MAX_FILE_LEN, &content) < 0) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + _ + ("%s: Failed to read AppArmor profiles list " + "%s: %s"), __func__, + APPARMOR_PROFILES_PATH, virStrerror(errno, + ebuf, + sizeof + ebuf));
I realize you're just copying the existing SELinux code style here, but unfortunately the SELinux driver code was not following our best practices for error reporting. Any place where you have an 'errno' value should call the generic virReportSystemError() function, and not use virStrerror(). Also, there's no need to ever pass __func__ to any error reporting APIs since we capture function, filename & lineno automatically in all cases.
+static int +load_profile(const char *profile, virDomainObjPtr vm, + const char *skip_disk) +{ + const char **argv; + int rc = -1; + int offset; + size_t i; + size_t len; + int create; + int j; + + create = 1; + offset = 6; + if (profile_status_file(profile) >= 0) + create = 0; + len = offset + vm->def->ndisks + 1; + + if (VIR_ALLOC_N(argv, len) < 0) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + _("%s: could not allocate memory"), + __func__); + return rc; + } + + argv[0] = VIRT_AA_HELPER_PATH; + argv[2] = (char *) "-u"; + argv[3] = profile; + argv[4] = (char *) "-n"; + argv[5] = vm->def->name; + + if (create == 0) + argv[1] = (char *) "-r"; + else + argv[1] = (char *) "-c"; + + /* add disks to argv, skipping skip_disk if it is defined. */ + j = 0; + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->src == NULL || (skip_disk != NULL && + STREQ(skip_disk, + vm->def-> + disks[i]->src))) { + continue; + } + if (offset + j >= len - 1) + break; + argv[offset + j] = vm->def->disks[i]->src; + j++; + } + argv[offset + j] = NULL; + + if (virRun(NULL, argv, NULL) == 0) + rc = 0; + + VIR_FREE(argv); + return rc; +}
IMHO the way data is passed from the security driver to the aa-helper program is sub-optimal & requires too much unneccessary code. As you add in support for USB & PCI device labelling you'll have a large number of additional args to pass to the helper. We'll likely add even more later for serial/parallel device assignment & other config options. In effect we'll end up inventing a entirely new custom data interchange format for this helper, when we already have a perfect one for the job.... the domain XML. Thus I'd prefer to see this code simply take the virDomainDefPtr object, call virDomainDefFormat(), and feed the resulting XML string to the helper program on its STDIN file descriptor (or any other FD if desired). The helper program can then simply read the XML off the FD, and call virDomainDefParseString() to get back a virDomainDefPtr object. That way guarentees the aa-helper always has all the data it will ever need about the geust config without us needing to add more & more command line options to aa-helper to deal with new config params.
+ +/* + * profile_name is buffer to hold name and len is how many bytes in the + * buffer + */ +static int +get_profile_name(virConnectPtr conn, + virDomainObjPtr vm, char *profile_name, const size_t len) +{ + virDomainPtr dom = NULL; + int rc = -1; + char *prefix = (char *) "libvirt-";
Don't cast away const-ness, declare the variable with the neccessary const-ness eg const char *prefix = "libvirt-";
+ + if (len < PROFILE_NAME_SIZE) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: profile_name has wrong size"), + __func__); + return rc; + } + profile_name[0] = '\0'; + strcat(profile_name, prefix); + + /* generate the profile name */ + dom = virGetDomain(conn, vm->def->name, vm->def->uuid); + if (!dom) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: could not get domain for VM"), + __func__);
There's no need to raise an error when virGetDomain calls fail - it will already have done that for you.
+typedef struct { + char uuid[PROFILE_NAME_SIZE]; /* UUID of vm */ + char name[PATH_MAX]; /* name of vm */ + bool dryrun; /* dry run */ + char cmd; /* 'c' create + * 'a' add (load) + * 'r' replace + * 'R' remove */ + int ndisks; /* number of disks */ + char **disks; /* list of disks */ +} vahControl; + +static int +vahDeinit(vahControl * ctl) +{ + int i; + + if (ctl->disks != NULL) { + for (i = 0; i < ctl->ndisks; i++) { + if (ctl->disks[i] == NULL) + continue; + free(ctl->disks[i]); + } + VIR_FREE(ctl->disks); + } + return 0; +} + +static void +vah_error(vahControl * ctl, int doexit, const char *str) +{ + fprintf(stderr, _("%s: error: %s\n"), progname, str); + + if (doexit) { + if (ctl != NULL) + vahDeinit(ctl); + exit(EXIT_FAILURE); + } +} + +static void +vah_warning(const char *str) +{ + fprintf(stderr, _("%s: warning: %s\n"), progname, str); +} + +static void +vah_info(const char *str) +{ + fprintf(stderr, _("%s:\n%s\n"), progname, str); +} + +/* + * Replace @oldstr in @orig with @repstr + * @len is number of bytes allocated for @orig. Assumes @orig, @oldstr and + * @repstr are null terminated + */ +static int +replace_string(char *orig, const size_t len, const char *oldstr, + const char *repstr) +{ + int idx; + char *pos = NULL; + char *tmp = NULL; + + if ((pos = strstr(orig, oldstr)) == NULL) { + vah_error(NULL, 0, "could not find replacement string"); + return -1; + } + + if (VIR_ALLOC_N(tmp, len) < 0) { + vah_error(NULL, 0, "could not allocate memory for string"); + return -1; + } + tmp[0] = '\0'; + + idx = abs(pos - orig); + + /* copy everything up to oldstr */ + strncat(tmp, orig, idx); + + /* add the replacement string */ + if (strlen(tmp) + strlen(repstr) > len - 1) { + vah_error(NULL, 0, "not enough space in target buffer"); + VIR_FREE(tmp); + return -1; + } + strcat(tmp, repstr); + + /* add everything after oldstr */ + if (strlen(tmp) + strlen(orig) - (idx + strlen(oldstr)) > len - 1) { + vah_error(NULL, 0, "not enough space in target buffer"); + VIR_FREE(tmp); + return -1; + } + strncat(tmp, orig + idx + strlen(oldstr), + strlen(orig) - (idx + strlen(oldstr))); + + strncpy(orig, tmp, len); + orig[len - 1] = '\0'; + VIR_FREE(tmp); + + return 0; +} + +/* + * run an apparmor_parser command + */ +static int +parserCommand(const char *profile_name, const char cmd) +{ + const char *argv[4]; + char flag[3]; + char profile[PATH_MAX]; + + if (strchr("arR", cmd) == NULL) { + vah_error(NULL, 0, "invalid flag"); + return -1; + } + + snprintf(flag, 3, "-%c", cmd); + + if (snprintf(profile, PATH_MAX, "%s/%s", + APPARMOR_DIR "/libvirt", profile_name) > PATH_MAX - 1) { + vah_error(NULL, 0, "profile name exceeds maximum length"); + return -1; + } + + if (!virFileExists(profile)) { + vah_error(NULL, 0, "profile does not exist"); + return -1; + } + + argv[0] = (char *) "/sbin/apparmor_parser"; + argv[1] = flag; + argv[2] = profile; + argv[3] = NULL; + + if (virRun(NULL, argv, NULL) != 0) { + vah_error(NULL, 0, "failed to run apparmor_parser"); + return -1; + } + + return 0; +} + +/* + * Update the disks file + */ +static int +update_include_file(const char *include_file, const char *included_files) +{ + int plen; + int fd; + char *pcontent = NULL; + char *existing = NULL; + char *warning = (char *) + "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n";
More const-ness oissues here.
+ /* While 3*PATH_MAX is not accurate when thinking about PATH_MAX, + * it is good enough since the #include line is a relative path + * and we are just trying to keep tmp from overflowing here anyway + */ + if (snprintf(tmp, 3 * PATH_MAX, + " %s/log/libvirt/**/%s.log w,\n" + " %s/lib/libvirt/**/%s.monitor rw,\n" + " %s/run/libvirt/**/%s.pid rwk,\n", + LOCAL_STATE_DIR, ctl->name, LOCAL_STATE_DIR, ctl->name, + LOCAL_STATE_DIR, ctl->name) > 3 * PATH_MAX - 1) { + vah_error(ctl, 0, "added rules exceed maximum length"); + goto clean; + }
It is much safer to just use virAsprintf() here instead of a huge static buffer Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 4] patch_4_tests.patch: Adds tests for virt-aa-helper and the security driver. secaatest.c is identical to seclabeltest.c except it initializes the 'apparmor' driver instead of 'selinux'. These tests are integrated into 'make check' and pass.
-- Jamie Strandboge | http://www.canonical.com
On Tue, Sep 08, 2009 at 04:22:14PM -0500, Jamie Strandboge wrote:
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 4] patch_4_tests.patch: Adds tests for virt-aa-helper and the security driver. secaatest.c is identical to seclabeltest.c except it initializes the 'apparmor' driver instead of 'selinux'. These tests are integrated into 'make check' and pass.
-- Jamie Strandboge | http://www.canonical.com
diff -Nurp ./libvirt.orig/tests/Makefile.am ./libvirt/tests/Makefile.am --- ./libvirt.orig/tests/Makefile.am 2009-08-17 11:00:40.000000000 -0500 +++ ./libvirt/tests/Makefile.am 2009-09-08 15:32:22.000000000 -0500 @@ -77,6 +77,10 @@ if WITH_SECDRIVER_SELINUX noinst_PROGRAMS += seclabeltest endif
+if WITH_SECDRIVER_APPARMOR +noinst_PROGRAMS += secaatest +endif + if WITH_CIL noinst_PROGRAMS += object-locking endif @@ -112,6 +116,9 @@ test_scripts += \ virsh-synopsis endif
+if WITH_SECDRIVER_APPARMOR +test_scripts += virt-aa-helper-test +endif EXTRA_DIST += $(test_scripts)
TESTS = virshtest \ @@ -138,6 +145,10 @@ if WITH_SECDRIVER_SELINUX TESTS += seclabeltest endif
+if WITH_SECDRIVER_APPARMOR +TESTS += secaatest +endif + if WITH_LIBVIRTD noinst_PROGRAMS += eventtest TESTS += eventtest @@ -255,6 +266,14 @@ else EXTRA_DIST += seclabeltest.c endif
+if WITH_SECDRIVER_APPARMOR +secaatest_SOURCES = \ + secaatest.c +secaatest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) +else +EXTRA_DIST += secaatest.c +endif + qparamtest_SOURCES = \ qparamtest.c testutils.h testutils.c qparamtest_LDADD = $(LDADDS) diff -Nurp ./libvirt.orig/tests/secaatest.c ./libvirt/tests/secaatest.c --- ./libvirt.orig/tests/secaatest.c 1969-12-31 18:00:00.000000000 -0600 +++ ./libvirt/tests/secaatest.c 2009-09-08 15:32:22.000000000 -0500 @@ -0,0 +1,45 @@ +#include <config.h> + +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <errno.h> +#include "security.h" + +int +main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + int ret; + + const char *doi, *model; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup (&security_drv, "apparmor"); + if (ret == -1) + { + fprintf (stderr, "Failed to start security driver"); + exit (-1); + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + model = virSecurityDriverGetModel (security_drv); + if (!model) + { + fprintf (stderr, "Failed to copy secModel model: %s", + strerror (errno)); + exit (-1); + } + + doi = virSecurityDriverGetDOI (security_drv); + if (!doi) + { + fprintf (stderr, "Failed to copy secModel DOI: %s", + strerror (errno)); + exit (-1); + } + + return 0; +} diff -Nurp ./libvirt.orig/tests/virt-aa-helper-test ./libvirt/tests/virt-aa-helper-test --- ./libvirt.orig/tests/virt-aa-helper-test 1969-12-31 18:00:00.000000000 -0600 +++ ./libvirt/tests/virt-aa-helper-test 2009-09-08 15:32:22.000000000 -0500 @@ -0,0 +1,100 @@ +#!/bin/sh +set -e + +output="/dev/null" +use_valgrind="" +ld_library_path="" +if [ ! -z "$1" ] && [ "$1" = "-d" ]; then + output="/dev/stdout" + shift +fi + +exe="../src/virt-aa-helper" +if [ ! -z "$1" ]; then + if [ "$1" = "-v" ]; then + use_valgrind="yes" + exe="./src/.libs/virt-aa-helper" + ld_library_path="./src/.libs" + else + exe="$1" + fi + shift +fi + +if [ ! -x "$exe" ]; then + echo "Could not find '$exe'" + exit 1 +fi + +echo "testing `basename $exe`" >$output +if [ "$use_valgrind" = "yes" ]; then + exe="valgrind --error-exitcode=2 --track-origins=yes $exe" +fi + +extra_args="--dryrun" +errors=0 + +tmpdir=`mktemp -d` +trap "rm -rf $tmpdir" EXIT HUP INT QUIT TERM + +disk1="$tmpdir/1.img" +disk2="$tmpdir/2.img" +relative_disk1="$tmpdir/./../`basename $tmpdir`//./1.img" +nonexistent="$tmpdir/nonexistant.img" +bad_disk="/etc/passwd" +valid_uuid="libvirt-00000000-0000-0000-0000-0123456789ab" +valid_name="foo" +nonexistent_uuid="libvirt-00000000-0000-0000-0000-000000000001" +touch "$disk1" "$disk2" + +testme() { + expected="$1" + outstr="$2" + args="$3" + echo -n " $outstr: " >$output + echo " '$extra_args $args': " >$output + set +e + LD_LIBRARY_PATH="$ld_library_path" $exe $extra_args $args >$output 2>&1 + rc="$?" + set -e + if [ "$rc" = "$expected" ]; then + echo "pass" >$output + else + echo "FAIL: exited with '$rc'" >$output + errors=$(($errors + 1)) + fi +} + +# Expected failures +echo "Expected failures:" >$output +testme "1" "invalid arg" "-z" +testme "1" "invalid case" "-A" +testme "1" "not enough args" "-c" +testme "1" "missing name" "-c -n -u $valid_uuid $disk1" +testme "1" "bad name" "-c -n foo[a-z] -u $valid_uuid $disk1" +testme "1" "no -u with -c" "-c -n $valid_name $disk1" +testme "1" "bad uuid (bad digit)" "-c -n $valid_name -u libvirt-00000000-0000-0000-0000-00000000000g $disk1" +testme "1" "bad uuid (too long)" "-c -n $valid_name -u ${valid_uuid}abcdef $disk1" +testme "1" "bad uuid (too short)" "-c -n $valid_name -u libvirt-00000000-0000-0000-0000-0123456789a $disk1" +testme "1" "missing uuid" "-c -n $valid_name -u $disk1" +testme "1" "no -u with -R" "-R" +testme "1" "non-existent uuid" "-R -u $nonexistent_uuid" +testme "1" "no -u with -r" "-r" +testme "1" "no name with -r" "-r -u $valid_uuid $disk1" +testme "1" "bad disk" "-c -n $valid_name -u $valid_uuid $bad_disk" +testme "1" "bad disk2" "-c -n $valid_name -u $valid_uuid $disk1 $bad_disk $disk2" + +echo "Expected pass:" >$output +testme "0" "create" "-c -n foo -u $valid_uuid $disk1" +testme "0" "create (non-existent disk)" "-c -n foo -u $valid_uuid $nonexistent" +testme "0" "create (relative path)" "-c -n foo -u $valid_uuid $relative_disk1" +testme "0" "replace" "-r -n foo -u $valid_uuid $disk2" +testme "0" "replace (non-existent disk)" "-r -n foo -u $valid_uuid $nonexistent" +testme "0" "help" "-h" + +echo "" >$output +if [ "$errors" != "0" ]; then + echo "FAIL: $errors error(s)" >$output + exit 1 +fi +echo PASS >$output
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 5] patch_5_docs.patch: Updates docs/drvqemu.html.in for AppArmor and adds profile examples to examples/apparmor.
-- Jamie Strandboge | http://www.canonical.com
On Tue, Sep 08, 2009 at 04:22:56PM -0500, Jamie Strandboge wrote:
diff -Nurp ./libvirt.orig/examples/apparmor/usr.sbin.libvirtd ./libvirt/examples/apparmor/usr.sbin.libvirtd --- ./libvirt.orig/examples/apparmor/usr.sbin.libvirtd 1969-12-31 18:00:00.000000000 -0600 +++ ./libvirt/examples/apparmor/usr.sbin.libvirtd 2009-09-08 15:32:22.000000000 -0500 @@ -0,0 +1,39 @@ +# Last Modified: Mon Jul 6 17:23:58 2009 +#include <tunables/global> +@{LIBVIRT}="libvirt" + +/usr/sbin/libvirtd { + #include <abstractions/base> + + capability kill, + capability net_admin, + capability net_raw, + capability setgid, + capability sys_admin, + capability sys_module, + capability sys_ptrace,
I'm fairly sure libvirtd will need more than this set of capabilities. We tried to limit this in the C code a few months back, but gave up because we ended up requiring about 2/3s of all capabilities and once you allow net_admin & sys_admin its game over for security benefits. You'll certainly have broken functionality without sys_nice, sys_chroot, setuid, setpcap, mknod, dac_override, dac_read_search, fowner, chown Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 6] patch_6_autoconf.patch: Updates Makefile.am and configure.in for AppArmor. It is based on and should operate the same as the SELinux configuration.
-- Jamie Strandboge | http://www.canonical.com
On Tue, Sep 08, 2009 at 04:23:34PM -0500, Jamie Strandboge wrote:
On Tue, 08 Sep 2009, Jamie Strandboge wrote:
[PATCH 6] patch_6_autoconf.patch: Updates Makefile.am and configure.in for AppArmor. It is based on and should operate the same as the SELinux configuration.
-- Jamie Strandboge | http://www.canonical.com
diff -Nurp ./libvirt.orig/configure.in ./libvirt/configure.in --- ./libvirt.orig/configure.in 2009-09-08 12:59:59.000000000 -0500 +++ ./libvirt/configure.in 2009-09-08 15:32:22.000000000 -0500 @@ -799,6 +799,84 @@ fi AM_CONDITIONAL([WITH_SECDRIVER_SELINUX], [test "$with_secdriver_selinux" != "no"])
+dnl AppArmor +AC_ARG_WITH([apparmor], + [ --with-apparmor use AppArmor to manage security], + [], + [with_apparmor=check]) + +APPARMOR_CFLAGS= +APPARMOR_LIBS= +if test "$with_apparmor" != "no"; then + old_cflags="$CFLAGS" + old_libs="$LIBS" + if test "$with_apparmor" = "check"; then + AC_CHECK_HEADER([sys/apparmor.h],[],[with_apparmor=no]) + AC_CHECK_LIB([apparmor], [aa_change_profile],[],[with_apparmor=no]) + AC_CHECK_LIB([apparmor], [aa_change_hat],[],[with_apparmor=no]) + if test "$with_apparmor" != "no"; then + with_apparmor="yes" + fi + else + fail=0 + AC_CHECK_HEADER([sys/apparmor.h],[],[fail=1]) + AC_CHECK_LIB([apparmor], [aa_change_profile],[],[fail=1]) + AC_CHECK_LIB([apparmor], [aa_change_hat],[],[fail=1]) + test $fail = 1 && + AC_MSG_ERROR([You must install the AppArmor development package in order to compile libvirt]) + fi + CFLAGS="$old_cflags" + LIBS="$old_libs" +fi +if test "$with_apparmor" = "yes"; then + APPARMOR_LIBS="-lapparmor" + AC_DEFINE_UNQUOTED([HAVE_APPARMOR], 1, [whether AppArmor is available for security]) + AC_DEFINE_UNQUOTED([APPARMOR_DIR], "/etc/apparmor.d", [path to apparmor directory]) + AC_DEFINE_UNQUOTED([APPARMOR_PROFILES_PATH], "/sys/kernel/security/apparmor/profiles", [path to kernel profiles]) + AC_DEFINE_UNQUOTED([VIRT_AA_HELPER_PATH], "$prefix/bin/virt-aa-helper", [path to virt-aa-helper]) +fi +AM_CONDITIONAL([HAVE_APPARMOR], [test "$with_apparmor" != "no"]) +AC_SUBST([APPARMOR_CFLAGS]) +AC_SUBST([APPARMOR_LIBS]) + + +AC_ARG_WITH([secdriver-apparmor], + [ --with-secdriver-apparmor use AppArmor security driver], + [], + [with_secdriver_apparmor=check]) + +if test "$with_apparmor" != "yes" ; then + if test "$with_secdriver_apparmor" = "check" ; then + with_secdriver_apparmor=no + else + AC_MSG_ERROR([You must install the AppArmor development package in order to compile libvirt]) + fi +else + old_cflags="$CFLAGS" + old_libs="$LIBS" + CFLAGS="$CFLAGS $APPARMOR_CFLAGS" + LIBS="$CFLAGS $APPARMOR_LIBS" + + fail=0 + AC_CHECK_FUNC([change_hat], [], [fail=1]) + AC_CHECK_FUNC([aa_change_profile], [], [fail=1]) + CFLAGS="$old_cflags" + LIBS="$old_libs" + + if test "$fail" = "1" ; then + if test "$with_secdriver_apparmor" = "check" ; then + with_secdriver_apparmor=no + else + AC_MSG_ERROR([You must install the AppArmor development package in order to compile libvirt]) + fi + else + with_secdriver_apparmor=yes + AC_DEFINE_UNQUOTED([WITH_SECDRIVER_APPARMOR], 1, [whether AppArmor security driver is available]) + fi +fi +AM_CONDITIONAL([WITH_SECDRIVER_APPARMOR], [test "$with_secdriver_apparmor" != "no"]) + +
dnl NUMA lib AC_ARG_WITH([numactl], @@ -1706,6 +1784,7 @@ AC_MSG_NOTICE([]) AC_MSG_NOTICE([Security Drivers]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([ SELinux: $with_secdriver_selinux]) +AC_MSG_NOTICE([ AppArmor: $with_secdriver_apparmor]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Driver Loadable Modules]) AC_MSG_NOTICE([]) @@ -1753,6 +1832,11 @@ AC_MSG_NOTICE([ selinux: $SELINUX_CFLAGS else AC_MSG_NOTICE([ selinux: no]) fi +if test "$with_apparmor" = "yes" ; then +AC_MSG_NOTICE([ apparmor: $APPARMOR_CFLAGS $APPARMOR_LIBS]) +else +AC_MSG_NOTICE([ apparmor: no]) +fi if test "$with_numactl" = "yes" ; then AC_MSG_NOTICE([ numactl: $NUMACTL_CFLAGS $NUMACTL_LIBS]) else diff -Nurp ./libvirt.orig/src/Makefile.am ./libvirt/src/Makefile.am --- ./libvirt.orig/src/Makefile.am 2009-09-08 13:00:00.000000000 -0500 +++ ./libvirt/src/Makefile.am 2009-09-08 15:32:22.000000000 -0500 @@ -9,6 +9,7 @@ INCLUDES = \ $(LIBSSH2_CFLAGS) \ $(XEN_CFLAGS) \ $(SELINUX_CFLAGS) \ + $(APPARMOR_CFLAGS) \ $(DRIVER_MODULE_CFLAGS) \ -DLIBDIR=\""$(libdir)"\" \ -DBINDIR=\""$(libexecdir)"\" \ @@ -216,6 +217,8 @@ SECURITY_DRIVER_SOURCES = \ SECURITY_DRIVER_SELINUX_SOURCES = \ security_selinux.h security_selinux.c
+SECURITY_DRIVER_APPARMOR_SOURCES = \ + security_apparmor.h security_apparmor.c
NODE_DEVICE_DRIVER_SOURCES = \ node_device.c node_device.h @@ -527,6 +530,9 @@ libvirt_la_LIBADD += libvirt_driver_secu if WITH_SECDRIVER_SELINUX libvirt_driver_security_la_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) endif +if WITH_SECDRIVER_APPARMOR +libvirt_driver_security_la_SOURCES += $(SECURITY_DRIVER_APPARMOR_SOURCES) +endif
# Add all conditional sources just in case... EXTRA_DIST += \ @@ -615,7 +621,7 @@ libvirt_la_LIBADD += \ libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)libvirt.syms \ -version-info @LIBVIRT_VERSION_INFO@ \ $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \ - $(LIBXML_LIBS) $(SELINUX_LIBS) \ + $(LIBXML_LIBS) $(SELINUX_LIBS) $(APPARMOR_LIBS) \ $(XEN_LIBS) $(DRIVER_MODULE_LIBS) \ $(DEVMAPPER_LIBS) \ @CYGWIN_EXTRA_LDFLAGS@ @MINGW_EXTRA_LDFLAGS@ @@ -654,6 +660,23 @@ virsh_LDADD = \ ../gnulib/lib/libgnu.la \ $(VIRSH_LIBS) virsh_CFLAGS = $(COVERAGE_CFLAGS) $(READLINE_CFLAGS) $(NUMACTL_CFLAGS) + +if WITH_SECDRIVER_APPARMOR +bin_PROGRAMS += virt-aa-helper + +virt_aa_helper_SOURCES = \ + virt-aa-helper.c + +virt_aa_helper_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) +virt_aa_helper_LDADD = \ + $(STATIC_BINARIES) \ + $(WARN_CFLAGS) \ + libvirt.la \ + ../gnulib/lib/libgnu.la \ + $(VIRSH_LIBS) +virt_aa_helper_CFLAGS = $(COVERAGE_CFLAGS) +endif + BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c libvirt.syms
virsh-net-edit.c: virsh.c Makefile.am
ACK, though again this patch needs to be adjusted wrt the others, since earlier patches look like they have build time depends on this one. It is probably counter-productive to try & split up this series since it is all one big feature. Probably just have the first generic cleanup, then the apparmour implementation, and then the documentation patch Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Sep 08, 2009 at 04:17:26PM -0500, Jamie Strandboge wrote:
Resubmitting based on feedback from this list. Notably, *alloc calls have been removed and syntax-check completes without error for all files.
Thanks ! I think this is an item for post 0.7.1, so most likely you won't get much comment before early next week, as focuse is now for the upcoming release, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Jamie Strandboge