[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 0/6] sVirt AppArmor security driver



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
diff -Nurp ./libvirt.orig/docs/drvqemu.html.in ./libvirt/docs/drvqemu.html.in
--- ./libvirt.orig/docs/drvqemu.html.in	2009-09-02 14:34:08.000000000 -0500
+++ ./libvirt/docs/drvqemu.html.in	2009-09-04 15:42:13.000000000 -0500
@@ -296,6 +296,72 @@
       file can be used to change the setting to <code>security_driver="none"</code>
     </p>
 
+    <h3><a name="securitysvirtaa">AppArmor sVirt confinement</a></h3>
+
+    <p>
+      When using basic AppArmor protection for the libvirtd daemon and
+      QEMU virtual machines, the intention is to protect the host OS
+      from a compromised virtual machine process. There is no protection
+      between guests.
+    </p>
+
+    <p>
+      The AppArmor sVirt protection for QEMU virtual machines builds on
+      this basic level of protection, to also allow individual guests to
+      be protected from each other.
+    </p>
+
+    <p>
+      In the sVirt model, if a profile is loaded for the libvirtd daemon,
+      then each <code>qemu:///system</code> QEMU virtual machine will run
+      starts. This generated profile uses a profile name based on the UUID
+      of the QEMU virtual machine and contains rules allowing access to
+      only the files it needs to run, such as its disks, pid file and log
+      files. Just before the QEMU virtual machine is started, the libvirtd
+      daemon will change into this unique profile, preventing the QEMU
+      process from accessing any file resources that are present in another
+      QEMU process or the host machine.
+    </p>
+
+    <p>
+      The AppArmor sVirt implementation is flexible in that it allows an
+      administrator to customize the template file in
+      <code>/etc/apparmor.d/libvirt/TEMPLATE</code> for site-specific
+      access for all newly created QEMU virtual machines. Also, when a new
+      profile is generated, two files are created:
+      <code>/etc/apparmor.d/libvirt/libvirt-&lt;uuid&gt;</code> and
+      <code>/etc/apparmor.d/libvirt/libvirt-&lt;uuid&gt;.files</code>. The
+      former can be fine-tuned by the administrator to allow custom access
+      for this particular QEMU virtual machine, and the latter will be
+      updated appropriately when required file access changes, such as when
+      a disk is added. This flexibility allows for situations such as
+      having one virtual machine in complain mode with all others in
+      enforce mode.
+    </p>
+
+    <p>
+      While users can define their own AppArmor profile scheme, a typical
+      configuration will include a profile for <code>/usr/sbin/libvirtd</code>,
+      <code>/usr/bin/virt-aa-helper</code> (a helper program which the
+      libvirtd daemon uses instead of manipulating AppArmor directly), and
+      an abstraction to be included by <code>/etc/apparmor.d/libvirt/TEMPLATE</code>
+      (typically <code>/etc/apparmor.d/abstractions/libvirt-qemu</code>).
+      An example profile scheme can be found in the examples/apparmor
+      directory of the source distribution.
+    </p>
+
+    <p>
+      If the sVirt security model is active, then the node capabilities
+      XML will include its details. If a virtual machine is currently
+      protected by the security model, then the guest XML will include
+      its assigned profile name. 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. To disable sVirt, and revert to the basic level of AppArmor
+      protection (host protection only), the <code>/etc/libvirt/qemu.conf</code>
+      file can be used to change the setting to <code>security_driver="none"</code>.
+    </p>
+
 
     <h3><a name="securityacl">Cgroups device ACLs</a></h3>
 
