6:15 a.m.
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.
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.
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(a)bupt.edu.cn>
+ * Changyao Han <changyao(a)bupt.edu.cn>
+ *
+ * Updated to libvirt v1.2.15: (Original was written for libvirt v1.1.4)
+ * Raghuram S. Sudhaakar <rsudhaak(a)cisco.com>
+ * Randy Aybar <raybar(a)cisco.com>
+ *
+ * Based on security_selinux.c by James Morris <jmorris(a)namei.org>
+ * and security_apparmor.c by Jamie Strandboge <jamie(a)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 :|