On Thu, Apr 14, 2016 at 07:47:23PM +0000, Randy Aybar (raybar) wrote:
> First time RFC/mailing list/community submission/etc. so please
> forgive if not in the appropriate format.

Thanks for taking the time to submit this, and sorry for not giving
feedback on it sooner. Next time feel free to send "ping" responses
if you've not had feedback on a patch after a week or so and you
want to remind people!


> I. Introduction
>
> Libvirt currently has support for two of the various Linux security modules
> (LSM), AppArmor and SELinux. While these work well enough for most cases, there
> are others in which a platform may require less complexity and overhead.
>
> Smack LSM (http://schaufler-ca.com/description_from_the_linux_source_tree)
> intrinsically provides access control with much more simplicity while
> remaining secure. A subject-object model is adapted with an access type
> relationship to determine if permissions or granted or denied.

Yep, we're more than happy to accept patches to support any LSM module
that is in the mainline kernel tree, as we're aware not everyone uses
SELinux / AppArmor

> Initial findings on the topic yielded us to a research paper worked on by
> students at Beijing University of Posts and Telecommunications. As part of the
> paper's objective to create a lightweight approach to secure deployment of
> virtual machines in the cloud, a Smack security driver was implemented for use
> with QEMU-based virtual machines spawned by using the Libvirt API. After
> contacting the authors of the papers, they were more than willing to share the
> source code.

'sharing the source code' can mean many different things to different
people. Can you just confirm explicitly that they gave their permission
for their contributions being licensed under the "LGPLv2.1 or later"
which libvirt uses.

I can confirm that the intent of the authors from BUPT (who wrote an initial version of the smack driver for 1.1.4) is to upstream the code. However, they did not do it for various reasons. Their contributions are included in the copyright header of all files. They are also ‘cced and can explicitly state their intent if required. 

> The Smack driver was initially derived from parts of the SELinux

> and AppArmor drivers as noted by the authors and uses the Libsmack library to
> assign Smack labels when needed.

Ok, libsmack.so is under the LGPLv2.1 which should be ok.

> Another thing to note is that the Smack security driver itself depends on a security
> interface when fully enforced with namespaces. This interface is brought in
> by the Linux kernel in 4.3 but our team has backported and tested Libvirt with version
> 3.19 kernel. Details on why this was necessary found below in LXC container
> changes. Link that references the change in the kernel:
>
> https://lwn.net/Articles/660675/

Ok, we should probably document that security requirement, probably in the
docs/drvlxc.html.in file.

> i. LXC Container (src/lxc/lxc_container.c)
>
> a. Reordering of dropping capabilities and setting up security labeling
>
> Upon forking into a new user namespace (lxcContainerSetID), the child process no
> longer has the ability to override LSM and access folders that have been previously assigned
> Smack labels. This is important to note since after entering the namespace, the child
> still needs to share mount devpts and pivot root however the process does not
> take on the container's label until much after this setup. Thus a suggestion to
> move the set process label (virSecurityManagerSetProcessLabel) after setting namespaces is made.
>
> Dropping capabilities was also moved after sending the continue signal to the
> parent. We had come across an issue as well with setting up FDs and the
> container would fail to start.

Yep that's fine - there's no particular reason for the current ordering - its
just historically chosen.

>
> b. Adding a call to set the child's process label (virSecurityManagerSetChildProcessLabel)
>
> When the child process is spawned from lxcContainerStart, although it has full
> capabilities in its own namespace (after SetUID), it is unable to change its own security
> context (label). Currently the only workaround is to incorporate an
> upcoming patch to the Smack LSM (kernel base) that allows an unprivileged
> process to change its context only if the label its trying to acquire exists in
> a predefined list held by the parent.
>
> With the patch in place, the LXC driver needs to ensure that the label makes it
> to the list of the parent so that the child can continue to take on the context
> which brings a change to lxcContainerStart.
>
> Although the SetChildProcessLabel is a hook that is primarily used by QEMU, we
> decided to use it as the name best fit the operation being performed.
> A check is implemented to ensure that it only continues if Smack is the driver
> being used since this does not need to occur in the other LSMs.
>
> Within the Smack driver itself, the hook also performs a check to see if LXC is
> used otherwise skip the block pertaining to LXC. In the LXC code, it checks
> to see if the Smack relabel-self interface exists and if namespaces is being
> used to continue. The label is written with a simple write call to the file. The
> logic is allowed to flow into the QEMU portion since no command is passed in,
> the function acts as a noop and safely exits.
>
> Any comments for improvement on the logic handled in this change are highly
> appreciated!

So you moved the cal to SetProcessLabel to quite early in the
lxcContainerChild() method, and then added a second call to
SetChildProcessLabel right before virCommandExec().

With SELinux there's no functional difference between the
SetProcessLabel and SetChildProcessLabel - both end up
calling setexeccon_raw() which tells SELinux to change the
label when execve() is called.

There might be a difference for AppArmor, but if there is,
I wouldn't be surprised if it doesn't matter.

IOW, I think we can probably just remove the SetProcessLabel
call entirely, and only ever call SetChildProcessLabel
for all drivers.

For SMACK, if we remove SetProcessLabel the init process of the container does not get labelled and hence the container won’t stand up properly. So we had to leave that in. 

> ii. LXC Controller (src/lxc/lxc_controller.c)
>
> a. Add logic to label the special devices created under
> (/var/run/libvirt/lxc/container.XXX)
>
> The following function calls setup the appropriate special devices such as
> ttys, ptmx, null, etc. (/dev)
>
> virLXCControllerSetupDev
> virLXCControllerPopulateDevices
> virLXCControllerSetupDevPTS
> virLXCControllerSetupConsoles
>
> It was decided to add the call to do a label operation in these functions due to
> the fact that chown'ing also happened at these locations and wanted to provide
> some uniformity in that sense. Since the security hook to label these are no-op
> in all but the Smack driver it's just a straightforward call with the usual
> check to see if it succeeded.

Yep, that makes sense.

> iii. Security Drivers (src/security)
>
> Changes to security driver model include an additional hook to apply a security
> label by passing in a string. Although there exists functions made for applying
> security labels, complications arose from using either.
>
> The first of these is SetImageLabel which requires to pass in a virStorageSource
> that holds the string. LXC does not make use of this data structure and
> initializing one for the sake of a string did not seem feasible due to the
> numerous and differing members the struct consisted of.

Agreed.

> The second function major function to apply security labels is SetImageFDLabel
> which uses a file descriptor rather than a string text. While this approach
> seemed more promising there were hurdles that came across while attempting to
> label items. A file descriptor would be needed to be opened for each special
> device available. However devices like tty could not be opened without being
> issued as a controlling tty by the process. Note that a workaround was attempted
> to change the open call to include the O_PATH flag and obtain the path by looking up the
> location the FD pointed to in the process's list of file descriptors within
> /proc. While this change worked for the tty case, it was dismissed due to the
> additional changes that may have been needed to the other security drivers as we
> were targeting to minimize the changes. This solution may be revisited upon further
> testing and review.
>
> a. Update Definitions for DAC, Stack, AppArmor, SELinux, Smack,
> driver base (security_driver.h), and driver manager (security_manager.c,h)
>
> Changes here involve updated the driver's definitions to include the new hook,
> virSecurityDomainSetImagePathLabel. All drivers except Smack implement this by
> just returning 0 to not disrupt the flow in a non-Smack setup.
>
> b. Updates to Smack driver to support namespaces with LXC
>
> Already described include changes to implement the extra hook to label a file
> using a path as well as SetChildProcessLabel wihthin the driver to
> include the process of adding a label to the approved list via relabel-self.
>
> The driver itself was cleaned up a bit and refactored to eliminate the use of
> excessive amount of helper functions and rely on the API provided by the
> libsmack library. Function names were refactored to match the style of the
> SELinux driver (after the dropping of an extra "Security" in the function's
> name).
>
> iv. Support for entering namespaces in Smack (src/libvirt-lxc.c)
>
> A small addition was made to enable support for Smack using the
> lxc-enter-namespace and was merely a mirror of the other security drivers
> included in this file. Smack in this case uses libsmack API call to label the
> process entering the namespace.

> This patch has been created for patching Libvirt 1.3.1 base source code.

Generally we like to have patches created against current GIT master
revision. Could you rebase to master when addressing feedback.

> diff --git a/configure.ac b/configure.ac
> index 047ad3b..12d0bb8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -253,6 +253,7 @@ LIBVIRT_CHECK_READLINE
>  LIBVIRT_CHECK_SANLOCK
>  LIBVIRT_CHECK_SASL
>  LIBVIRT_CHECK_SELINUX
> +LIBVIRT_CHECK_SMACK
>  LIBVIRT_CHECK_SSH2
>  LIBVIRT_CHECK_SYSTEMD_DAEMON
>  LIBVIRT_CHECK_UDEV
> @@ -1535,6 +1536,27 @@ if test "$with_apparmor" = "no"; then
>  fi
>  AM_CONDITIONAL([WITH_APPARMOR_PROFILES], [test "$with_apparmor_profiles" != "no"])
>
> +AC_ARG_WITH([secdriver-smack],
> +  [AS_HELP_STRING([--with-secdriver-smack],
> +    [use Smack security driver @<:@default=check@:>@])],
> +  [],
> +  [with_secdriver_smack=check])
> +
> +if test "$with_smack" != "yes" ; then
> +  if test "$with_secdriver_smack" = "check" ; then
> +    with_secdriver_smack=no
> +  fi
> +  if test "$with_secdriver_smack" != "no" ; then
> +    AC_MSG_ERROR([You must install the Smack development package in order to compile libvirt])
> +  fi
> +elif test "with_secdriver_smack" != "no" ; then
> +  with_secdriver_smack=yes
> +  AC_DEFINE_UNQUOTED([WITH_SECDRIVER_SMACK], 1, [whether Smack security driver is available])
> +fi
> +AM_CONDITIONAL([WITH_SECDRIVER_SMACK], [test "$with_secdriver_smack" != "no"])

I realize you just copied existing pattern for apparmor, but
could you ignore that existing pattern, and just move this
block of code into the LIBVIRT_CHECK_SMACK fnuction you
created too.


> +AC_DEFUN([LIBVIRT_CHECK_SMACK],[
> +  LIBVIRT_CHECK_LIB([SMACK], [smack],
> +                    [smack_set_label_for_self], [sys/smack.h])
> +
> +  AC_ARG_WITH([smack_mount],
> +    [AS_HELP_STRING([--with-smack-mount],
> +      [set Smack mount point @<:@default=check@:>@])],
> +    [],
> +    [with_smack_mount=check])
> +
> +  if test "$with_smack" = "yes"; then
> +    AC_MSG_CHECKING([Smack mount point])
> +    if test "$with_smack_mount" = "check" || test -z "$with_smack_mount"; then
> +      if test -d /sys/fs/smackfs ; then
> +        SMACK_MOUNT=/sys/fs/smackfs
> +      else
> +        SMACK_MOUNT=/smack
> +      fi

Do any distros exist which use /smack as a path - I know that
/selinux was used in Fedora/RHEL for a long time, but now
uses /sys/fs/selinux.  If we don't need to support /smack
we should just fix /sys/fs/smackfs as the standard location


> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index c5a70a1..71203aa 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2206,6 +2209,10 @@ static int lxcContainerChild(void *data)
>      if (lxcContainerSetID(vmDef) < 0)
>          goto cleanup;
>
> +    VIR_DEBUG("Setting up security labeling");
> +    if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0)
> +        goto cleanup;
> +

As mentioned earlier, I think you can just drop this....

>      root = virDomainGetFilesystemForTarget(vmDef, "/");
>
>      if (argv->nttyPaths) {
> @@ -2254,20 +2261,12 @@ static int lxcContainerChild(void *data)
>          goto cleanup;
>      }
>
> -    /* drop a set of root capabilities */
> -    if (lxcContainerDropCapabilities(vmDef, !!hasReboot) < 0)
> -        goto cleanup;
> -
>      if (lxcContainerSendContinue(argv->handshakefd) < 0) {
>          virReportSystemError(errno, "%s",
>                              _("Failed to send continue signal to controller"));
>          goto cleanup;
>      }
>
> -    VIR_DEBUG("Setting up security labeling");
> -    if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0)
> -        goto cleanup;
> -
>      VIR_DEBUG("Setting up inherited FDs");
>      VIR_FORCE_CLOSE(argv->handshakefd);
>      VIR_FORCE_CLOSE(argv->monitor);
> @@ -2275,6 +2274,10 @@ static int lxcContainerChild(void *data)
>                               argv->npassFDs, argv->passFDs) < 0)
>          goto cleanup;
>
> +    /* drop a set of root capabilities */
> +    if (lxcContainerDropCapabilities(vmDef, !!hasReboot) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      VIR_FREE(ttyPath);
> @@ -2389,6 +2392,16 @@ int lxcContainerStart(virDomainDefPtr def,
>          if (userns_supported()) {
>              VIR_DEBUG("Enable user namespace");
>              cflags |= CLONE_NEWUSER;
> +#ifdef WITH_SMACK
> +            if(STREQ(virSecurityManagerGetModel(securityDriver),"smack") &&
> +               virSecurityManagerSetChildProcessLabel(securityDriver,
> +                                                      def,
> +                                                      NULL) < 0) {

And remove the check for GetModel() == "smack" - just let all drivers
use this codepath - though it would hae to be outside the userns_supported()
check

> +                   virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to send label to relabel interface."));
> +                   return -1;
> +             }
> +#endif
>          } else {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                             _("Kernel doesn't support user namespace"));

> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 438103a..37080b8 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c

> @@ -1476,6 +1479,9 @@ static int virLXCControllerSetupDev(virLXCControllerPtr ctrl)
>
>      if (lxcContainerChown(ctrl->def, dev) < 0)
>          goto cleanup;
> +
> +    if (virSecurityManagerSetImagePathLabel(ctrl->securityManager,ctrl->def,dev) < 0)
> +        goto cleanup;

BTW, there should always be a space after a ',' in an argument list.
If you run 'make syntax-check' it should warn you about style mistakes
like this.


>
>      ret = 0;
>   cleanup:
> @@ -1525,6 +1531,11 @@ static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
>          if (lxcContainerChown(ctrl->def, path) < 0)
>              goto cleanup;
>
> +        if (virSecurityManagerSetImagePathLabel(ctrl->securityManager,
> +                                                ctrl->def,
> +                                                path) < 0)
> +            goto cleanup;
> +
>          VIR_FREE(path);
>      }
>
> @@ -2183,6 +2194,14 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
>          (lxcContainerChown(ctrl->def, devpts) < 0))
>          goto cleanup;
>
> +    if ((virSecurityManagerSetImagePathLabel(ctrl->securityManager,
> +                                        ctrl->def,
> +                                        ctrl->devptmx)) < 0 ||

This has for whitespace at the end of lines - again just run
'make syntax-check' and fix any problems it reports. Generally
you want to align parameters on each line with the '('

> diff --git a/src/security/security_smack.c b/src/security/security_smack.c
> new file mode 100644
> index 0000000..50301ad
> --- /dev/null
> +++ b/src/security/security_smack.c
> @@ -0,0 +1,1519 @@
> +/*
> + * Copyright (C) 2015 Cisco Systems, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author:
> + *   Hongliang Liang <hliang@bupt.edu.cn>
> + *   Changyao Han <changyao@bupt.edu.cn>
> + *
> + * Updated to libvirt v1.2.15: (Original was written for libvirt v1.1.4)
> + *   Raghuram S. Sudhaakar <rsudhaak@cisco.com>
> + *   Randy Aybar <raybar@cisco.com>
> + *
> + *   Based on security_selinux.c by James Morris <jmorris@namei.org>
> + *   and security_apparmor.c by Jamie Strandboge <jamie@canonical.com>
> + *
> + *   Smack scurity driver.
> + *
> + */

> +static int
> +virSecuritySmackGetPIDLabel(pid_t pid, char **label)
> +{
> +
> +      char *result;
> +      int fd;
> +      int ret;
> +      char *path;
> +
> +      result = calloc(SMACK_LABEL_LEN + 1, 1);
> +      if (result == NULL)
> +               return -1;

Use of 'calloc' / 'malloc' / 'free' etc is not allowed in
libvirt - use VIR_ALLOC, VIR_FREE, etc

   http://libvirt.org/hacking.html#memalloc


Also, please only indent lines by 4 spaces at a time - not 8.

There's tips here on how to configure emacs or vim to indent
correctly for libvirt:

   http://libvirt.org/hacking.html#indent

> +      ret = virAsprintf(&path, "/proc/%d/attr/current", pid);
> +      if (ret < 0)
> +               return -1;
> +      fd = open(path, O_RDONLY);
> +      VIR_FREE(path);
> +      if (fd < 0) {
> +               free(result);
> +               return -1;
> +      }
> +      ret = read(fd, result, SMACK_LABEL_LEN);
> +      VIR_FORCE_CLOSE(fd);
> +      if (ret < 0) {
> +               free(result);
> +               return -1;
> +      }
> +      *label = result;
> +      return ret;
> +
> +}

You could replace the open/read/close code here with a simple
virFileReadAll() call.

> +int
> +virSecuritySmackSockCreate(const char *label, const char *attr)
> +{
> +      int fd;
> +      int ret = -1;
> +      long int tid;
> +      char *path;
> +      tid = syscall(SYS_gettid);
> +      VIR_DEBUG("/proc/self/task/%ld/attr/%s", tid, attr);
> +      ret = virAsprintf(&path, "/proc/self/task/%ld/attr/%s", tid, attr);
> +      if (ret < 0)
> +               return -1;
> +
> +      VIR_DEBUG("virSecuritySmackSockCreate pid is in %d", getpid());
> +      VIR_DEBUG("real user ID is in %d", getuid());
> +      VIR_DEBUG("effective user ID is in %d", geteuid());
> +      VIR_DEBUG("label from self %s", label);
> +      VIR_DEBUG("location /proc/self/attr/%s", attr);
> +
> +      if (label) {
> +               fd = open(path, O_WRONLY | O_CLOEXEC);
> +               VIR_DEBUG("open file %s", path);
> +               VIR_FREE(path);
> +               if (fd < 0) {
> +                             VIR_DEBUG("open fail");
> +                             return -1;
> +               }
> +               VIR_DEBUG("open success");
> +               do {
> +                             ret = write(fd, label, strlen(label) + 1);
> +               } while (ret < 0 && errno == EINTR);

You can use virFileWriteStr() to write a string to a file.

> +      }
> +      else {
> +               fd = open(path, O_TRUNC);
> +               VIR_FREE(path);
> +               if (fd < 0)
> +                             return -1;
> +               ret = 0;
> +      }
> +
> +      close(fd);

Use VIR_CLOSE or VIR_FORCE_CLOSE

> +
> +      return (ret < 0) ? -1 : 0;
> +
> +}
> +



Broadly speaking this looks like a reasonable design. Biggest thing todo
is rebase to current git master & run 'make syntax-check' to fix all
the problems it shows, as that'll make it nicer to review again.

Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|