diff -Nurp ./libvirt.orig/examples/apparmor/libvirt-qemu ./libvirt/examples/apparmor/libvirt-qemu
--- ./libvirt.orig/examples/apparmor/libvirt-qemu	1969-12-31 18:00:00.000000000 -0600
+++ ./libvirt/examples/apparmor/libvirt-qemu	2009-09-04 15:42:34.000000000 -0500
@@ -0,0 +1,70 @@
+# Last Modified: Wed Jul  8 09:57:41 2009
+
+  #include <abstractions/base>
+  #include <abstractions/consoles>
+  #include <abstractions/nameservice>
+
+  # required for reading disk images
+  capability dac_override,
+  capability dac_read_search,
+
+  network inet stream,
+  network inet6 stream,
+
+  /dev/net/tun rw,
+  /dev/kvm rw,
+  /dev/ptmx rw,
+  /dev/kqemu rw,
+
+  # WARNING: uncommenting these gives the guest direct access to host hardware.
+  # This is required for USB pass through but is a security risk. You have been
+  # warned.
+  #/sys/bus/usb/devices/ r,
+  #/sys/devices/*/*/usb[0-9]*/** r,
+  #/dev/bus/usb/*/[0-9]* rw,
+
+  /usr/share/kvm/** r,
+  /usr/share/qemu/** r,
+  /usr/share/bochs/** r,
+  /usr/share/openbios/** r,
+  /usr/share/openhackware/** r,
+  /usr/share/proll/** r,
+  /usr/share/vgabios/** r,
+
+  # the various binaries
+  /usr/bin/kvm rmix,
+  /usr/bin/qemu rmix,
+  /usr/bin/qemu-system-arm rmix,
+  /usr/bin/qemu-system-cris rmix,
+  /usr/bin/qemu-system-i386 rmix,
+  /usr/bin/qemu-system-m68k rmix,
+  /usr/bin/qemu-system-mips rmix,
+  /usr/bin/qemu-system-mips64 rmix,
+  /usr/bin/qemu-system-mips64el rmix,
+  /usr/bin/qemu-system-mipsel rmix,
+  /usr/bin/qemu-system-ppc rmix,
+  /usr/bin/qemu-system-ppc64 rmix,
+  /usr/bin/qemu-system-ppcemb rmix,
+  /usr/bin/qemu-system-sh4 rmix,
+  /usr/bin/qemu-system-sh4eb rmix,
+  /usr/bin/qemu-system-sparc rmix,
+  /usr/bin/qemu-system-sparc64 rmix,
+  /usr/bin/qemu-system-x86_64 rmix,
+  /usr/bin/qemu-alpha rmix,
+  /usr/bin/qemu-arm rmix,
+  /usr/bin/qemu-armeb rmix,
+  /usr/bin/qemu-cris rmix,
+  /usr/bin/qemu-i386 rmix,
+  /usr/bin/qemu-m68k rmix,
+  /usr/bin/qemu-mips rmix,
+  /usr/bin/qemu-mipsel rmix,
+  /usr/bin/qemu-ppc rmix,
+  /usr/bin/qemu-ppc64 rmix,
+  /usr/bin/qemu-ppc64abi32 rmix,
+  /usr/bin/qemu-sh4 rmix,
+  /usr/bin/qemu-sh4eb rmix,
+  /usr/bin/qemu-sparc rmix,
+  /usr/bin/qemu-sparc64 rmix,
+  /usr/bin/qemu-sparc32plus rmix,
+  /usr/bin/qemu-sparc64 rmix,
+  /usr/bin/qemu-x86_64 rmix,
diff -Nurp ./libvirt.orig/examples/apparmor/TEMPLATE ./libvirt/examples/apparmor/TEMPLATE
--- ./libvirt.orig/examples/apparmor/TEMPLATE	1969-12-31 18:00:00.000000000 -0600
+++ ./libvirt/examples/apparmor/TEMPLATE	2009-09-04 15:42:13.000000000 -0500
@@ -0,0 +1,9 @@
+#
+# This profile is for the domain whose UUID matches this file.
+#
+
+#include <tunables/global>
+
+profile LIBVIRT_TEMPLATE {
+  #include <abstractions/libvirt-qemu>
+}
diff -Nurp ./libvirt.orig/examples/apparmor/usr.bin.virt-aa-helper ./libvirt/examples/apparmor/usr.bin.virt-aa-helper
--- ./libvirt.orig/examples/apparmor/usr.bin.virt-aa-helper	1969-12-31 18:00:00.000000000 -0600
+++ ./libvirt/examples/apparmor/usr.bin.virt-aa-helper	2009-09-04 15:42:13.000000000 -0500
@@ -0,0 +1,22 @@
+# Last Modified: Mon Jul  06 17:22:37 2009
+#include <tunables/global>
+
+/usr/bin/virt-aa-helper {
+  #include <abstractions/base>
+
+  # needed for searching directories
+  capability dac_override,
+  capability dac_read_search,
+
+  # needed for when disk is on a network filesystem
+  network inet,
+
+  deny @{PROC}/[0-9]*/mounts r,
+  @{PROC}/filesystems r,
+
+  /usr/bin/virt-aa-helper mr,
+  /sbin/apparmor_parser Ux,
+
+  /etc/apparmor.d/libvirt/* r,
+  /etc/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw,
+}
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-04 15:42:13.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,
+
+  network inet stream,
+
+  # Very lenient profile for libvirtd since we want to first focus on confining
+  # the guests. Guests will have a very restricted profile.
+  /** rwmkl,
+
+  /bin/* Ux,
+  /sbin/* Ux,
+  /usr/bin/* Ux,
+  /usr/sbin/* Ux,
+
+  # force the use of virt-aa-helper
+  audit deny /sbin/apparmor_parser rwxl,
+  audit deny /etc/apparmor.d/libvirt/** wxl,
+  audit deny /sys/kernel/security/apparmor/features rwxl,
+  audit deny /sys/kernel/security/apparmor/matching rwxl,
+  audit deny /sys/kernel/security/apparmor/.* rwxl,
+  /sys/kernel/security/apparmor/profiles r,
+  /usr/bin/virt-aa-helper Pxr,
+
+  # allow changing to our UUID-based named profiles
+  change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
+
+}

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]