[libvirt] [PATCH 0/6] sVirt AppArmor security driver
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 Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 1*] patch_1a_reenable-nonfile-labels.patch: patch_1b_optional.patch:
-- Jamie Strandboge | http://www.canonical.com
On Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 2] patch_2_security_c.patch:
-- Jamie Strandboge | http://www.canonical.com
On Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 3] patch_3_security_apparmor.patch:
-- Jamie Strandboge | http://www.canonical.com
On Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 4] patch_4_tests.patch:
-- Jamie Strandboge | http://www.canonical.com
On Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 5] patch_5_docs.patch:
-- Jamie Strandboge | http://www.canonical.com
On Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 6] patch_6_autoconf.patch:
-- Jamie Strandboge | http://www.canonical.com
On Fri, 04 Sep 2009, Jamie Strandboge wrote:
[PATCH 1*] patch_1a_reenable-nonfile-labels.patch: patch_1b_optional.patch:
Resending with the proper subject line. :\ -- Jamie Strandboge | http://www.canonical.com
On Fri, Sep 04, 2009 at 12:23:33PM -0500, 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.
Could you triple check that the make syntax-check run clean with your patches applied, because a very quick glimpse shows up a number of malloc() which we don't allow for example. It may be because you didn't enter the files in the SCM but the checks should be done anyway on the new files please. 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/
On Fri, 04 Sep 2009, Daniel Veillard wrote:
On Fri, Sep 04, 2009 at 12:23:33PM -0500, 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.
Could you triple check that the make syntax-check run clean with your patches applied, because a very quick glimpse shows up a number of malloc() which we don't allow for example. It may be because you didn't enter the files in the SCM but the checks should be done anyway on the new files please.
I noticed that syntax-check wouldn't scan the new files unless I did a git commit first. I did not run 'syntax-check' after adding the documention examples, and just found that examples/apparmor/libvirt-qemu had a trailing blank line (I was a bit surprised syntax-check checked these files (attached is an updated docs patch)). I can demonstrate that the files are being checked by removing the '#include <config.h>' line in security_apparmor.c and virt-aa-helper.c and syntax-check failing: $ make syntax-check .. require_config_h src/virt-aa-helper.c maint.mk: the above files do not include <config.h> make: *** [sc_require_config_h] Error 1 $ make syntax-check .. require_config_h src/security_apparmor.c maint.mk: the above files do not include <config.h> make: *** [sc_require_config_h] Error 1 I also knew the files were checked because, for example, I used str[n]cmp() in a couple places and prohibit_strcmp_and_strncmp caught them. I changed those to STR* before submission. I don't see that 'syntax-check' enforces not using *alloc, but I did read in HACKING that *alloc are deprecated, though I had already written the code before reading it. I do see other examples of *alloc in the codebase, and I do properly check the return code of all calls to *alloc. If you insist on those calls being replaced, I can do that, but I didn't before submitting because I knew the current calls were correct and didn't want to introduce bugs into the code. Here is what I do after a 'git pull': $ cd libvirt $ git checkout <apply patches> (using the attached patch_5_docs.patch) $ chmod 755 ./tests/virt-aa-helper-test $ git add . $ git commit ... [master 4b08401] add apparmor security driver 18 files changed, 1966 insertions(+), 3 deletions(-) create mode 100644 examples/apparmor/TEMPLATE create mode 100644 examples/apparmor/libvirt-qemu create mode 100644 examples/apparmor/usr.bin.virt-aa-helper create mode 100644 examples/apparmor/usr.sbin.libvirtd create mode 100644 src/security_apparmor.c create mode 100644 src/security_apparmor.h create mode 100644 src/virt-aa-helper.c create mode 100644 tests/secaatest.c create mode 100755 tests/virt-aa-helper-test $ ./autogen.sh $ ./configure --enable-compile-warnings=error $ make check .. CC security_apparmor.lo .. CC virt_aa_helper-virt-aa-helper.o CCLD virt-aa-helper .. CC secaatest.o CCLD secaatest .. PASS: virt-aa-helper-test .. PASS: secaatest .. (no errors, and I see my two added tests passed) $ make syntax-check void_if_before_free cast_of_argument_to_free cast_of_x_alloc_return_value prohibit_strcmp error_message_warn_fatal error_message_period prohibit_have_config_h require_config_h require_config_h_first prohibit_HAVE_MBRTOWC prohibit_assert_without_use prohibit_getopt_without_use prohibit_long_options_without_use prohibit_inttostr_without_use prohibit_error_without_use prohibit_safe_read_without_use prohibit_argmatch_without_use prohibit_root_dev_ino_without_use prohibit_c_ctype_without_use prohibit_signal_without_use changelog the_the trailing_blank unmarked_diagnostics prohibit_cvs_keyword proper_name_utf8_requires_ICONV redundant_const const_long_option makefile_TAB_only_indentation m4_quote_check po_check copyright_check avoid_write prohibit_strcmp_and_strncmp prohibit_asprintf prohibit_VIR_ERR_NO_MEMORY prohibit_nonreentrant prohibit_ctype_h TAB_in_indentation avoid_ctype_macros prohibit_virBufferAdd_with_string_literal prohibit_gethostby libvirt_unmarked_diagnostics prohibit_trailing_blank_lines Thanks! Jamie -- Jamie Strandboge | http://www.canonical.com
Jamie Strandboge wrote:
I don't see that 'syntax-check' enforces not using *alloc, but I did read in HACKING that *alloc are deprecated, though I had already written the code before reading it. I do see other examples of *alloc in the codebase, and I do properly check the return code of all calls to *alloc. If you insist on those calls being replaced, I can do that, but I didn't before submitting because I knew the current calls were correct and didn't want to introduce bugs into the code.
Yeah, make syntax-check doesn't do a check for the malloc/calloc/realloc in the code base, but it probably should. Actually, given the number of allocations that libvirt actually does, there are surprisingly few uses of malloc/calloc/realloc left, and we've been converting them as we come across them. It probably just needs a bit more concerted effort to finish the job. For that reason, my preference would be to not introduce new ones into the code. Once your patches go in, they'll be modified over time anyway, so changing them up-front shouldn't invalidate all of the testing. They will obviously need to be re-tested to make sure subtle issues didn't creep in, but it should be a fairly mechanical change. -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Jamie Strandboge